telemetry: Allow page tests to be configured via constructor

telemetry: Allow page tests to be configured via constructor

Some parameters of page tests can be hard coded for a particular benchmark
and don't need to be exposed as command line arguments. This patch
allows configuring those parameters through the constructor of the
page test.

R=tonyg@chromium.org,nednguyen@google.com,nduca@chromium.org,chrishenry@google.com,dtu@chromium.org,vmiura@chromium.org
BUG=421276

[ernstm, 2014-10-17 00:49:04]

This is an alternate way to achieve the goal for which
https://codereview.chromium.org/637153002/ was created. The idea is to configure
parameters through the constructor if they can be hard coded for a benchmark.
The handling of real command line arguments doesn't change. Given that the
removal of command line arguments from page tests is controversial, I propose to
use this solution instead. If we want to change how command line arguments are
handled in telemetry, than we should discuss a more general solution separately.

[nednguyen, 2014-10-17 02:59:46]

https://codereview.chromium.org/641823004/diff/1/tools/telemetry/telemetry/be...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/641823004/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:185: Override to generate a custom page
test.
Can you add some comment explaining why this has to be @classmethod.

https://codereview.chromium.org/641823004/diff/1/tools/telemetry/telemetry/be...
tools/telemetry/telemetry/benchmark.py:190: raise TypeError('"%s" is not a
PageTest.' % cls.test.__name__)
It's surprising to page_test's writer that this will be called twice throughout
telemetry benchmark. What if the __init__ of PageTest creates some side effect?

If you insist in this direction, either make the creation of page_test earlier
in benchmark_runner or either cache it with a class attribute of benchmark.

[chrishenry, 2014-10-17 23:54:29]

Personally, I slightly prefer the other patch due to its simplicity, cleaner API
(not requiring @classmethod for CreatePageTest). There is also a potential to
simplify the user experience, in that they don't need to use command-line flags
to tune the benchmark behavior (although we aren't realizing that potential as
we simply move the flags from measurement to benchmark).

In conclusion, I'm fine with this - we should still do some accounting on what
each measurements are adding to command-line flags and whether they're really
needed or not. If the only devs who need to tweak the command-line args are the
benchmark writers, we should make them constants in benchmark; if only
measurement writers need it, they should be constants in measurement.

(I wonder whether there is a middle ground, such as asking benchmark writer to
override AddTestCommandLineFlags and call AddCommandLineFlags on the measurement
if needed (so the command-line flags can still be centralized if necessary).
This also adds API complexity but only for those measurements that want to add
their own parameters.)

[ernstm, 2014-10-20 22:51:53]

On 2014/10/17 23:54:29, chrishenry wrote:
> Personally, I slightly prefer the other patch due to its simplicity, cleaner
API
> (not requiring @classmethod for CreatePageTest). There is also a potential to
> simplify the user experience, in that they don't need to use command-line
flags
> to tune the benchmark behavior (although we aren't realizing that potential as
> we simply move the flags from measurement to benchmark).
> 
> In conclusion, I'm fine with this - we should still do some accounting on what
> each measurements are adding to command-line flags and whether they're really
> needed or not. If the only devs who need to tweak the command-line args are
the
> benchmark writers, we should make them constants in benchmark; if only
> measurement writers need it, they should be constants in measurement.
> 
> (I wonder whether there is a middle ground, such as asking benchmark writer to
> override AddTestCommandLineFlags and call AddCommandLineFlags on the
measurement
> if needed (so the command-line flags can still be centralized if necessary).
> This also adds API complexity but only for those measurements that want to add
> their own parameters.)

The latest patch gets rid of the classmethod. Instead, it instantiates the page
test in the constructor of the benchmark and offers an instance method to
configure command line options. This allows us to use the constructor to
configure all parameters that can be hard coded in he benchmark. This should be
the preferred method. If a parameter really needs to be exposed on the command
line, then it can be done on the page test as before, with all the benefits that
vmiura@ pointed out. I would like to move forward with this approach, if
everybody agrees.

[vmiura, 2014-10-20 23:31:18]

Seems like a good step to me, since this gives the mechanism for replacing
command options, while keeping it where needed e.g. TimelineBasedMeasurement
"--trace-dir" which doesn't fit well as a Benchmark coded option.

[ernstm, 2014-10-21 18:55:41]

On 2014/10/20 23:31:18, vmiura wrote:
> Seems like a good step to me, since this gives the mechanism for replacing
> command options, while keeping it where needed e.g. TimelineBasedMeasurement
> "--trace-dir" which doesn't fit well as a Benchmark coded option.

This patch doesn't work, because options can be passed on the command line as
"--option-name <value>", and the patch would consider <value> to be an argument.
The real problem here is that we need to manually parse the command line to
figure out which benchmark we want to run and then add it's command line
arguments. How about we simplify benchmark_runner, by:
- moving the deps command into a separate run_deps script
- remove support for "run_benchmark.py run <benchmark>" and only support
"run_benchmark <benchmark>"
- require <benchmark> to always be the first argument, i.e. "run_benchmark
<benchmark> <options>"

This would allow us to look at the first argument, instantiate the right
benchmark, configure the command line, parse it, and then run the benchmark.

[nednguyen, 2014-10-21 19:38:32]

On 2014/10/21 18:55:41, ernstm wrote:
> On 2014/10/20 23:31:18, vmiura wrote:
> > Seems like a good step to me, since this gives the mechanism for replacing
> > command options, while keeping it where needed e.g. TimelineBasedMeasurement
> > "--trace-dir" which doesn't fit well as a Benchmark coded option.
> 
> This patch doesn't work, because options can be passed on the command line as
> "--option-name <value>", and the patch would consider <value> to be an
argument.
> The real problem here is that we need to manually parse the command line to
> figure out which benchmark we want to run and then add it's command line
> arguments. How about we simplify benchmark_runner, by:
> - moving the deps command into a separate run_deps script
> - remove support for "run_benchmark.py run <benchmark>" and only support
> "run_benchmark <benchmark>"
> - require <benchmark> to always be the first argument, i.e. "run_benchmark
> <benchmark> <options>"
> 
> This would allow us to look at the first argument, instantiate the right
> benchmark, configure the command line, parse it, and then run the benchmark.

I think forcing the benchmark name to be the specified before any options is the
right model anyway. 

However, we still need to distinguish between "run_benchmark run" and
"run_benchmark list". How about we deprecate "run_benchmark <benchmark>" and
only support "run_benchmark run <benchmark>"?

[ernstm, 2014-10-21 22:24:36]

On 2014/10/21 19:38:32, nednguyen wrote:
> On 2014/10/21 18:55:41, ernstm wrote:
> > On 2014/10/20 23:31:18, vmiura wrote:
> > > Seems like a good step to me, since this gives the mechanism for replacing
> > > command options, while keeping it where needed e.g.
TimelineBasedMeasurement
> > > "--trace-dir" which doesn't fit well as a Benchmark coded option.
> > 
> > This patch doesn't work, because options can be passed on the command line
as
> > "--option-name <value>", and the patch would consider <value> to be an
> argument.
> > The real problem here is that we need to manually parse the command line to
> > figure out which benchmark we want to run and then add it's command line
> > arguments. How about we simplify benchmark_runner, by:
> > - moving the deps command into a separate run_deps script
> > - remove support for "run_benchmark.py run <benchmark>" and only support
> > "run_benchmark <benchmark>"
> > - require <benchmark> to always be the first argument, i.e. "run_benchmark
> > <benchmark> <options>"
> > 
> > This would allow us to look at the first argument, instantiate the right
> > benchmark, configure the command line, parse it, and then run the benchmark.
> 
> I think forcing the benchmark name to be the specified before any options is
the
> right model anyway. 
> 
> However, we still need to distinguish between "run_benchmark run" and
> "run_benchmark list". How about we deprecate "run_benchmark <benchmark>" and
> only support "run_benchmark run <benchmark>"?

"run_benchmark run <benchmark>" is clunky. We could just split it up and have
separate commands run_benchmark, list_benchmarks, and run_deps instead.

[nednguyen, 2014-10-22 03:07:27]

On 2014/10/21 22:24:36, ernstm wrote:
> On 2014/10/21 19:38:32, nednguyen wrote:
> > On 2014/10/21 18:55:41, ernstm wrote:
> > > On 2014/10/20 23:31:18, vmiura wrote:
> > > > Seems like a good step to me, since this gives the mechanism for
replacing
> > > > command options, while keeping it where needed e.g.
> TimelineBasedMeasurement
> > > > "--trace-dir" which doesn't fit well as a Benchmark coded option.
> > > 
> > > This patch doesn't work, because options can be passed on the command line
> as
> > > "--option-name <value>", and the patch would consider <value> to be an
> > argument.
> > > The real problem here is that we need to manually parse the command line
to
> > > figure out which benchmark we want to run and then add it's command line
> > > arguments. How about we simplify benchmark_runner, by:
> > > - moving the deps command into a separate run_deps script
> > > - remove support for "run_benchmark.py run <benchmark>" and only support
> > > "run_benchmark <benchmark>"
> > > - require <benchmark> to always be the first argument, i.e. "run_benchmark
> > > <benchmark> <options>"
> > > 
> > > This would allow us to look at the first argument, instantiate the right
> > > benchmark, configure the command line, parse it, and then run the
benchmark.
> > 
> > I think forcing the benchmark name to be the specified before any options is
> the
> > right model anyway. 
> > 
> > However, we still need to distinguish between "run_benchmark run" and
> > "run_benchmark list". How about we deprecate "run_benchmark <benchmark>" and
> > only support "run_benchmark run <benchmark>"?
> 
> "run_benchmark run <benchmark>" is clunky. We could just split it up and have
> separate commands run_benchmark, list_benchmarks, and run_deps instead.

This sounds fine to me.

[ernstm, 2014-11-19 18:02:25]

Closing. We decided to move forward with
https://codereview.chromium.org/637153002/ instead.