Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
616063004:
Add --output-dir flag for page_runner.py. (Closed)
DescriptionAdd --output-dir flag for page_runner.py to specify a custom directory for
output profiling and results data.
Motivation behind this flag: I've run into a bug where a run on a local device
and a run on the try bots are showing different versions of the page. I wanted
to use the android_screen_recorder_profiler.py to record the try bot version
and see what it is actually showing. However, it currently tries to output the
video to a temporary directory in /tmp (which I do not have permission to write
to).
BUG=
Committed: https://crrev.com/2a859c8f48b40d7bc443812e6cf760f10e0d22ba
Cr-Commit-Position: refs/heads/master@{#297740}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use more generic --output-dir #
Total comments: 4
Messages
Total messages: 27 (4 generated)
ariblue@google.com changed reviewers: + nednguyen@google.com, tonyg@chromium.org
On 2014/09/30 22:06:18, ariblue wrote: Can you wrap your description? It sucks that we have to do it manually.
lgtm
https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', default=None, We try to be thoughtful about the command line options we add in order to keep it minimal and easy to use. I'm wondering wether we should consider something more generic that is used for all run artifacts.
https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', default=None, On 2014/09/30 22:18:39, tonyg wrote: > We try to be thoughtful about the command line options we add in order to keep > it minimal and easy to use. I'm wondering wether we should consider something > more generic that is used for all run artifacts. How about we use the same output directory as results. Eventually, saving & uploading these artifacts to cloud should be handled by the results system too.
Patchset #2 (id:20001) has been deleted
On 2014/09/30 22:23:03, nednguyen wrote: > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/browser_options.py (right): > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', > default=None, > On 2014/09/30 22:18:39, tonyg wrote: > > We try to be thoughtful about the command line options we add in order to keep > > it minimal and easy to use. I'm wondering wether we should consider something > > more generic that is used for all run artifacts. > > How about we use the same output directory as results. Eventually, saving & > uploading these artifacts to cloud should be handled by the results system too. I've added a more generic --output-dir flag that works for both results and profiling output. How do we want to handle --trace-dir (which is both an output directory and a boolean as to whether or not the tracing is enabled)?
On 2014/09/30 22:51:47, ariblue wrote: > On 2014/09/30 22:23:03, nednguyen wrote: > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > File tools/telemetry/telemetry/core/browser_options.py (right): > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', > > default=None, > > On 2014/09/30 22:18:39, tonyg wrote: > > > We try to be thoughtful about the command line options we add in order to > keep > > > it minimal and easy to use. I'm wondering wether we should consider > something > > > more generic that is used for all run artifacts. > > > > How about we use the same output directory as results. Eventually, saving & > > uploading these artifacts to cloud should be handled by the results system > too. > > I've added a more generic --output-dir flag that works for both results and > profiling output. How do we want to handle --trace-dir (which is both an output > directory and a boolean as to whether or not the tracing is enabled)? Cool, I like that better. Is --trace-dir=/foo a synonym for --profiler=trace --output-dir=/foo ?
On 2014/09/30 22:54:35, tonyg wrote: > On 2014/09/30 22:51:47, ariblue wrote: > > On 2014/09/30 22:23:03, nednguyen wrote: > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > File tools/telemetry/telemetry/core/browser_options.py (right): > > > > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', > > > default=None, > > > On 2014/09/30 22:18:39, tonyg wrote: > > > > We try to be thoughtful about the command line options we add in order to > > keep > > > > it minimal and easy to use. I'm wondering wether we should consider > > something > > > > more generic that is used for all run artifacts. > > > > > > How about we use the same output directory as results. Eventually, saving & > > > uploading these artifacts to cloud should be handled by the results system > > too. > > > > I've added a more generic --output-dir flag that works for both results and > > profiling output. How do we want to handle --trace-dir (which is both an > output > > directory and a boolean as to whether or not the tracing is enabled)? > > Cool, I like that better. > > Is --trace-dir=/foo a synonym for --profiler=trace --output-dir=/foo ? Not entirely. Looks like different paths to the same backends. --trace-dir is used with timeline_based_measurement.py for outputting the trace that was already generated for use with metrics. So: web_perf/timeline_Based_measurement.py:177: tab.browser.platform.tracing_controller.Start(...) platform/tracing_controller.py:27: self._tracing_controller_backend.Start(...) platform/tracing_controller_backend.py:44: browser_backend.StartTracing(...) Setting --profiler=trace calls self._browser_backend.StartTracing(...) directly.
On 2014/10/01 00:01:52, ariblue wrote: > On 2014/09/30 22:54:35, tonyg wrote: > > On 2014/09/30 22:51:47, ariblue wrote: > > > On 2014/09/30 22:23:03, nednguyen wrote: > > > > > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > > File tools/telemetry/telemetry/core/browser_options.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > > tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', > > > > default=None, > > > > On 2014/09/30 22:18:39, tonyg wrote: > > > > > We try to be thoughtful about the command line options we add in order > to > > > keep > > > > > it minimal and easy to use. I'm wondering wether we should consider > > > something > > > > > more generic that is used for all run artifacts. > > > > > > > > How about we use the same output directory as results. Eventually, saving > & > > > > uploading these artifacts to cloud should be handled by the results system > > > too. > > > > > > I've added a more generic --output-dir flag that works for both results and > > > profiling output. How do we want to handle --trace-dir (which is both an > > output > > > directory and a boolean as to whether or not the tracing is enabled)? > > > > Cool, I like that better. > > > > Is --trace-dir=/foo a synonym for --profiler=trace --output-dir=/foo ? > > Not entirely. Looks like different paths to the same backends. > > --trace-dir is used with timeline_based_measurement.py for outputting the trace > that was already generated for use with metrics. So: > web_perf/timeline_Based_measurement.py:177: > tab.browser.platform.tracing_controller.Start(...) > platform/tracing_controller.py:27: self._tracing_controller_backend.Start(...) > platform/tracing_controller_backend.py:44: browser_backend.StartTracing(...) > > Setting --profiler=trace calls self._browser_backend.StartTracing(...) directly. TimelineBasedMeasurement's --trace-dir is a hack. We will kill it sooner or later.
https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:129: else: This is awkward since results.html will no longer be in the same directory as results.html. If we expect all of the artifacts of the run are produced in a same directory, let's make it consistent. https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/results/results_options.py (right): https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/results/results_options.py:37: help='Where to save output data after the run.') It looks like output_file takes precedence over output_dir if both are specified? @Tony: I wonder whether we can kill the --output flag.
On 2014/10/01 04:37:17, nednguyen wrote: > On 2014/10/01 00:01:52, ariblue wrote: > > On 2014/09/30 22:54:35, tonyg wrote: > > > On 2014/09/30 22:51:47, ariblue wrote: > > > > On 2014/09/30 22:23:03, nednguyen wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > > > File tools/telemetry/telemetry/core/browser_options.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/616063004/diff/1/tools/telemetry/telemetry/co... > > > > > tools/telemetry/telemetry/core/browser_options.py:109: '--profiler-dir', > > > > > default=None, > > > > > On 2014/09/30 22:18:39, tonyg wrote: > > > > > > We try to be thoughtful about the command line options we add in order > > to > > > > keep > > > > > > it minimal and easy to use. I'm wondering wether we should consider > > > > something > > > > > > more generic that is used for all run artifacts. > > > > > > > > > > How about we use the same output directory as results. Eventually, > saving > > & > > > > > uploading these artifacts to cloud should be handled by the results > system > > > > too. > > > > > > > > I've added a more generic --output-dir flag that works for both results > and > > > > profiling output. How do we want to handle --trace-dir (which is both an > > > output > > > > directory and a boolean as to whether or not the tracing is enabled)? > > > > > > Cool, I like that better. > > > > > > Is --trace-dir=/foo a synonym for --profiler=trace --output-dir=/foo ? > > > > Not entirely. Looks like different paths to the same backends. > > > > --trace-dir is used with timeline_based_measurement.py for outputting the > trace > > that was already generated for use with metrics. So: > > web_perf/timeline_Based_measurement.py:177: > > tab.browser.platform.tracing_controller.Start(...) > > platform/tracing_controller.py:27: > self._tracing_controller_backend.Start(...) > > platform/tracing_controller_backend.py:44: browser_backend.StartTracing(...) > > > > Setting --profiler=trace calls self._browser_backend.StartTracing(...) > directly. > > TimelineBasedMeasurement's --trace-dir is a hack. We will kill it sooner or > later. Then I won't worry about it for now.
https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:129: else: On 2014/10/01 04:37:36, nednguyen wrote: > This is awkward since results.html will no longer be in the same directory as > results.html. If we expect all of the artifacts of the run are produced in a > same directory, let's make it consistent. I just wanted to keep the default behavior the same as it was for now, as to not break anyone's workflow. Is this CL good as is, and then we can revisit what we want the default behavior for files to be in a follow-up? https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/results/results_options.py (right): https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/results/results_options.py:37: help='Where to save output data after the run.') On 2014/10/01 04:37:36, nednguyen wrote: > It looks like output_file takes precedence over output_dir if both are > specified? > > @Tony: I wonder whether we can kill the --output flag. Nope. You can't specify both. Throws a parser error below (line 63).
On 2014/10/01 17:21:09, ariblue wrote: > https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/page/page_runner.py (right): > > https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/page/page_runner.py:129: else: > On 2014/10/01 04:37:36, nednguyen wrote: > > This is awkward since results.html will no longer be in the same directory as > > results.html. If we expect all of the artifacts of the run are produced in a > > same directory, let's make it consistent. > > I just wanted to keep the default behavior the same as it was for now, as to not > break anyone's workflow. Is this CL good as is, and then we can revisit what we > want the default behavior for files to be in a follow-up? > > https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/results/results_options.py (right): > > https://codereview.chromium.org/616063004/diff/40001/tools/telemetry/telemetr... > tools/telemetry/telemetry/results/results_options.py:37: help='Where to save > output data after the run.') > On 2014/10/01 04:37:36, nednguyen wrote: > > It looks like output_file takes precedence over output_dir if both are > > specified? > > > > @Tony: I wonder whether we can kill the --output flag. > > Nope. You can't specify both. Throws a parser error below (line 63). lgtm
tonyg@chromium.org changed reviewers: + dtu@chromium.org
Something still doesn't feel intuitive here. Maybe Dave'll have some ideas. With this patch, would results.html go into --output-dir if specified? If --output and --output-dir are mutually exclusive and --output-dir dictates where all artifacts are saved, then this might be reasonable.
On 2014/10/01 19:44:44, tonyg wrote: > Something still doesn't feel intuitive here. Maybe Dave'll have some ideas. > > With this patch, would results.html go into --output-dir if specified? If > --output and --output-dir are mutually exclusive and --output-dir dictates where > all artifacts are saved, then this might be reasonable. I would say results output files are also artifacts. Ideally, I want results.htm to be saved in directory specified by --output-dir.
On 2014/10/01 19:44:44, tonyg wrote: > Something still doesn't feel intuitive here. Maybe Dave'll have some ideas. > > With this patch, would results.html go into --output-dir if specified? If > --output and --output-dir are mutually exclusive and --output-dir dictates where > all artifacts are saved, then this might be reasonable. That's exactly right. --output -> specifies the full path of the file where results are output --output-dir -> specifies what directory to output all results and profiling data --output and --output-dir -> parser error.
lgtm
The CQ bit was checked by ariblue@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616063004/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as d6523d657610b525b8086fae01538a09aeddc33e
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2a859c8f48b40d7bc443812e6cf760f10e0d22ba Cr-Commit-Position: refs/heads/master@{#297740}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:40001) has been created in https://codereview.chromium.org/616953005/ by dtu@chromium.org. The reason for reverting is: Failing on all perf bots in generate_telemetry_profiles: Traceback (most recent call last): <module> at tools/perf/generate_profile:13 sys.exit(profile_generator.Main()) Main at tools/telemetry/telemetry/page/profile_generator.py:145 AddCommandLineArgs(parser) AddCommandLineArgs at tools/telemetry/telemetry/page/profile_generator.py:115 help='Generated profile is placed in this directory.') add_option at /usr/lib/python2.7/optparse.py:1020 self._check_conflict(option) _check_conflict at /usr/lib/python2.7/optparse.py:995 option) OptionConflictError: option --output-dir: conflicting option string(s): --output-dir Locals: co : ('--output-dir', <Option at 0x2b074d0: --output-dir>) conflict_opts : [('--output-dir', <Option at 0x2b074d0: --output-dir>)] handler : 'error' opt : '--output-dir' option : <Option at 0x2b08290: --output-dir> .
Message was sent while issue was closed.
On 2014/10/02 03:34:32, dtu wrote: > A revert of this CL (patchset #2 id:40001) has been created in > https://codereview.chromium.org/616953005/ by mailto:dtu@chromium.org. > > The reason for reverting is: Failing on all perf bots in > generate_telemetry_profiles: > > Traceback (most recent call last): > <module> at tools/perf/generate_profile:13 > sys.exit(profile_generator.Main()) > Main at tools/telemetry/telemetry/page/profile_generator.py:145 > AddCommandLineArgs(parser) > AddCommandLineArgs at tools/telemetry/telemetry/page/profile_generator.py:115 > help='Generated profile is placed in this directory.') > add_option at /usr/lib/python2.7/optparse.py:1020 > self._check_conflict(option) > _check_conflict at /usr/lib/python2.7/optparse.py:995 > option) > OptionConflictError: option --output-dir: conflicting option string(s): > --output-dir > > Locals: > co : ('--output-dir', <Option at 0x2b074d0: --output-dir>) > conflict_opts : [('--output-dir', <Option at 0x2b074d0: --output-dir>)] > handler : 'error' > opt : '--output-dir' > option : <Option at 0x2b08290: --output-dir> > . This is again showing that each flags bring a lot of technical debt for us. Let's go with: 1) Solve chrishenry's TODO, i.e: make --output-dir flag instead of --output-file 2) Place all the artifacts in output_dir What do you guys think?
Message was sent while issue was closed.
On 2014/10/02 15:12:15, nednguyen wrote: > On 2014/10/02 03:34:32, dtu wrote: > > A revert of this CL (patchset #2 id:40001) has been created in > > https://codereview.chromium.org/616953005/ by mailto:dtu@chromium.org. > > > > The reason for reverting is: Failing on all perf bots in > > generate_telemetry_profiles: > > > > Traceback (most recent call last): > > <module> at tools/perf/generate_profile:13 > > sys.exit(profile_generator.Main()) > > Main at tools/telemetry/telemetry/page/profile_generator.py:145 > > AddCommandLineArgs(parser) > > AddCommandLineArgs at > tools/telemetry/telemetry/page/profile_generator.py:115 > > help='Generated profile is placed in this directory.') > > add_option at /usr/lib/python2.7/optparse.py:1020 > > self._check_conflict(option) > > _check_conflict at /usr/lib/python2.7/optparse.py:995 > > option) > > OptionConflictError: option --output-dir: conflicting option string(s): > > --output-dir > > > > Locals: > > co : ('--output-dir', <Option at 0x2b074d0: --output-dir>) > > conflict_opts : [('--output-dir', <Option at 0x2b074d0: --output-dir>)] > > handler : 'error' > > opt : '--output-dir' > > option : <Option at 0x2b08290: --output-dir> > > . > > This is again showing that each flags bring a lot of technical debt for us. > Let's go with: > 1) Solve chrishenry's TODO, i.e: make --output-dir flag instead of --output-file > 2) Place all the artifacts in output_dir > > What do you guys think? Love it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
