Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410353007:
Add CT isolate file and CT script that will be run on swarming slaves (Closed)
DescriptionAdd CT isolate file and CT script that will be run on swarming slaves.
The code here will be invoked by the recipe in https://codereview.chromium.org/1423993007/
BUG=skia:4503
Committed: https://crrev.com/e5d22be8e7f8a612a08753d698e8dc33b366a305
Cr-Commit-Position: refs/heads/master@{#360322}
Patch Set 1 : Initial upload #Patch Set 2 : Add isolate file template #Patch Set 3 : checkpoint #Patch Set 4 : Checkpoint #Patch Set 5 : Checkpoint #Patch Set 6 : Working end-to-end #Patch Set 7 : Cleanup isolate template file #Patch Set 8 : Output in slave specific directory #Patch Set 9 : Going to look at perf upload #Patch Set 10 : Pass in git_hash, master, builder #Patch Set 11 : Remove csv_pivot_table_merger #Patch Set 12 : Cleanup #Patch Set 13 : Remove reference to telemetry.isolate #Patch Set 14 : Move wprs to telemetry data dir #
Total comments: 6
Patch Set 15 : Address comments #
Total comments: 2
Patch Set 16 : Add testing/variations #Patch Set 17 : Setup executable bit for presubmit #
Total comments: 5
Patch Set 18 : Address comments #
Total comments: 12
Patch Set 19 : Address comments #
Total comments: 3
Messages
Total messages: 63 (30 generated)
Description was changed from ========== Checkpoint BUG= ========== to ========== Adds a CT isolate template and the CT script that will be run on swarming slaves. BUG=skia:4503 ==========
Description was changed from ========== Adds a CT isolate template and the CT script that will be run on swarming slaves. BUG=skia:4503 ========== to ========== Add CT isolate template and CT script that will be run on swarming slaves. BUG=skia:4503 ==========
Description was changed from ========== Add CT isolate template and CT script that will be run on swarming slaves. BUG=skia:4503 ========== to ========== Add CT isolate template and CT script that will be run on swarming slaves. The code here will be invoked by the recipe in https://codereview.chromium.org/1423993007/ BUG=skia:4503 ==========
rmistry@google.com changed reviewers: + nednguyen@google.com
Hi Ned, Since https://codereview.chromium.org/1423993007/ is very close, could you please review this CL? (it will be invoked by the recipe in https://codereview.chromium.org/1423993007/). Thanks!
https://codereview.chromium.org/1410353007/diff/260001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/260001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:10: '../tools/perf/chrome_telemetry_build/telemetry_binary_manager.isolate', Telemetry & perf dependencies can be found fully described with https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/perf.is... If not, we should add more deps to that isolate instead. https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:8: import optparse Let's use argparse :-) https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:51: shutil.move(os.path.join(ct_data_dir, wpr), telemetry_data_dir) I think it's better for ct page sets to know how to specify the wpr data dir rather than doing this copy, since this risks creating file names collision.
https://codereview.chromium.org/1410353007/diff/260001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/260001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:10: '../tools/perf/chrome_telemetry_build/telemetry_binary_manager.isolate', On 2015/11/13 18:24:39, nednguyen wrote: > Telemetry & perf dependencies can be found fully described with > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/perf.is... > > > If not, we should add more deps to that isolate instead. I get this error when I use perf.isolate when running locally: IsolateError: These configuration variables were missing from the command line: CONFIGURATION_NAME, asan, component, disable_nacl, fastbuild, icu_use_data_file_flag, kasko, lsan, msan, msvs_version, target_arch, tsan, use_custom_libcxx, use_instrumented_libraries, use_prebuilt_instrumented_libraries, v8_use_external_startup_data It could be that those variables are automatically provided when running on the waterfall. I'll turn it on once its live to see what happens. Updated the TODO. https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:8: import optparse On 2015/11/13 18:24:39, nednguyen wrote: > Let's use argparse :-) Done. https://codereview.chromium.org/1410353007/diff/260001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:51: shutil.move(os.path.join(ct_data_dir, wpr), telemetry_data_dir) On 2015/11/13 18:24:39, nednguyen wrote: > I think it's better for ct page sets to know how to specify the wpr data dir > rather than doing this copy, since this risks creating file names collision. Agreed, this was a temporary hack I forgot to update. Done.
lgtm https://codereview.chromium.org/1410353007/diff/280001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/280001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:22: '../tools/variations/', You also need '../testing/variations/', otherwise the finch trial flags will be empty.
https://codereview.chromium.org/1410353007/diff/280001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/280001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:22: '../tools/variations/', On 2015/11/16 17:49:49, nednguyen wrote: > You also need '../testing/variations/', otherwise the finch trial flags will be > empty. Done.
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/1410353007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/1410353007/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/16 19:52:34, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. About to land this because it is needed for https://codereview.chromium.org/1423993007/
The CQ bit was checked by rmistry@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1410353007/#ps320001 (title: "Setup executable bit for presubmit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410353007/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rmistry@google.com changed reviewers: + maruel@chromium.org, phajdan.jr@chromium.org
Missing OWNERs LGTM. Added Pawel for content/test/ct/... Added M-A for the isolate template (I think you already looked at this via https://codereview.chromium.org/1423993007/ but not sure).
https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:35: '--slave_num=[[SLAVE_NUM]]', You should use native isolate variable support. Then you don't need to make a template. It's used via the --path-variable that is also usable for arguments. https://codereview.chromium.org/1410353007/diff/320001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/320001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:65: subprocess.call(cmd) return subprocess.call(cmd) otherwise you silently succeed.
https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:35: '--slave_num=[[SLAVE_NUM]]', On 2015/11/16 20:22:02, M-A Ruel wrote: > You should use native isolate variable support. > > Then you don't need to make a template. It's used via the --path-variable that > is also usable for arguments. That sounds good. I modified things and tried to pass in: --path-variable MASTER chromium.perf.fyi --path-variable SLAVE_NUM 1 but it fails with: File "/repos/chromium-tools/build/slave/fake_slave/build/swarming.client/isolate.py", line 153, in <genexpr> for k, v in path_variables.iteritems()) File "/repos/chromium-tools/build/slave/fake_slave/build/swarming.client/isolate.py", line 130, in _normalize_path_variable raise ExecutionError('%s=%s is not a directory' % (key, normalized)) ExecutionError: MASTER=/repos/chromium-tools/build/slave/fake_slave/build/src/chrome/chromium.perf.fyi is not a directory Looks like it expects a directory and always fails if it is not a dir. Is there another way to pass in args?
PTAL https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... File chrome/ct_top1k.isolate.tmpl (right): https://codereview.chromium.org/1410353007/diff/320001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate.tmpl:35: '--slave_num=[[SLAVE_NUM]]', On 2015/11/17 13:00:56, rmistry wrote: > On 2015/11/16 20:22:02, M-A Ruel wrote: > > You should use native isolate variable support. > > > > Then you don't need to make a template. It's used via the --path-variable that > > is also usable for arguments. > > That sounds good. I modified things and tried to pass in: > --path-variable MASTER chromium.perf.fyi > --path-variable SLAVE_NUM 1 > > but it fails with: > > File > "/repos/chromium-tools/build/slave/fake_slave/build/swarming.client/isolate.py", > line 153, in <genexpr> > for k, v in path_variables.iteritems()) > File > "/repos/chromium-tools/build/slave/fake_slave/build/swarming.client/isolate.py", > line 130, in _normalize_path_variable > raise ExecutionError('%s=%s is not a directory' % (key, normalized)) > ExecutionError: > MASTER=/repos/chromium-tools/build/slave/fake_slave/build/src/chrome/chromium.perf.fyi > is not a directory > > > Looks like it expects a directory and always fails if it is not a dir. Is there > another way to pass in args? Ah, it works with --extra-variable. Done. https://codereview.chromium.org/1410353007/diff/320001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/320001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:65: subprocess.call(cmd) On 2015/11/16 20:22:02, M-A Ruel wrote: > return subprocess.call(cmd) > otherwise you silently succeed. Done.
The CQ bit was checked by rmistry@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1410353007/#ps340001 (title: "Address comments")
The CQ bit was unchecked by rmistry@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/1410353007/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/340001
https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:6: """This script is meant to be run on a Swarming slave.""" Swarming bot :) https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:10: import path_util This one is not stdlib, so it should be in separate group below https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:11: import shutil Not used. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:23: PARENT_DIR = os.path.dirname(os.path.realpath(__file__)) I'd prefer this to be done before line 16. Who know what line 16 does. :) https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:50: os.chmod(ct_binary_path, os.stat(ct_binary_path).st_mode | stat.S_IEXEC) If the file is stored as executable in the tree before isolation, you shouldn't need this (which I'd prefer)
Do a +x on run_chromium_perf_swarming in this CL if you can (?) https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:21: CT_BINARY = 'run_chromium_perf_swarming' Don't use global constants unless you use it more than once. Otherwise it's obfuscation by indirection. :)
Patchset #19 (id:360001) has been deleted
Patchset #19 (id:380001) has been deleted
Patchset #19 (id:400001) has been deleted
Patchset #19 (id:420001) has been deleted
Patchset #19 (id:440001) has been deleted
https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... File content/test/ct/run_ct_top1k.py (right): https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:6: """This script is meant to be run on a Swarming slave.""" On 2015/11/17 16:15:33, M-A Ruel wrote: > Swarming bot :) Done. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:10: import path_util On 2015/11/17 16:15:33, M-A Ruel wrote: > This one is not stdlib, so it should be in separate group below Done. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:11: import shutil On 2015/11/17 16:15:33, M-A Ruel wrote: > Not used. Done. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:21: CT_BINARY = 'run_chromium_perf_swarming' On 2015/11/17 16:30:36, M-A Ruel wrote: > Don't use global constants unless you use it more than once. Otherwise it's > obfuscation by indirection. :) Done. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:23: PARENT_DIR = os.path.dirname(os.path.realpath(__file__)) On 2015/11/17 16:15:33, M-A Ruel wrote: > I'd prefer this to be done before line 16. Who know what line 16 does. :) Done. https://codereview.chromium.org/1410353007/diff/340001/content/test/ct/run_ct... content/test/ct/run_ct_top1k.py:50: os.chmod(ct_binary_path, os.stat(ct_binary_path).st_mode | stat.S_IEXEC) On 2015/11/17 16:15:33, M-A Ruel wrote: > If the file is stored as executable in the tree before isolation, you shouldn't > need this (which I'd prefer) The binary is downloaded from Google storage, I moved this line to where the file is downloaded.
lgtm https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolate File chrome/ct_top1k.isolate (right): https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate:25: '../out/Release/', FTR, we generally use <(PRODUCT_DIR), otherwise you defacto do not support Release_x64 on Windows and Debug.
https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolate File chrome/ct_top1k.isolate (right): https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate:25: '../out/Release/', On 2015/11/17 18:31:49, M-A Ruel wrote: > FTR, we generally use <(PRODUCT_DIR), otherwise you defacto do not support > Release_x64 on Windows and Debug. Do I have to explicitly set PRODUCT_DIR with path_variables (or is it magically set somewhere)?
https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolate File chrome/ct_top1k.isolate (right): https://codereview.chromium.org/1410353007/diff/460001/chrome/ct_top1k.isolat... chrome/ct_top1k.isolate:25: '../out/Release/', On 2015/11/17 18:33:57, rmistry wrote: > On 2015/11/17 18:31:49, M-A Ruel wrote: > > FTR, we generally use <(PRODUCT_DIR), otherwise you defacto do not support > > Release_x64 on Windows and Debug. > > Do I have to explicitly set PRODUCT_DIR with path_variables (or is it magically > set somewhere)? No you have to set it, so for now hardcoding this path here is fine and this can be fixed later.
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/1410353007/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rmistry@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1410353007/#ps460001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410353007/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410353007/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/460001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
LGTM
The CQ bit was checked by rmistry@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410353007/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410353007/460001
Message was sent while issue was closed.
Committed patchset #19 (id:460001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/e5d22be8e7f8a612a08753d698e8dc33b366a305 Cr-Commit-Position: refs/heads/master@{#360322}
Message was sent while issue was closed.
Description was changed from ========== Add CT isolate template and CT script that will be run on swarming slaves. The code here will be invoked by the recipe in https://codereview.chromium.org/1423993007/ BUG=skia:4503 Committed: https://crrev.com/e5d22be8e7f8a612a08753d698e8dc33b366a305 Cr-Commit-Position: refs/heads/master@{#360322} ========== to ========== Add CT isolate file and CT script that will be run on swarming slaves. The code here will be invoked by the recipe in https://codereview.chromium.org/1423993007/ BUG=skia:4503 Committed: https://crrev.com/e5d22be8e7f8a612a08753d698e8dc33b366a305 Cr-Commit-Position: refs/heads/master@{#360322} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
