1191796 - remove the filters arguments, and rework filters definition

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

Bug 1170287 comment 7 is quite informative to know what should be removed in term of options.

This bug is specifically for the filters arguments, as I think we can rework the filter system in the same time.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: rework_filters.patch

tell me what you think of it. :)

Comment 2

[Julien Pagès (:parkouss)]

this is working fine locally.

Comment 3

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

Comment on attachment 8644317 [details] [diff] [review]
rework_filters.patch

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

a few small things, but overall I really like this- a lot of cleanup.

::: talos/filter.py
@@ +17,5 @@
> +
> +
> +class Filters(tuple):
> +    def apply(self, data):
> +        for filter in self:

filter in self?  that is odd syntax

::: talos/run_tests.py
@@ +105,5 @@
>  def run_tests(config, browser_config):
>      """Runs the talos tests on the given configuration and generates a report.
>      """
>      # data filters
> +    filters = filter.ignore_max.prepare() + filter.median.prepare()

could we default to ignore_first(1)

Comment 4

[Julien Pagès (:parkouss)]

Attachment: rework_filters.patch

I updated the patch, according to what you said.

Also I added some comments, and refactored a bit more so every test config now have filters - and I changed the default filters as you mentionned (will that change something on results ?)

Asking for another review so you can check all this.

Seems to work fine locally, and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f443746c575

Comment 5

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

Comment on attachment 8644373 [details] [diff] [review]
rework_filters.patch

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

I would like to make the default ignore_first(x) + median(), and then only show the differences as custom filter definitions.  Ideally we can adjust cycles to get us to the same x, probably 5, but we might scale back to 3 or something.

::: talos/filter.py
@@ +8,5 @@
> +`prepare` method that create a tuple with one instance of a
> +:class:`Filter`; this allow to write stuff like::
> +
> +  from talos import filter
> +  filters = filter.ignore_first(1) + filter.mean.prepare()

median should be default, not mean.

Comment 6

[Julien Pagès (:parkouss)]

Attachment: rework_filters.patch

Thanks Joel!

I was a bit slow to understand what you meant - but it's ok  now and this is working fine on try. :) So this is patch you reviewed previously, merged with the patch I sent on irc.

I retried erroneous jobs on try, now this is all good. We can wait for the try to finish, I am confident now that every next job will be green.

Comment 7

[Julien Pagès (:parkouss)]

Landed:

https://hg.mozilla.org/build/talos/rev/0eabb0e18ab2