720773 - Talos should mark undefined command line options as undefined

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

bug 716921 allows for run_tests.py to accept the same arguments from
PerfConfigurator's option parser
(http://hg.mozilla.org/build/talos/rev/757fb43d5eba). So the workflow
is

1. generate a .yml file from PerfConfigurator.py
2. invoke run_test.py with overrides

However, we check to see if the options we get are overrides by
checking to see if they are defaults:

http://hg.mozilla.org/build/talos/file/ca816866e975/talos/run_tests.py#l440

This means that

- if you generate a .yml file with non-default options
AND
- you attempt to override these to their defaults with run_test.py
arguments

the overrides won't take place.  Instead, we need to somehow note if
we have actually parsed options.

https://bug716921.bugzilla.mozilla.org/attachment.cgi?id=590346 gives
an example of how to do this.  However, see https://bugzilla.mozilla.org/show_bug.cgi?id=716921#c16

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Mark parsed args as parsed for override of existing config in run_tests

Marks options as Parsed if they are found in command line args. Similar overall pattern to https://bug716921.bugzilla.mozilla.org/attachment.cgi?id=590346 except parsed options are marked as Parsed, rather than undefined options marked as Undefined.

Comment 2

[Jeff Hammel]

Comment on attachment 592414 [details] [diff] [review]
Mark parsed args as parsed for override of existing config in run_tests

This isn't going to work for a couple of reasons:

+            if args[0] in sys.argv[1:] or args[1] in sys.argv[1:]:
...
+            if args[0] in sys.argv[1:]:

We should not check for sys.argv[1:] here.  While OptionParser and therefore TalosOptions default to the use of sys.argv[1:], parse_args takes args as a parameter:

http://hg.mozilla.org/build/talos/file/9c1b3addb9ee/talos/PerfConfigurator.py#l504

You cannot be sure if this is sys.argv[1:] or other in the general API sense.

Also, while you test if args[0] and args[1] in sys.argv[1:], args can be arbitrary:

parser.add_option('-f', '--foo', '--long-foo', ...)

So this isn't right.

I'm not sure what this is supposed to do, either.  Why check for this?  I don't think that add_option is the correct place to check for it.  I think you will have much better luck working in TalosOptions.parse_args.  You can set the defaults there to something (Parsed), parse the arguments (via OptionParser) then set them back.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: Mark parsed args as parsed in parse_args() for override of existing config in run_tests

Thanks Jeff, I think this is closer - marking options as parsed in the parse_args() method. This doesn't mark 'store_true' args in the same way, so I left the check in run_tests for whether an option is a default.

Comment 4

[Jeff Hammel]

Comment on attachment 593160 [details] [diff] [review]
Mark parsed args as parsed in parse_args() for override of existing config in run_tests

So ABICT this will note the args passed in as parsed but not the options.  We should operate on the options level

Comment 5

[Jeff Hammel]

Attachment: WIP

SO this doesn't quite work but is the basic idea

Comment 6

[Jeff Hammel]

Attachment: proposed fix

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 595159 [details] [diff] [review]
proposed fix

Review of attachment 595159 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Thanks, Jeff, for taking a look at this.

Comment 8

[Mozilla RelEng Bot]

Try run for 5d6b63ec29cb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d6b63ec29cb
Results (out of 78 total builds):
    exception: 1
    success: 68
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d6b63ec29cb
 Timed out after 06 hours without completing.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

from the try server run:
Traceback (most recent call last):
  File "PerfConfigurator.py", line 592, in <module>
    sys.exit(main())
  File "PerfConfigurator.py", line 570, in main
    options, args = parser.parse_args(argv)
  File "PerfConfigurator.py", line 514, in parse_args
    options, args = optparse.OptionParser.parse_args(self, args=args, values=values)
  File "/home/cltbld/lib/python2.5/optparse.py", line 1387, in parse_args
    stop = self._process_args(largs, rargs, values)
  File "/home/cltbld/lib/python2.5/optparse.py", line 1427, in _process_args
    self._process_long_opt(rargs, values)
  File "/home/cltbld/lib/python2.5/optparse.py", line 1502, in _process_long_opt
    option.process(opt, value, values, self)
  File "/home/cltbld/lib/python2.5/optparse.py", line 774, in process
    self.action, self.dest, opt, value, values, parser)
  File "/home/cltbld/lib/python2.5/optparse.py", line 786, in take_action
    values.ensure_value(dest, []).append(value)
AttributeError: 'Undefined' object has no attribute 'append'

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 595159 [details] [diff] [review]
proposed fix

will wait for updated patch.

Comment 11

[Jeff Hammel]

The line that causes the error:

python PerfConfigurator.py -v -e `which firefox` --setPref hangmonitor.timeout=0

lists with action=append apparently don't work properly

Comment 12

[Jeff Hammel]

Attachment: use Option subclass to mark parsed

This is probably a cleaner solution regardless

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 595439 [details] [diff] [review]
use Option subclass to mark parsed

Review of attachment 595439 [details] [diff] [review]:
-----------------------------------------------------------------

ok, this is looking great.

::: talos/PerfConfigurator.py
@@ +310,5 @@
>  
>  class TalosOptions(optparse.OptionParser):
> +    """Parses Talos commandline options."""
> +
> +    class TalosOption(optparse.Option):

TalosOption is a sublcass of TalosOptions, my stomach is turning.

Comment 14

[Jeff Hammel]

> TalosOption is a sublcass of TalosOptions, my stomach is turning.

TalosOptions should probably be TalosOptionsParser but...

Comment 15

[Jeff Hammel]

try is pretty green: https://tbpl.mozilla.org/?tree=Try&rev=a143fdb10342

tested ts and tsvg locally without issue.  pushing

Comment 16

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/0f26ab632b09

Also probably worthwhile keeping this pattern in mind for other option parsers.  In general it is something to consider when accepting inputs from both the command line and other sources

Comment 17

[Mozilla RelEng Bot]

Try run for a143fdb10342 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a143fdb10342
Results (out of 73 total builds):
    success: 72
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a143fdb10342