738478 - upgrade mozharness for modern talos

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Jeff Hammel]

http://hg.mozilla.org/build/mozharness/file/93c2b774fd38/scripts/talos_script.py

Talos has changed a lot since this was originally rolled in.  With the fix of bug 700722 bug 650887 is fixed. However, since then talos has real console scripts.  These should be used.  

Much work needs to be done in order to make this script usable for jetperf (https://bugzilla.mozilla.org/show_bug.cgi?id=729205, bug 717036). 
We'll also want a better structure for getting this in production:
https://bugzilla.mozilla.org/show_bug.cgi?id=713055

Comment 1

[Jeff Hammel]

Attachment: suggested fixes

This restructures talos_script.py quite a bit to take advantage of modern talos (console_scripts, not downloading pageloader, etc).

In addition, several changes were made to make the structure of the script consumable to the jetperf script (bug 729205).  I think in general these are positive changes we're going to want anyway

Comment 2

[Jeff Hammel]

We may want to figure out a better more reflective way to deal with CLI options (for Talos and in general) but this is the best I could do for now (manually mirroring)

Comment 3

[Aki Sasaki (not active)]

Comment on attachment 608498 [details] [diff] [review]
suggested fixes

I'm not entirely sure whether this was submitted as WIP or as a finished product; my r- is assuming the latter.  I think it's going in the right direction.

>diff --git a/scripts/talos_script.py b/scripts/talos_script.py
>old mode 100644
>new mode 100755

I approve :)

