733514 - filters should be configurable on a per-test basis

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

currently Talos filters live at the top-level of configuration.
Instead, the master filters should live in the `basetest:` section and
each test should be able to define its own set of filters separately

Comment 1

[Jeff Hammel]

As with anything dealing with configuration, it would be helpful if ours wasn't such a mess before implementing this.

In addition, we should come up with general behaviour for options defined in basetest that may be overridden by command line options

Comment 2

[Jeff Hammel]

So probably the hard part of solving this will be figuring out what to do about per test variables and the command line.  We are currently inconsistent on this.

--tpmanifest overrides the manifest for *each* test specified on the command line *if* tpmanifest is in a line in the test section.  If it is not, we do nothing. 

--noChrome, --tpcycles, --tpmozafterpaint, --tppagecycles, maybe more *add* these keys to each test section and thereby assume that they are not there to begin with 

In addition, once you have a yml file and run with:

  run_tests.py myyamlfile.yml --noChrome --tpmanifest mymanifest --tpcycles 5

All of these options are *ignored* and the ones from myyamlfile.yml are used without complaining.

Again, we really need a better configuration system here.

Comment 3

[Jeff Hammel]

I've been thinking about this.  How should this work in general?  That is, if you could specify filters on a per-test basis, are we talking about these just living in .config/yml files with a master in basetest?  And --filter overrides both?

Illustrious examples welcome!

Comment 4

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

oh why do you ask for specifics...that is a challenge ya know.

In general talos runs a single test at a time anyway.  The one exception is a few test suites that bundle ts and pageloader tests.  This exceptional bundling is the exact scenario where we need unique filters.

I think basetest should have a filter.  If we have a baseTS and a baseTP, then we can provide a default filter for them.  

I am thinking we could have:
--filterTS [[ignore: 1], [average]]
--filterTP [[ignore: 5], [median]]

I know this could turn into an ugly mess, but TS and TP are two different beasts.  What happens in the future when we have more types?  I think we add into it.  I don't see us having 20 types.

Ideally this could live in the .config files as such:
basetest:
filter_ts: ...
filter_tp: ...

testname:
filter_ts: .^.
fitler_tp: ^..

and of course cli args would overwrite any specific filters for the entire talos run_tests.py pass.

Comment 5

[Jeff Hammel]

I think this is overall the right approach

Maybe the thing to do is to have three basetest sections:

* basetest: things all tests inherit from
* baseTs: things that Ts tests inherit from 
* baseTp: things that Tp tests inherit from

(This fits in nicely with the structure in test.py).

So information will be fill out as follows, for e.g. a Ts test:

* if specified in the test, use that
* if not, use the value specified in baseTs
* if not, use the value specified in basetest

I'm thinking of something like

basetest:
  timeout: 3600
  ts:
     cycles: 1
     timeout: 1800

tests:
- name: ts
  timeout: 900
- name: ts_paint
  cycles: 2
- name: tp
  tpmanifest: some/file

In this case, ts_paint will get {'cycles': 2, 'timeout': 1800, ...}, ts will get {'cycles': 1, timeout: 900, ...}, and tp will get {'timeout': 3600, ...}

Its a bit complex, but I think it makes sense.

I also don't see introducing more than ts or tp for any time in the reasonable future.  If anything, I could see ts becoming a tp test

Comment 6

[Jeff Hammel]

So without implementing the whole infrastructure, I am looking at this bug and have a few comments up front that are mostly out of scope but since we've already diverged here a bit hopefully we can outline things a bit better.

- Looking through the entireity of the talos configuration code, I notice we have one more level of indirection than we actually should.  Namely, we have top level options, http://hg.mozilla.org/build/talos/file/d00eba6055f5/talos/PerfConfigurator.py#l197 + basetest and now we're talking about having a level even below that.  We should probably have a better story here outside of the scope of this bug.

- Looking at the code, we send results *after* all the tests are done: http://hg.mozilla.org/build/talos/file/d00eba6055f5/talos/run_tests.py#l580 . This makes restructuring more invasive than it would need to be otherwise.  I'll poke around and figure out what to do here.

Comment 7

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #4)
> oh why do you ask for specifics...that is a challenge ya know.
> 
> In general talos runs a single test at a time anyway.  The one exception is
> a few test suites that bundle ts and pageloader tests.  This exceptional
> bundling is the exact scenario where we need unique filters.
> 
> I think basetest should have a filter.  If we have a baseTS and a baseTP,
> then we can provide a default filter for them.  
> 
> I am thinking we could have:
> --filterTS [[ignore: 1], [average]]
> --filterTP [[ignore: 5], [median]]
> 
> I know this could turn into an ugly mess, but TS and TP are two different
> beasts.  What happens in the future when we have more types?  I think we add
> into it.  I don't see us having 20 types.

