Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1858713002:
Fail run_benchmark if archive_data_file is specified and not found (Closed)
DescriptionFail run_benchmark if archive_data_file is specified and not found.
Created from https://codereview.chromium.org/1856603003/#msg9
BUG=chromium:600154
Patch Set 1 : Initial upload #Patch Set 2 : Add test #Patch Set 3 : Try base path #
Messages
Total messages: 38 (16 generated)
Description was changed from ========== Fail run_benchmark if archive_data_file is specified and not found BUG=600154 ========== to ========== Fail run_benchmark if archive_data_file is specified and not found. Created from https://codereview.chromium.org/1856603003/#msg9 BUG=600154 ==========
rmistry@google.com changed reviewers: + nednguyen@google.com
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-catapult-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by rmistry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-catapult-committers". Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by rmistry@google.com
On 2016/04/04 17:50:47, nednguyen wrote: > lgtm Actually, can you add simple test coverage to story_set_unittest.py? :P
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/1
The CQ bit was unchecked by rmistry@google.com
On 2016/04/04 17:51:18, nednguyen wrote: > On 2016/04/04 17:50:47, nednguyen wrote: > > lgtm > > Actually, can you add simple test coverage to story_set_unittest.py? :P Sure, looking..
Patchset #2 (id:20001) has been deleted
On 2016/04/04 17:51:52, rmistry wrote: > On 2016/04/04 17:51:18, nednguyen wrote: > > On 2016/04/04 17:50:47, nednguyen wrote: > > > lgtm > > > > Actually, can you add simple test coverage to story_set_unittest.py? :P > > Sure, looking.. Done. PTAL.
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
Description was changed from ========== Fail run_benchmark if archive_data_file is specified and not found. Created from https://codereview.chromium.org/1856603003/#msg9 BUG=600154 ========== to ========== Fail run_benchmark if archive_data_file is specified and not found. Created from https://codereview.chromium.org/1856603003/#msg9 BUG=chromium:600154 ==========
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) Actually will this work in story_set? It checks for the file's existence but the file is relative to wherever the page set is.
On 2016/04/04 18:51:43, rmistry wrote: > On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) > > Actually will this work in story_set? It checks for the file's existence but the > file is relative to wherever the page set is. and maybe it makes sense to leave this check for specific page sets, maybe some page sets do not mind using live data when the archive is missing?
On 2016/04/04 18:54:20, rmistry wrote: > On 2016/04/04 18:51:43, rmistry wrote: > > On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > > > Dry run: Try jobs failed on following builders: > > > Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, > > > > > > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) > > > > Actually will this work in story_set? It checks for the file's existence but > the > > file is relative to wherever the page set is. Can we check for os.path.join(archive_data_file, base_dir) instead? > > and maybe it makes sense to leave this check for specific page sets, maybe some > page sets do not mind using live data when the archive is missing? If that is the case, the page_sets should explicitly set the directory to None. Otherwise, I think the page sets using live data is a very surprising behavior (like your case).
The CQ bit was checked by rmistry@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1858713002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1858713002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
On 2016/04/04 19:07:42, nednguyen wrote: > On 2016/04/04 18:54:20, rmistry wrote: > > On 2016/04/04 18:51:43, rmistry wrote: > > > On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > > > > Dry run: Try jobs failed on following builders: > > > > Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, > > > > > > > > > > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) > > > > > > Actually will this work in story_set? It checks for the file's existence but > > the > > > file is relative to wherever the page set is. > > Can we check for os.path.join(archive_data_file, base_dir) instead? > > > > and maybe it makes sense to leave this check for specific page sets, maybe > some > > page sets do not mind using live data when the archive is missing? > > If that is the case, the page_sets should explicitly set the directory to None. > Otherwise, I think the page sets using live data is a very surprising behavior > (like your case). This no longer appears to be a simple couple of lines fix, the catapult tests are failing because the test archive files do not exist and whenever this does land in Chromium there might be bot failures because the CL changes behavior. Ned, I recommend you take over this CL and try to see it through into Chromium since you are the most familiar with these moving pieces. Let me know if you need any help with this.
On 2016/04/04 20:34:59, rmistry wrote: > On 2016/04/04 19:07:42, nednguyen wrote: > > On 2016/04/04 18:54:20, rmistry wrote: > > > On 2016/04/04 18:51:43, rmistry wrote: > > > > On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > > > > > Dry run: Try jobs failed on following builders: > > > > > Catapult Mac Tryserver on master.tryserver.client.catapult > (JOB_FAILED, > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) > > > > > > > > Actually will this work in story_set? It checks for the file's existence > but > > > the > > > > file is relative to wherever the page set is. > > > > Can we check for os.path.join(archive_data_file, base_dir) instead? > > > > > > and maybe it makes sense to leave this check for specific page sets, maybe > > some > > > page sets do not mind using live data when the archive is missing? > > > > If that is the case, the page_sets should explicitly set the directory to > None. > > Otherwise, I think the page sets using live data is a very surprising behavior > > (like your case). > > This no longer appears to be a simple couple of lines fix, the catapult tests > are failing because the test archive files do not exist and whenever this does > land in Chromium there might be bot failures because the CL changes behavior. > Ned, I recommend you take over this CL and try to see it through into Chromium > since you are the most familiar with these moving pieces. > Let me know if you need any help with this. Actually for pointing you to the wrong direction, I think the we intentionally don't check for non existing archive_data_file when instantiating the story_set because in recording mode, it should create the the archive_data_file on fly for you. I make a patch in which I change the archive_data_file in Top25Smooth story set & story_runner throw exception because the archive data file doesn't exist: https://codereview.chromium.org/1859003002
On 2016/04/05 03:48:17, nednguyen wrote: > On 2016/04/04 20:34:59, rmistry wrote: > > On 2016/04/04 19:07:42, nednguyen wrote: > > > On 2016/04/04 18:54:20, rmistry wrote: > > > > On 2016/04/04 18:51:43, rmistry wrote: > > > > > On 2016/04/04 18:49:04, commit-bot: I haz the power wrote: > > > > > > Dry run: Try jobs failed on following builders: > > > > > > Catapult Mac Tryserver on master.tryserver.client.catapult > > (JOB_FAILED, > > > > > > > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) > > > > > > > > > > Actually will this work in story_set? It checks for the file's existence > > but > > > > the > > > > > file is relative to wherever the page set is. > > > > > > Can we check for os.path.join(archive_data_file, base_dir) instead? > > > > > > > > and maybe it makes sense to leave this check for specific page sets, maybe > > > some > > > > page sets do not mind using live data when the archive is missing? > > > > > > If that is the case, the page_sets should explicitly set the directory to > > None. > > > Otherwise, I think the page sets using live data is a very surprising > behavior > > > (like your case). > > > > This no longer appears to be a simple couple of lines fix, the catapult tests > > are failing because the test archive files do not exist and whenever this does > > land in Chromium there might be bot failures because the CL changes behavior. > > Ned, I recommend you take over this CL and try to see it through into Chromium > > since you are the most familiar with these moving pieces. > > Let me know if you need any help with this. > > Actually for pointing you to the wrong direction, I think the we intentionally > don't check for non existing archive_data_file when instantiating the story_set > because in recording mode, it should create the the archive_data_file on fly for > you. > > I make a patch in which I change the archive_data_file in Top25Smooth story set > & story_runner throw exception because the archive data file doesn't exist: > https://codereview.chromium.org/1859003002 Close this CL since it's probably wrong change to make. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