>+    talos_options = [
>+        [["-a", "--activeTests"],
>+         {"action": "extend",
>+          "dest": "activeTests",

Style nit: I think mozharness options are in the --words-split-by-hyphens style.  Are these camel case to be closer to Talos command line options?

Config options are words_split_by_underscores per PEP8.

>+        [["--branchName"],
>+         {"action": "store",
>+          "dest": "branch_name",
>+          "default": "Mozilla-Central",
>+          "help": "Specify the branch name",
>+          }],

I would love for the help string to be clearer.
This is specifically the graphserver branch name, correct?

>+        [["-t", "--title"],
>         {"action": "store",
>          "dest": "title",
>          "default": None,
>          "help": "talos run title"}],

This too, even though it's not part of this patch ;-)

>                              config={"virtualenv_modules": ["pyyaml", "talos"]},

Also not part of this patch, but if we go with urls-to-tarballs, we'll have to explicitly list all dependencies here.  A pypi server would remove that need, but listing all dependencies is a one-time hit that will probably take less time, should the pypi server take longer than expected or desired.

>+        if self.results_url is None:
>+            self.results_url = 'file://%s' % os.path.join(self.workdir, self.__class__.__name__.lower() + '.yml')

I'd love a comment saying what this expands to.

>+        # path to browser
>+        self.firefox = self.config.get('browser_path')
>+        if not self.firefox:
>+            self.fatal("No path to Firefox specified; please specify --executablePath")
>+        self.firefox = os.path.abspath(self.firefox)
>+        if not os.path.exists(self.firefox):
>+            self.fatal("Path to Firefox does not exist: %s" % self.firefox)

It looks like you're not allowing for download+install of firefox, which is fine for a first pass, but will be required to put into production.
Looks like that might be tracked in bug 720436.

>+        # find PerfConfigurator console script
>+        # TODO: support remotePerfConfigurator if
>+        # https://bugzilla.mozilla.org/show_bug.cgi?id=704654
>+        # is not fixed first
>+        PerfConfigurator = self.query_python_path('PerfConfigurator')
>+        assert os.path.exists(PerfConfigurator), "PerfConfigurator not found"

I tend to avoid asserts in mozharness, opting for descriptive error messages over stacks.  I find them more readable and they may be more non-python-dev friendly.  This one is probably fine, but could be rewritten as

        if not os.path.exists(PerfConfigurator):
            self.fatal("PerfConfigurator not found; have you run with --create-virtualenv?")

Or something like that.

>+        command = [PerfConfigurator] + options
>+
>+        # run PerfConfigurator and ensure conf creation
>+        self.run_command(command, cwd=self.workdir)
>+        assert os.path.exists(talos_conf_path), "PerfConfigurator invokation failed"

I would much prefer an error_list set in the perfconfigurator run_command() call, so we can catch specific problems and interpret if necessary.

This would probably expand on PythonErrorList:
http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/mozharness/base/errors.py#l61

>+        assert os.path.exists(talos), "talos script not found"
>+        command = [talos, '--noisy', talos_conf_path]
>+        self.return_code = self.run_command(command, cwd=self.workdir)

I would love to see an error_list here as well.
We can parse Talos' various output and add explanations if necessary.
Then again, if Talos' error messages all become perfectly intuitive and understandable to people at a glance, that will be less important.

See JarsignerErrorList for some log lines that trigger explanations:
http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/mozharness/base/errors.py#l87

>+        # fatal on failure
>+        if self.return_code:
>+            self.fatal("Failure running talos, exit %d: %s" % (self.return_code, command))

This isn't actually required, since we'll exit with the return code.
However, something like peptest.py's TBPL_{SUCCESS,WARNING,FAILURE} would help when we run in production.
http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/scripts/peptest.py#l324

Comment 4

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #3)
> Comment on attachment 608498 [details] [diff] [review]
> suggested fixes
> 
> I'm not entirely sure whether this was submitted as WIP or as a finished
> product; my r- is assuming the latter.  I think it's going in the right
> direction.

"Finished" is a bit absolute.  How about "commitable to the mozharness hg repo and ready for next steps".

> >diff --git a/scripts/talos_script.py b/scripts/talos_script.py
> >old mode 100644
> >new mode 100755
> 
> I approve :)
> 
> >+    talos_options = [
> >+        [["-a", "--activeTests"],
> >+         {"action": "extend",
> >+          "dest": "activeTests",
> 
> Style nit: I think mozharness options are in the --words-split-by-hyphens
> style.  Are these camel case to be closer to Talos command line options?
> 
> Config options are words_split_by_underscores per PEP8.
(etc)

So concerning the talos_options, the CLI options and dests in http://k0s.org/mozilla/hg/mozharness-patches/file/11101d4aa00f/core#l47 are exactly those of talos (http://hg.mozilla.org/build/talos/file/c6013a2f09ce/talos/PerfConfigurator.py#l366). The help is not preserved.  I used this convention so that talos_options could be mirrored more easily to PerfConfigurator and was intending some cleanup to make this even more reflective.  

A problem is that, in general, we will want to pass the range of PerfConfigurator options, potentially altered and/or restricted, through the talos_script (and also with consideration with scripts that inherit from talos_script.Talos), to PerfConfigurator.  I think it is neither maintainable nor a good investment up front to copy+paste all of PerfConfigurator's options to talos_script.  It will be an extra thing that should be changed every time PerfConfigurator is changed and it will be hard (optimistically) and time-consuming to keep in sync.  I did the 5 options the way that I did because they are the options that I use most often and are needed or they are a subset of what buildbot currently uses and I wanted to get something done that could be built upon.  You can also use --addOption to pass any options not covered by these 5, e.g. `--addOption --filter --addOption ignore_first --addOption --filter --addOption median` to call `PerfConfigurator --filter ignore_first --filter median`.  But this isn't very user friendly

While this problem is encountered for talos_script.py, it is a general problem for mozharness for any script that passes some subset of options to the harness.  For Talos, I can think of two solutions, neither of which you'll probably like:

1. Use a delimiter, --, to delimit mozharness options from subscript options.  e.g. 

talos_script.py --talos_url http://example.com/path -- --activeTests ts -e `which firefox`

In the bare-bones implementation, this could literally just pass the values along.  However, this does not allow us to augment or override this in mozharness.  For the latter, we would have to parse the options ourselves and futz with the values.  Entirely doable.  (FWIW, I believe argparse would let us do this a bit better if we got on python 2.7 [or the horror of horrors, depend on it]) We could also futz with optparse a bit to make this happen.

2. If we had access to PerfConfigurator.py, we could get the options from it.  e.g. 

import talos.PerfConfigurator
talos_options = talos.PerfConfigurator.TalosOptions.get_options()

The API for PerfConfigurator would have to be changed, but that is the least of my worries.

*However*, as it stands this is impossible.  talos_script.py does not have mozharness installed at the time of invocation.  By the time you have PerfConfigurator available for import, you have already passed in the options.  So I'm not sure how we could do this.


In any case, if we're going to block on this I would like a solution that does not involve copy+pasting code or maintaining it two places.  I am fine, if unexcited, about the two solutions I proposed.  I am also fine leaving in the five crappy options for now.

I'll address the other comments subsequently.  This is probably (I hope) the only huge architectural concern so I figured i'd address it individually

Comment 5

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #4)
> A problem is that, in general, we will want to pass the range of
> PerfConfigurator options, potentially altered and/or restricted, through the
> talos_script

How about self.config.get('perfconfigurator_options', []) ?

It's very clunky to pass those in through the mozharness commandline, as you stated.
However, it's very easy to pass in a list through a config file.

e.g., I had virtualenv_options set here:
http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/configs/peptest/win7_config.py#l13

before I found I didn't need it.

Then if we wanted to restrict we could verify nothing in there was bad.

> I think it is neither
> maintainable nor a good investment up front to copy+paste all of
> PerfConfigurator's options to talos_script.

Agreed.

> You can also use
> --addOption to pass any options not covered by these 5, e.g. `--addOption
> --filter --addOption ignore_first --addOption --filter --addOption median`
> to call `PerfConfigurator --filter ignore_first --filter median`.  But this
> isn't very user friendly

--add-options.

Also, it's an "extend", so you can comma-delimit.

--add-options --filter,ignore_first,--filter,median

would work, although I'm not sure if OptParse will treat "--filter,ignore_first,--filter,median" as an argument to --add-options or its own.

Again, we can pass that through the config file.

> 1. Use a delimiter, --, to delimit mozharness options from subscript
> options.  e.g. 
> 
> talos_script.py --talos_url http://example.com/path -- --activeTests ts -e
> `which firefox`
> 
> In the bare-bones implementation, this could literally just pass the values
> along.  However, this does not allow us to augment or override this in
> mozharness.  For the latter, we would have to parse the options ourselves
> and futz with the values.

Yeah, I'm not a fan of making command lines long.
I'm not opposed to enabling the above, but I'm more of a fan of config files and deprecating PerfConfigurator entirely.



> In any case, if we're going to block on this I would like a solution that
> does not involve copy+pasting code or maintaining it two places.  I am fine,
> if unexcited, about the two solutions I proposed.  I am also fine leaving in
> the five crappy options for now.
> 
> I'll address the other comments subsequently.  This is probably (I hope) the
> only huge architectural concern so I figured i'd address it individually

I don't think I was objecting to the options themselves; I was pointing out inconsistent style.  Since you're not actually accessing these programmatically, there doesn't seem to be anything keeping you from changing --activeTests to --active-tests or self.config['activeTests'] to self.config['active_tests'], especially since you immediately set self.tests to self.config['activeTests'] and then refer to self.tests from that point forward.

Comment 6

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #3)
> Comment on attachment 608498 [details] [diff] [review]

> >                              config={"virtualenv_modules": ["pyyaml", "talos"]},
> 
> Also not part of this patch, but if we go with urls-to-tarballs, we'll have
> to explicitly list all dependencies here.  A pypi server would remove that
> need, but listing all dependencies is a one-time hit that will probably take
> less time, should the pypi server take longer than expected or desired.

Is it worth putting up the dependencies at e.g. my people account (or elsewhere)?  If not, maybe the thing to do is to have a .txt requirements file in the directory and a script to generate it
 
> >+        if self.results_url is None:
> >+            self.results_url = 'file://%s' % os.path.join(self.workdir, self.__class__.__name__.lower() + '.yml')
> 
> I'd love a comment saying what this expands to.

http://k0s.org/mozilla/hg/mozharness-patches/rev/6c6382b6ad74

> >+        # path to browser
> >+        self.firefox = self.config.get('browser_path')
> >+        if not self.firefox:
> >+            self.fatal("No path to Firefox specified; please specify --executablePath")
> >+        self.firefox = os.path.abspath(self.firefox)
> >+        if not os.path.exists(self.firefox):
> >+            self.fatal("Path to Firefox does not exist: %s" % self.firefox)
> 
> It looks like you're not allowing for download+install of firefox, which is
> fine for a first pass, but will be required to put into production.
> Looks like that might be tracked in bug 720436.

Yes, we should have a general class (mixin or otherwise) that handles firefox fetching.  I'd be happy to tackle that bug but I think we fundamentally disagree on what should be done.

> >+        # find PerfConfigurator console script
> >+        # TODO: support remotePerfConfigurator if
> >+        # https://bugzilla.mozilla.org/show_bug.cgi?id=704654
> >+        # is not fixed first
> >+        PerfConfigurator = self.query_python_path('PerfConfigurator')
> >+        assert os.path.exists(PerfConfigurator), "PerfConfigurator not found"
> 
> I tend to avoid asserts in mozharness, opting for descriptive error messages
> over stacks.  I find them more readable and they may be more non-python-dev
> friendly.  This one is probably fine, but could be rewritten as
> 
>         if not os.path.exists(PerfConfigurator):
>             self.fatal("PerfConfigurator not found; have you run with
> --create-virtualenv?")
> 
> Or something like that.
> 
> >+        command = [PerfConfigurator] + options
> >+
> >+        # run PerfConfigurator and ensure conf creation
> >+        self.run_command(command, cwd=self.workdir)
> >+        assert os.path.exists(talos_conf_path), "PerfConfigurator invokation failed"
> 
> I would much prefer an error_list set in the perfconfigurator run_command()
> call, so we can catch specific problems and interpret if necessary.
> 
> This would probably expand on PythonErrorList:
> http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/mozharness/base/
> errors.py#l61
> 
> >+        assert os.path.exists(talos), "talos script not found"
> >+        command = [talos, '--noisy', talos_conf_path]
> >+        self.return_code = self.run_command(command, cwd=self.workdir)
> 
> I would love to see an error_list here as well.
> We can parse Talos' various output and add explanations if necessary.
> Then again, if Talos' error messages all become perfectly intuitive and
> understandable to people at a glance, that will be less important.
> 
> See JarsignerErrorList for some log lines that trigger explanations:
> http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/mozharness/base/
> errors.py#l87

I'd be more inclined to make our errors more parseable

> >+        # fatal on failure
> >+        if self.return_code:
> >+            self.fatal("Failure running talos, exit %d: %s" % (self.return_code, command))
> 
> This isn't actually required, since we'll exit with the return code.

Is this the case even if there are more steps?

> However, something like peptest.py's TBPL_{SUCCESS,WARNING,FAILURE} would
> help when we run in production.
> http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/scripts/peptest.
> py#l324

Comment 7

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #5)
> > You can also use
> > --addOption to pass any options not covered by these 5, e.g. `--addOption
> > --filter --addOption ignore_first --addOption --filter --addOption median`
> > to call `PerfConfigurator --filter ignore_first --filter median`.  But this
> > isn't very user friendly
> 
> --add-options.
> 
> Also, it's an "extend", so you can comma-delimit.

True.
 
> --add-options --filter,ignore_first,--filter,median
> 
> would work, although I'm not sure if OptParse will treat
> "--filter,ignore_first,--filter,median" as an argument to --add-options or
> its own.

Casual inspection says its fine:

(jetperf2)│python
Python 2.7.2+ (default, Oct  4 2011, 20:03:08) 
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import optparse
>>> parser = optparse.OptionParser()
>>> parser.add_option('--addOption')
<Option at 0xb703ed0c: --addOption>
>>> options, args = parser.parse_args(['--addOption', '--executable'])
>>> options.addOption
'--executable'

Comment 8

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #6)
> Is it worth putting up the dependencies at e.g. my people account (or
> elsewhere)?  If not, maybe the thing to do is to have a .txt requirements
> file in the directory and a script to generate it

It would be handy for staging purposes.
I don't think we want people.m.o to be a dependency in production.
I'm not entirely sure what you mean by the .txt file, but the above works.

> > It looks like you're not allowing for download+install of firefox, which is
> > fine for a first pass, but will be required to put into production.
> > Looks like that might be tracked in bug 720436.
> 
> Yes, we should have a general class (mixin or otherwise) that handles
> firefox fetching.  I'd be happy to tackle that bug but I think we
> fundamentally disagree on what should be done.

I'm not certain what parts we fundamentally disagree on?
I was expressing my concerns about having scripts guess what behavior to fall back to.  That might be ok when run by a human, but when run by automation across thousands of nodes you want to know precisely what behavior to expect.

> > I would love to see an error_list here as well.
> > We can parse Talos' various output and add explanations if necessary.
> > Then again, if Talos' error messages all become perfectly intuitive and
> > understandable to people at a glance, that will be less important.
> > 
> > See JarsignerErrorList for some log lines that trigger explanations:
> > http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/mozharness/base/
> > errors.py#l87
> 
> I'd be more inclined to make our errors more parseable

Ok.
We should still have an error_list, even if we don't add explanations, so we can mark the appropriate lines as errors/warnings/etc.
 
> > >+        # fatal on failure
> > >+        if self.return_code:
> > >+            self.fatal("Failure running talos, exit %d: %s" % (self.return_code, command))
> > 
> > This isn't actually required, since we'll exit with the return code.
> 
> Is this the case even if there are more steps?

That depends.
If you never want to proceed to the following steps after a talos error, then sure.

Comment 9

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #8)
> Ok.
> We should still have an error_list, even if we don't add explanations, so we
> can mark the appropriate lines as errors/warnings/etc.

Looks like I started a talos error list here:

https://github.com/escapewindow/mozharness/blob/78e18d91da668a3e247d4675860c4df157e085e0/scripts/device_talosrunner.py#L328

The regexes need to be changed to re.compile()s but otherwise I think that would be a decent start.

Comment 10

[Jeff Hammel]

If the main division is concerning the options help parameter, maybe an expedient compromise is e.g. 

['-a', '--activeTests',]
{'help': '--activeTests option for PerfConfigurator'}

Comment 11

[Jeff Hammel]

> Aki Sasaki [:aki] 2012-03-23 16:09:35 PDT
>
> (In reply to Jeff Hammel [:jhammel] from comment #4)
> > A problem is that, in general, we will want to pass the range of
> > PerfConfigurator options, potentially altered and/or restricted, through the
> > talos_script
>
> How about self.config.get('perfconfigurator_options', []) ?

See below.  Since it is your program, I am willing to do this if it is
the only thing we can agree to, but I strongly object, for ease of my
use as well as the ease of other consumers and also because I believe
it to be the wrong decision architecturally.

> It's very clunky to pass those in through the mozharness
  commandline, as you stated.
> However, it's very easy to pass in a list through a config file.

I would agree if we were talking about 10-20 options.  We're talking
more like five or six with --addOptions. talos_script.py has some
twenty options OOTB from mozharness alone.

> e.g., I had virtualenv_options set here:
> http://hg.mozilla.org/build/mozharness/file/5f44ba08f4be/configs/peptest/win7_config.py#l13
>
> before I found I didn't need it.
>
> Then if we wanted to restrict we could verify nothing in there was bad.

How would we do this?  Reparse the options?  That seems overly complex
and unnecessary.

> > I think it is neither
> > maintainable nor a good investment up front to copy+paste all of
> > PerfConfigurator's options to talos_script.
>
> Agreed.
>
> > You can also use
> > --addOption to pass any options not covered by these 5, e.g. `--addOption
> > --filter --addOption ignore_first --addOption --filter --addOption median`
> > to call `PerfConfigurator --filter ignore_first --filter median`.  But this
> > isn't very user friendly
>
> --add-options.
>
> Also, it's an "extend", so you can comma-delimit.
>
> --add-options --filter,ignore_first,--filter,median
>
> would work, although I'm not sure if OptParse will treat "--filter,ignore_first,--filter,median" as an argument to --add-options or its own.
>
> Again, we can pass that through the config file.
>
> > 1. Use a delimiter, --, to delimit mozharness options from subscript
> > options.  e.g.
> >
> > talos_script.py --talos_url http://example.com/path -- --activeTests ts -e
> > `which firefox`
> >
> > In the bare-bones implementation, this could literally just pass the values
> > along.  However, this does not allow us to augment or override this in
> > mozharness.  For the latter, we would have to parse the options ourselves
> > and futz with the values.
>
> Yeah, I'm not a fan of making command lines long.
> I'm not opposed to enabling the above, but I'm more of a fan of config files 
and deprecating PerfConfigurator entirely.

I also want to deprecate PerfConfigurator entirely and have been
extending some effort, in my free time, towards doing so.
https://bugzilla.mozilla.org/show_bug.cgi?id=704654 exists for this
case.  While, in abstract, restructuring how config is dealt with is
pretty straight-forward, there are a number of "Talosisms" that make
this harder than it probably should be.

The current talos workflow is:

0. Install Talos (see https://wiki.mozilla.org/Buildbot/Talos#Running_locally_-_Source_Code)
1. Run `PerfConfigurator` with appropriate options
2. Run `talos` with the output .yml file (and probably '-n -d'; not
   sure why these aren't defaults.)

A typical run for me, assuming talos is installed is:

PerfConfigurator -a tsvg -e `which firefox` --develop -o tsvg.yml --results_url file://${PWD}/tsvg.txt
talos -n -d tsvg.yml
Really, for this case, the tsvg.yml is extraneous since I have no
need/want to keep it around.

The desired workflow (bug 704654) is obviates step 1. so that the
above example looks like this:

talos -a tsvg -e `which firefox` --develop --results_url file://${PWD}/tsvg.txt

If we did want to keep the resultant configure around,
https://bugzilla.mozilla.org/show_bug.cgi?id=719861 exists for adding
a --dump option.

One advantage of having a programmatic configuration is that the
configuration may be verified against what is allowed.  For example,
`mozharness.base.config` does not verify that any of the values input
from an (e.g.) .json configuration file actually correspond to what is
desired by the code.  Likewise, mozharness lacks an ability to
serialize configuration.

As I understand it, the triple primary goals of mozhaness are (in no
particular order):

1. To make testing more unified (and therefor maintainable) in
production (and, consequently, to eliminate a lot of difficult
buildbot-related code).

2. To make testing easier for developers.

3. To give developers a way of running tests similar to how they are
done in production.

While I agree that trying to do everything via the command line fails
compared to configuration files, as would be the case for complicated
runs, I am of the school that the simple use-case -- just running the
tests -- should be doable via the command line.  I find it much
more convenient, for the simple cases, to be able to invoke a program
from the command line versus editing a config file.  I realize this is
a philosophical difference between our approaches.  I would guess most
developers would want to run talos via command line switches and as
few of them as possible.  Ideally, this should be possible and should
serve most of the develop-facing use-cases:

talos -a tsvg -e `which firefox` -o tsvg.txt
Config files are useful for (IMHO) A. complicated setup;
B. reproducibility; C. being able to share run time configurations
The latter is only useful if the configuration contains nothing
machine-specific (e.g. the path to an executable might vary from
machine to machine) or if the configuration is overridable from the
command line.  For simple setup -- just running e.g. the ts tests for
talos -- I greatly prefer the command line.  This may be a
philosophical difference between us.  In addition, without a method of
serializing mozharness config, the only way of getting the required
parameters is reading source code.

As I understand it, though I haven't been at Mozilla for the entire
time that Talos existed, originally there was a single file,
sample.config.  Users edited this file, saved as something different,
and then ran `python run_tests.py <new file>`.  With the proliferation
of options and various tests to be run, at some point this was found
cumbersome. So someone wrote PerfConfigurator.py, a poor excuse for a
serializer but, like most software developer-facing software, it was
written to save time in the short term, and, it probably did.
Fast-forwarding, having 6 .config files makes maintaining this system
time-consuming and error prone (I'd say maybe 25% of my time on Talos
bugs is spent dealing with the faults of PerfConfigurator --
primarily, its lack of canonical configuration and its poor excuse for
serialization).

I'd like to avoid repeating these mistakes and in general having sane
and extensible configuration that is easy to read, easy to use, and
easy to serialize/deserialize (and easy to combine).

PerfConfigurator is used in production to generate .yml
files, in contrast to using production .yml files directly. I would
guess this is for reasons implied above: it is difficult to manage and
keep in sync a large number of configurations.

Using --perfconfigurator-options, the above command becomes

./talos_script --perfconfigurator-options -a,tsvg,-e,`which firefox`,--develop,--results_url,file://${PWD}/tsvg.txt

I wouldn't consider this easier to run than the current talos.  If the
complaint rests agains the poor help from porting the PerfConfigurator
options to mozharness, 'perfconfigurator_options' doesn't provide any
more help there and relies on the programmer having a checkout of
talos to see what the options are.  In addition, if the options are
going to be programmatically manipulated or inspected, the options
will have to be reparsed (see
http://k0s.org/mozilla/hg/mozharness-patches/file/6c6382b6ad74/core#l160
and also the fact that we may wish to add other options in mozharness
by default).

> > In any case, if we're going to block on this I would like a solution that
> > does not involve copy+pasting code or maintaining it two places.  I am fine,
> > if unexcited, about the two solutions I proposed.  I am also fine leaving in
> > the five crappy options for now.
> >
> > I'll address the other comments subsequently.  This is probably (I hope) the
> > only huge architectural concern so I figured i'd address it individually

> I don't think I was objecting to the options themselves; I was
> pointing out inconsistent style.  Since you're not actually
> accessing these programmatically, there doesn't seem to be anything
> keeping you from changing --activeTests to --active-tests or
> self.config['activeTests'] to self.config['active_tests'],
> especially since you immediately set self.tests to
> self.config['activeTests'] and then refer to self.tests from that
> point forward.

While in this patch I do not currently use the options
programmatically, the jetperf follow up that depends on this
(https://bugzilla.mozilla.org/show_bug.cgi?id=729205) does make use of
setting the defaults of --activeTests as, in the use case the Jetpack
team currently cares about, we want to measure using the 'ts' test:

http://k0s.org/mozilla/hg/mozharness-patches/file/6c6382b6ad74/jetperf#l92

(Aside: this is, IMHO, already less than ideal; it would be much
easier to index as e.g.
self.config_options['activeTests']['default'] = ['ts'])

In addition, I would prefer to make PerfConfiguration_options
(http://k0s.org/mozilla/hg/mozharness-patches/file/6c6382b6ad74/core#l190)
more programmatic.

Comment 12

[Jeff Hammel]

see also https://bugzilla.mozilla.org/show_bug.cgi?id=704654#c3

Comment 13

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #8)
> (In reply to Jeff Hammel [:jhammel] from comment #6)
> > Is it worth putting up the dependencies at e.g. my people account (or
> > elsewhere)?  If not, maybe the thing to do is to have a .txt requirements
> > file in the directory and a script to generate it
> 
> It would be handy for staging purposes.
> I don't think we want people.m.o to be a dependency in production.
> I'm not entirely sure what you mean by the .txt file, but the above works.

So I'm not sure what the action item is here.  Should i put up a pypi on people for the time being? Should we figure this out in more detail? Should we block on having a dedicated pypi (that isn't my people account)?

> > > It looks like you're not allowing for download+install of firefox, which is
> > > fine for a first pass, but will be required to put into production.
> > > Looks like that might be tracked in bug 720436.
> > 
> > Yes, we should have a general class (mixin or otherwise) that handles
> > firefox fetching.  I'd be happy to tackle that bug but I think we
> > fundamentally disagree on what should be done.
> 
> I'm not certain what parts we fundamentally disagree on?
> I was expressing my concerns about having scripts guess what behavior to
> fall back to.  That might be ok when run by a human, but when run by
> automation across thousands of nodes you want to know precisely what
> behavior to expect.

See https://bugzilla.mozilla.org/show_bug.cgi?id=720436#c3

Comment 14

[Aki Sasaki (not active)]

Attachment: interdiff against http://k0s.org/mozilla/hg/mozharness-patches/file/6c6382b6ad74/core

This will add output parsing to the talos run_command().

Comment 15

[Jeff Hammel]

Comment on attachment 610394 [details] [diff] [review]
interdiff against http://k0s.org/mozilla/hg/mozharness-patches/file/6c6382b6ad74/core

Thanks, aki!

http://k0s.org/mozilla/hg/mozharness-patches/rev/2b30557e9d54

Comment 16

[Jeff Hammel]

So cleaning up the talos config options a bit, I have

        [["-a", "--tests"],
         {'action': 'extend',
          "dest": "tests",
          "default": [],
          "help": "Specify the tests to run"
          }],
        [["-e", "--binary"],
         {"action": "store",
          "dest": "firefox",
          "default": None,
          "help": "Path to the Firefox binary to run tests on",
          }],
        [["--results-url"],
         {'action': 'store',
          'dest': 'results_url',
          'default': None,
          'help': "URL to send results to"
          }],
        ]

I've eliminated --branchName since its not user-facing (or needed) and --title for the same reason.  These can be added with --add-option

:aki, is this acceptable to you?

Comment 17

[Aki Sasaki (not active)]

I think that's ok, with a question: will we ever want to run Talos on non-Firefox products?  The answer was yes until relatively recently, at least: Fennec was considered a separate product.  If SeaMonkey, B2G, Camino, or any other product uses or will use Talos, we may want to consider rewording the --binary dest and help.

Comment 18

[Jeff Hammel]

Just remove the 'Firefox' ?

Comment 19

[Jeff Hammel]

:aki, how about this?

        [["-a", "--tests"],
         {'action': 'extend',
          "dest": "tests",
          "default": [],
          "help": "Specify the tests to run"
          }],
        [["-e", "--binary"],
         {"action": "store",
          "dest": "binary",
          "default": None,
          "help": "Path to the binary to run tests on",
          }],
        [["--results-url"],
         {'action': 'store',
          'dest': 'results_url',
          'default': None,
          'help': "URL to send results to"
          }],

Comment 20

[Jeff Hammel]

I put up a "duck-typed" pypi for all of talos' dependencies at http://people.mozilla.com/~jhammel/pypi/.  Using `easy_install talos` for this URL shows that these dependencies are hit internally:

(tmp)│easy_install -i http://people.mozilla.com/~jhammel/pypi/ talos
Searching for talos
Reading http://people.mozilla.com/~jhammel/pypi/talos/
Reading http://people.mozilla.com/~jhammel/pypi/talos/?C=N;O=D
Reading http://people.mozilla.com/~jhammel/pypi/talos/?C=D;O=A
Reading http://people.mozilla.com/~jhammel/pypi/talos/?C=M;O=A
Reading http://people.mozilla.com/~jhammel/pypi/talos/?C=S;O=A
Best match: talos 0.0
Downloading http://people.mozilla.com/~jhammel/pypi/talos/talos-0.0.tar.gz
Processing talos-0.0.tar.gz
Running talos-0.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-fcibEo/talos-0.0/egg-dist-tmp-dtmcBf
Adding talos 0.0 to easy-install.pth file
Installing PerfConfigurator script to /home/jhammel/tmp/bin
Installing talos script to /home/jhammel/tmp/bin
Installing remotePerfConfigurator script to /home/jhammel/tmp/bin

Installed /home/jhammel/tmp/lib/python2.7/site-packages/talos-0.0-py2.7.egg
Processing dependencies for talos
Searching for mozinfo
Reading http://people.mozilla.com/~jhammel/pypi/mozinfo/
Reading http://people.mozilla.com/~jhammel/pypi/mozinfo/?C=S;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozinfo/?C=N;O=D
Reading http://people.mozilla.com/~jhammel/pypi/mozinfo/?C=D;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozinfo/?C=M;O=A
Best match: mozinfo 0.3.3
Downloading http://people.mozilla.com/~jhammel/pypi/mozinfo/mozinfo-0.3.3.tar.gz
Processing mozinfo-0.3.3.tar.gz
Running mozinfo-0.3.3/setup.py -q bdist_egg --dist-dir /tmp/easy_install-CxqmL_/mozinfo-0.3.3/egg-dist-tmp-DU7BXW
Adding mozinfo 0.3.3 to easy-install.pth file
Installing mozinfo script to /home/jhammel/tmp/bin

Installed /home/jhammel/tmp/lib/python2.7/site-packages/mozinfo-0.3.3-py2.7.egg
Searching for mozhttpd>=0.3
Reading http://people.mozilla.com/~jhammel/pypi/mozhttpd/
Reading http://people.mozilla.com/~jhammel/pypi/mozhttpd/?C=D;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozhttpd/?C=M;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozhttpd/?C=S;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozhttpd/?C=N;O=D
Best match: mozhttpd 0.3
Downloading http://people.mozilla.com/~jhammel/pypi/mozhttpd/mozhttpd-0.3.tar.gz
Processing mozhttpd-0.3.tar.gz
Running mozhttpd-0.3/setup.py -q bdist_egg --dist-dir /tmp/easy_install-hwD5Pg/mozhttpd-0.3/egg-dist-tmp-qv8wEG
Adding mozhttpd 0.3 to easy-install.pth file
Installing mozhttpd script to /home/jhammel/tmp/bin

Installed /home/jhammel/tmp/lib/python2.7/site-packages/mozhttpd-0.3-py2.7.egg
Searching for mozdevice>=0.2
Reading http://people.mozilla.com/~jhammel/pypi/mozdevice/
Reading http://people.mozilla.com/~jhammel/pypi/mozdevice/?C=S;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozdevice/?C=N;O=D
Reading http://people.mozilla.com/~jhammel/pypi/mozdevice/?C=D;O=A
Reading http://people.mozilla.com/~jhammel/pypi/mozdevice/?C=M;O=A
Best match: mozdevice 0.2
Downloading http://people.mozilla.com/~jhammel/pypi/mozdevice/mozdevice-0.2.tar.gz
Processing mozdevice-0.2.tar.gz
Running mozdevice-0.2/setup.py -q bdist_egg --dist-dir /tmp/easy_install-HX8vJO/mozdevice-0.2/egg-dist-tmp-7Tl6li
Adding mozdevice 0.2 to easy-install.pth file

Installed /home/jhammel/tmp/lib/python2.7/site-packages/mozdevice-0.2-py2.7.egg
Searching for PyYAML
Reading http://people.mozilla.com/~jhammel/pypi/PyYAML/
Reading http://people.mozilla.com/~jhammel/pypi/PyYAML/?C=D;O=A
Reading http://people.mozilla.com/~jhammel/pypi/PyYAML/?C=M;O=A
Reading http://people.mozilla.com/~jhammel/pypi/PyYAML/?C=N;O=D
Reading http://people.mozilla.com/~jhammel/pypi/PyYAML/?C=S;O=A
Best match: PyYAML 3.10
Downloading http://people.mozilla.com/~jhammel/pypi/PyYAML/PyYAML-3.10.tar.gz
Processing PyYAML-3.10.tar.gz
Running PyYAML-3.10/setup.py -q bdist_egg --dist-dir /tmp/easy_install-uvxO9_/PyYAML-3.10/egg-dist-tmp-qtDK3I
build/temp.linux-x86_64-2.7/check_libyaml.c:2:18: fatal error: yaml.h: No such file or directory
compilation terminated.

libyaml is not found or a compiler error: forcing --without-libyaml
(if libyaml is installed correctly, you may need to
 specify the option --include-dirs or uncomment and
 modify the parameter include_dirs in setup.cfg)
zip_safe flag not set; analyzing archive contents...
Adding PyYAML 3.10 to easy-install.pth file

Installed /home/jhammel/tmp/lib/python2.7/site-packages/PyYAML-3.10-py2.7-linux-x86_64.egg
Finished processing dependencies for talos

I can put this on pypi.ateam.phx1.mozilla.com if this is helpful for talking to staging systems.  I am unfamiliar if this is accessible to the build network, etc.  It is currently not accessible from the outside world.

Comment 21

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #19)
> :aki, how about this?

Sure. I'm not entirely sure about the 'binary' help but we can adjust later if we think of something.

Comment 22

[Jeff Hammel]

Attachment: WIP

Firefox fetching needs to be standardized (https://bugzilla.mozilla.org/show_bug.cgi?id=720436) and we need to be able to point to a pypi (bug 740147) and the script should be moved to mozharness.mozilla.talos, but i think this addresses the other fixes.

When moving to mozharness.mozilla.talos, should we create a console script for this?

Comment 23

[Jeff Hammel]

Comment on attachment 611847 [details] [diff] [review]
WIP

Sorry, i meant f? but at this point we've mostly talked about the issues.  I'll put up a new patch shortly

Comment 24

[Jeff Hammel]

Attachment: update talos_script.py -> mozharness.mozilla.talos

I hope I got all the nits here

Comment 25

[Aki Sasaki (not active)]

Comment on attachment 612268 [details] [diff] [review]
update talos_script.py -> mozharness.mozilla.talos

We already know this'll change over time to support desktop talos, device talos, and jetperf.  I think this works as a first pass.  Thanks Jeff!

Comment 26

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/mozharness/rev/c5249a886c6d

Yeah, follow up is likely.  I'm not even 100% sure that I won't have to change things for bug 729205, but its a lot closer

Comment 27

[Jeff Hammel]

bah, my commit did not move the file :( Sorry about that, will push in a follow up

Comment 28

[Jeff Hammel]

done http://hg.mozilla.org/build/mozharness/rev/e76d4405f7ee