Currently, we only use filters on tp tests:

http://hg.mozilla.org/build/talos/file/d00eba6055f5/talos/run_tests.py#l184

Comment 8

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

well, lets start there:)

I would like things to be in test.py if possible.  The goal is not to change the buildbot code that runs tests to achieve parity.  My current use case is dromaeo which does an internal mean calculation for data points and historically has produced only 1 value.  We want to break that out, but I need to ignore nothing and produce a mean of all the data points.

Comment 9

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

inside of talos, we could have the 'filters' stored in the test variable.  Then we could pass that test object around and reference the filters from the test itself.  If we move the filters into basetest, that might do the job.

Comment 10

[Jeff Hammel]

Yeah, I think I will do that for pass one.  There's a lot of code that I'd like to restructure but I think I'll do the simple thing now and ignore the walls caving in around me.

Comment 11

[Jeff Hammel]

Attachment: allow filters to be configurable on a per-test basis

Doing this patch made me realize how bad our results handling is.  However, I have mostly ignored that and just wrote a patch.  I'll file follow-up bugs on how to make this better.

The test is a bit of a monster.  Because we have to stub the world, it is very long and it is also slow as it times out on raw results upload.  These problems should be taken care of by a results refactoring.

Comment 12

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

Comment on attachment 624161 [details] [diff] [review]
allow filters to be configurable on a per-test basis

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

r=me if you can help me understand my confusion on the below comments.

::: talos/filter.py
@@ +146,5 @@
> +    filter_names = [f[0] for f in _filters]
> +    filter_functions = filters(*filter_names)
> +    for index, value in enumerate(_filters):
> +        retval.append([filter_functions[index], value[-1]])
> +    return retval

not really clear on this.

::: talos/run_tests.py
@@ +426,5 @@
> +  filters = config['filters']
> +  try:
> +      filters = filter.filters_args(filters)
> +  except AssertionError, e:
> +      raise talosError(str(e))

is this a top level config?

@@ +454,5 @@
> +        filter.filters_args(test['filters'])
> +      except AssertionError, e:
> +        raise talosError(str(e))
> +      except IndexError, e:
> +        raise talosError(str(e))

this doesn't set filters, just validates them?

Comment 13

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #12)
> Comment on attachment 624161 [details] [diff] [review]
> allow filters to be configurable on a per-test basis
> 
> Review of attachment 624161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you can help me understand my confusion on the below comments.
> 
> ::: talos/filter.py
> @@ +146,5 @@
> > +    filter_names = [f[0] for f in _filters]
> > +    filter_functions = filters(*filter_names)
> > +    for index, value in enumerate(_filters):
> > +        retval.append([filter_functions[index], value[-1]])
> > +    return retval
> 
> not really clear on this.

Yeah, its mostly awful :(  We should probably have a real class for Filter and/or FilterChain but currently filters are a list of 2-"tuples" (actually 2-item lists).  In these lists, the first item is the filter and the second item is a list of arguments to the filter (besides the data).  This helper function transforms:

[["mean", []]

into the function mean() and keeps the arguments the same.  Since you can't have callables in config, the name of the function is the lookup key.

Does that make sense?  Really, filters and/or the filter chain should be objects.


> ::: talos/run_tests.py
> @@ +426,5 @@
> > +  filters = config['filters']
> > +  try:
> > +      filters = filter.filters_args(filters)
> > +  except AssertionError, e:
> > +      raise talosError(str(e))
> 
> is this a top level config?

Yes.  That has not changed.

> @@ +454,5 @@
> > +        filter.filters_args(test['filters'])
> > +      except AssertionError, e:
> > +        raise talosError(str(e))
> > +      except IndexError, e:
> > +        raise talosError(str(e))
> 
> this doesn't set filters, just validates them?

filter.filter_args() does return the filter chain needed.  However, we don't use it anywhere.

Comment 14

[Jeff Hammel]

see https://bugzilla.mozilla.org/show_bug.cgi?id=755527 for ideas about how results should be made better

Comment 15

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/f2aec5d0a58c

This is a fairly quick and dirty solution and hopefully bug 755527 can make this -- and other things -- a bit better