Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1045553003:
Add support to conditionally running telemetry benchmarks on CQ. (Closed)
DescriptionAdd support to conditionally running telemetry benchmarks on CQ.
This is one of 3 changes that are required to complete conditionally running
benchmarks on CQ. The other two are adding post_upload_hooks to telemetry presubmit
and passing build_properties to run-bisect-perf-regression on buildbot.
BUG=462581
Committed: https://crrev.com/155d7e5b71ca9b4e6ea5b86d5edfc503862f1a60
Cr-Commit-Position: refs/heads/master@{#324062}
Patch Set 1 #
Total comments: 39
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Messages
Total messages: 16 (3 generated)
prasadv@chromium.org changed reviewers: + prasadv@chromium.org, qyearsley@chromium.org, simonhatch@chromium.org, sullivan@chromium.org
On 2015/03/30 19:56:57, prasadv wrote: Nice, this is going to be awesome. Maybe we're at the point we should consider breaking this file up? It's the entry point for bisects, perf try jobs, and now the CQ jobs. I could see refactoring into like bisect_runner, etc. wdyt?
On 2015/03/31 15:38:45, shatch wrote: > On 2015/03/30 19:56:57, prasadv wrote: > > Nice, this is going to be awesome. > > Maybe we're at the point we should consider breaking this file up? It's the > entry point for bisects, perf try jobs, and now the CQ jobs. I could see > refactoring into like bisect_runner, etc. wdyt? That would definitely make it less confusing, IMO :-) Meanwhile, in the future auto-bisect, jobs will be started by making a request to buildbucket, with the parameters given as JSON, with no need to run this script or read a config file. I would still love to see this script divided into several with different tasks.
First pass of comments. Looks good. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:451: cq_tryjob: Determines if the try job is initiated by commit queue. This could be named is_cq_tryjob, since it's a boolean. Alternate description: is_cq_tryjob: Whether or not the try job was initiated by commit queue. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:580: # and the browser to use may depend on the platform. This comment seems to me like it could be omitted. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:587: return browser Slightly shorter alternative: def _GetBrowserType(bot_platform): if bot_platform == 'android': return 'android-chrome-shell' if 'x64' in bot_platform: return 'release_x64' return 'release' https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:590: def _GuessTestCommand(bot_platform, test_name=None): 1. Possible alternate name: _GuessTelemetryTestCommand 2. Does it make sense to have a test command with no test name? https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:591: """Creates a telemetry benchmark command based on bot and test name.""" For consistency, I would capitalize Telemetry everywhere. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:595: command.append('python') I wonder whether we could add "python" for all platforms? e.g. command = [ 'python', 'tools/perf/run_benchmark', '--verbose', '--browser=%s' % _GetBrowserType(bot_platform)) test_name ] return ' '.join(command) https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:609: if config.has_key('use_goma'): Equivalent: if 'use_goma' in config: ... (has_key is deprecated and removed in Python 3, so "in" is slightly preferred). https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:623: opts_dict['bisect_mode'] = bisect_utils.BISECT_MODE_RETURN_CODE Alternate structure for this function: First put required items in a literal dict, then add optional ones. opts_dict = { 'command': _GuessTestCommand(bot_name, test_name), 'target_arch': 'x64' if 'x64' in bot_name else 'ia32', 'build_preference': 'ninja', 'output_buildbot_annotations': True, 'repeat_test_count': 1, 'bisect_mode': bisect_utils.BISECT_MODE_RETURN_CODE, } if 'use_goma' in config: opts_dict['use_goma'] = config['use_goma'] if 'goma_dir' in config: opts_dict['goma_dir'] = config['goma_dir'] if 'android-chrome-shell' in opts_dict['command']: opts_dict['target_platform'] = 'android' https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:637: """Gets list of modified benchmark files under tools/perf/benchmarks.""" Technically, I wouldn't say that this necessarily returns "benchmarks". For example, if someone modifies just tools/perf/benchmarks/smoothness.py, this will just return ["smoothness"], but the actual benchmark names that correspond to this are smoothness.key_silk_cases, smoothness.maps, etc. So maybe this function could be called _GetAffectedBenchmarkPrefixes? Or something else, like _GetAffectedBenchmarkModuleNames, etc.? https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:643: modified_benchmarks.append(benchamrk) benchamrk -> benchmark https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:689: print 'Results of benchamrks:' benchamrks -> benchmarks https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:697: return not test_status This function will return False for success and True for failure, if test_status is a boolean that is True if all the tests have passed. If you want it to return an int you should: return 0 if test_status else 1 https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:761: def convert_json(option, _, value, parser): Nit: Should be CamelCase. Could be put inside the body of _OptionParser. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:803: help='build properties in JSON format') Nit: Alignment https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:860: config={}, path_to_goma=opts.path_to_goma, cq_tryjob=True) How is it determined which benchmark(s) to run?
https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:11: bisect scrip there. Not related to this CL, but could do a drive-by fix: scrip -> script https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:41: BENCHMARKS_JSON_FILE = 'benchmarks.json' Optionally, you could add comments about these constants. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:661: BENCHMARKS_JSON_FILE]) Would it be possible to get the benchmarks list without writing to a file and then reading it? I wonder whether you can use --json-output and make it output to stdout? https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:860: config={}, path_to_goma=opts.path_to_goma, cq_tryjob=True) On 2015/03/31 21:29:25, qyearsley wrote: > How is it determined which benchmark(s) to run? Never mind -- Upon reading the code more, I can see that it's determined by looking at the patch (git diff --no-ext-diff --name-only HEAD~1) and then listing benchmarks that match any modified files with run_benchmark, then running each of those. Probably no need for more comments here.
I do agree we should move and split run-bisect-perf-regressions.py which is an entry point for both bisect and perf tryjob. But the codes are so tightly coupled that this refactoring might take some time. And I think this effort might be redundant since we are anyhow planning to migrating to recipes part of Q2 OKR. Please let me know your opinion if I can commit this CL first and then work on refactoring in a separate CL.
https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:11: bisect scrip there. On 2015/03/31 22:21:25, qyearsley wrote: > Not related to this CL, but could do a drive-by fix: > scrip -> script Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:41: BENCHMARKS_JSON_FILE = 'benchmarks.json' On 2015/03/31 22:21:25, qyearsley wrote: > Optionally, you could add comments about these constants. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:451: cq_tryjob: Determines if the try job is initiated by commit queue. On 2015/03/31 21:29:24, qyearsley wrote: > This could be named is_cq_tryjob, since it's a boolean. > Alternate description: > is_cq_tryjob: Whether or not the try job was initiated by commit queue. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:580: # and the browser to use may depend on the platform. On 2015/03/31 21:29:23, qyearsley wrote: > This comment seems to me like it could be omitted. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:587: return browser On 2015/03/31 21:29:25, qyearsley wrote: > Slightly shorter alternative: > > def _GetBrowserType(bot_platform): > if bot_platform == 'android': > return 'android-chrome-shell' > if 'x64' in bot_platform: > return 'release_x64' > return 'release' Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:590: def _GuessTestCommand(bot_platform, test_name=None): On 2015/03/31 21:29:25, qyearsley wrote: > 1. Possible alternate name: _GuessTelemetryTestCommand Done > 2. Does it make sense to have a test command with no test name? Since we are going to run test for only for changed benchmarks, And to create a BisectPerformanceMetrics instance we need a command but at this time we don't know which benchmark to run. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:591: """Creates a telemetry benchmark command based on bot and test name.""" On 2015/03/31 21:29:25, qyearsley wrote: > For consistency, I would capitalize Telemetry everywhere. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:595: command.append('python') On 2015/03/31 21:29:23, qyearsley wrote: > I wonder whether we could add "python" for all platforms? e.g. > > command = [ > 'python', > 'tools/perf/run_benchmark', > '--verbose', > '--browser=%s' % _GetBrowserType(bot_platform)) > test_name > ] > return ' '.join(command) I think thi should be possible, but I just followed the way we do from dashboard. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:609: if config.has_key('use_goma'): On 2015/03/31 21:29:24, qyearsley wrote: > Equivalent: > if 'use_goma' in config: > ... > (has_key is deprecated and removed in Python 3, so "in" is slightly preferred). Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:623: opts_dict['bisect_mode'] = bisect_utils.BISECT_MODE_RETURN_CODE On 2015/03/31 21:29:24, qyearsley wrote: > Alternate structure for this function: First put required items in a literal > dict, then add optional ones. > > opts_dict = { > 'command': _GuessTestCommand(bot_name, test_name), > 'target_arch': 'x64' if 'x64' in bot_name else 'ia32', > 'build_preference': 'ninja', > 'output_buildbot_annotations': True, > 'repeat_test_count': 1, > 'bisect_mode': bisect_utils.BISECT_MODE_RETURN_CODE, > } > if 'use_goma' in config: > opts_dict['use_goma'] = config['use_goma'] > if 'goma_dir' in config: > opts_dict['goma_dir'] = config['goma_dir'] > if 'android-chrome-shell' in opts_dict['command']: > opts_dict['target_platform'] = 'android' Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:637: """Gets list of modified benchmark files under tools/perf/benchmarks.""" On 2015/03/31 21:29:24, qyearsley wrote: > Technically, I wouldn't say that this necessarily returns "benchmarks". > > For example, if someone modifies just tools/perf/benchmarks/smoothness.py, this > will just return ["smoothness"], but the actual benchmark names that correspond > to this are smoothness.key_silk_cases, smoothness.maps, etc. > > So maybe this function could be called _GetAffectedBenchmarkPrefixes? Or > something else, like _GetAffectedBenchmarkModuleNames, etc.? Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:643: modified_benchmarks.append(benchamrk) On 2015/03/31 21:29:24, qyearsley wrote: > benchamrk -> benchmark Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:661: BENCHMARKS_JSON_FILE]) On 2015/03/31 22:21:25, qyearsley wrote: > Would it be possible to get the benchmarks list without writing to a file and > then reading it? I wonder whether you can use --json-output and make it output > to stdout? I tried using "list" option to list all benchmarks, which outputs the list of benchmarks along with test description to stdout. And to get benchmark names we need to parse and strip test descriptions from stdout. So as suggested by Simon I'm using JSON the way chromium.perf is used. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:689: print 'Results of benchamrks:' On 2015/03/31 21:29:24, qyearsley wrote: > benchamrks -> benchmarks Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:697: return not test_status On 2015/03/31 21:29:23, qyearsley wrote: > This function will return False for success and True for failure, if test_status > is a boolean that is True if all the tests have passed. > If you want it to return an int you should: > > return 0 if test_status else 1 Correct, here we are returning the overall exit status for the running benchmarks for CQ to indicate whether the job is successful or not. I modified this accordingly. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:761: def convert_json(option, _, value, parser): On 2015/03/31 21:29:24, qyearsley wrote: > Nit: Should be CamelCase. Could be put inside the body of _OptionParser. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:803: help='build properties in JSON format') On 2015/03/31 21:29:24, qyearsley wrote: > Nit: Alignment Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:860: config={}, path_to_goma=opts.path_to_goma, cq_tryjob=True) On 2015/03/31 22:21:25, qyearsley wrote: > On 2015/03/31 21:29:25, qyearsley wrote: > > How is it determined which benchmark(s) to run? > > Never mind -- > > Upon reading the code more, I can see that it's determined by looking at the > patch (git diff --no-ext-diff --name-only HEAD~1) and then listing benchmarks > that match any modified files with run_benchmark, then running each of those. > Probably no need for more comments here. Done. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:860: config={}, path_to_goma=opts.path_to_goma, cq_tryjob=True) On 2015/03/31 21:29:25, qyearsley wrote: > How is it determined which benchmark(s) to run? Done.
LGTM with a couple more comments/nits. https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/1/tools/run-bisect-perf-regre... tools/run-bisect-perf-regression.py:590: def _GuessTestCommand(bot_platform, test_name=None): On 2015/04/02 21:30:12, prasadv wrote: > On 2015/03/31 21:29:25, qyearsley wrote: > > 1. Possible alternate name: _GuessTelemetryTestCommand > Done > > 2. Does it make sense to have a test command with no test name? > Since we are going to run test for only for changed benchmarks, And to create a > BisectPerformanceMetrics instance we need a command but at this time we don't > know which benchmark to run. Although, it looks like everywhere in this file where this function is called, the test name is always passed; could it theoretically be made a required argument for this function? https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... tools/run-bisect-perf-regression.py:584: return 'release' Extra space https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... tools/run-bisect-perf-regression.py:762: """Provide an OptionParser callback to unmarshal a JSON string.""" Nit: for consistency with other docstrings, could change "Provide" -> "Provides".
Simon, can you please take a look at this for final pass https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... File tools/run-bisect-perf-regression.py (right): https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... tools/run-bisect-perf-regression.py:584: return 'release' On 2015/04/02 21:51:06, qyearsley wrote: > Extra space Done. https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... tools/run-bisect-perf-regression.py:762: """Provide an OptionParser callback to unmarshal a JSON string.""" On 2015/04/02 21:51:05, qyearsley wrote: > Nit: for consistency with other docstrings, could change "Provide" -> > "Provides". Done.
On 2015/04/02 22:17:13, prasadv wrote: > Simon, can you please take a look at this for final pass > > https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... > File tools/run-bisect-perf-regression.py (right): > > https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... > tools/run-bisect-perf-regression.py:584: return 'release' > On 2015/04/02 21:51:06, qyearsley wrote: > > Extra space > > Done. > > https://codereview.chromium.org/1045553003/diff/20001/tools/run-bisect-perf-r... > tools/run-bisect-perf-regression.py:762: """Provide an OptionParser callback to > unmarshal a JSON string.""" > On 2015/04/02 21:51:05, qyearsley wrote: > > Nit: for consistency with other docstrings, could change "Provide" -> > > "Provides". > > Done. Sorry, 4 day weekend :) lgtm I forgot we're migrating to recipes soon, so you're right that refactoring it isn't super critical. I really doubt we'll bolt on any more functionality, but I probably thought that after this script also became the perf-try bots
The CQ bit was checked by prasadv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/1045553003/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1045553003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/155d7e5b71ca9b4e6ea5b86d5edfc503862f1a60 Cr-Commit-Position: refs/heads/master@{#324062} |
