1339604 - stylo builds + tests should only run on m-c and stylo branch to reduce budget impact

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: savemoney.patch

Comment 2

[Kim Moir [:kmoir] ET]

My understanding is that we don't have budget for additional AWS builds for quantum thus we must limit the branches where we run builds and tests.  This patch limits stylo builds to running on m-c and the stylo repo. Please confirm cpeterson :-)

Comment 3

[Bobby Holley (:bholley)]

Sorry, this isn't going to be acceptable. See bug 1339396 comment 9.

Also, note that the stylo repo is going away. With the new repo sync work, we are working directly on autoland.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> Sorry, this isn't going to be acceptable. See bug 1339396 comment 9.
> 
> Also, note that the stylo repo is going away. With the new repo sync work,
> we are working directly on autoland.

(See [1] and [2] for a description of our new workflow)

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/acolCIYh0RI
[2] https://groups.google.com/forum/#!topic/mozilla.dev.servo/ZbWd0-eDwRA

Comment 5

[Bobby Holley (:bholley)]

Also, just to be clear: The Talos jobs are much less important right now, and are fine to run on central only.

Comment 6

[Chris Peterson [:cpeterson]]

Bobby, to reduce Stylo test load, could we turn off Stylo opt or debug tests on inbound? Since Servo and Stylo commits are all pushed through the autoland repo, tests on autoland seem like they are most likely to identify Stylo regressions.

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: savemoney2.patch

patch to enable builds on autoland as well

Comment 8

[Bobby Holley (:bholley)]

(In reply to Chris Peterson [:cpeterson] from comment #6)
> Bobby, to reduce Stylo test load, could we turn off Stylo opt or debug tests
> on inbound? Since Servo and Stylo commits are all pushed through the
> autoland repo, tests on autoland seem like they are most likely to identify
> Stylo regressions.

I agree that we could probably reduce the test matrix a bit on inbound.

The easiest target is probably the reftests, where we have 16 jobs (R1-R16) running 4 ways (debug/opt and e10s/non-e10s). The overhead of that dominates everything else, so if we're going to slice it somehow that would be the place to start. I would be ok with turning off 3 of those 4 configurations on inbound, and sticking with only Reftest-debug-e10s, which should reduce the job count on inbound by almost a factor of four.

We should leave the builds, crashtests and mochitest-style running in all the configurations, since they give us more best bang for buck.

Comment 9

[Kim Moir [:kmoir] ET]

Attachment: bug1339604.patch

not sure if this patch is what we want.

I couldn't find a way to specify that we only want to run reftest-stylo e10s debug on one branch, but the rest of the tests on others

Comment 10

[Kim Moir [:kmoir] ET]

Hmm, this didn't seem to work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c48f645ac917abc3017bcba00a522aefc9627499&selectedJob=77720143

Not sure how to specify that debug stylo-reftests for e10s should only run on m-i but all stylo reftests should run on other branches. Will ask in channel.

Comment 11

[Kim Moir [:kmoir] ET]

Dustin, looking at the current transforms I see that we can specify e10s on a per platform basis but I'm not sure how to toggle that on a per platform basis for multiple branches

I'm looking to run 
debug stylo-reftests for e10s should only run on m-i but all stylo reftests should run on other branches

hmm, maybe I need to land different patches on different branches?

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Yikes, I feel like we're getting endlessly creative with stylo solutions, and the clarity of our configuration is suffering accordingly.  I think there's an argument to make comparing cognitive load of implementing and understanding these solutions to the cost of just executing more stylo-related work.  Anyway, just throwing that out there.

It should be possible to make `e10s:` keyed by both test-platform and project.  Then you could write something like:

    e10s:
        by-test-platform:
            linux64-stylo/opt: true
            linux64-stylo/debug:
                # only run e10s on inbound
                by-platform:
                    mozilla-inbound: true
                    default: false
            default: both

(I'm not sure I got the logic right there, but hopefully the idea of nested by-* is clear from the example)

Note that `project` is not an attribute of a test description, so you will need to add `config.params['project']` to `handle_keyed_by` in taskcluster/taskgraph/transforms/tests.py`:

    for test in tests:
        for field in fields:
            resolve_keyed_by(test, field, item_name=test['test-name'], project=config.params['project'])
        yield test 

and also modify the schema for `e10s` at the top of that file to allow keying by project, too:

    Required('e10s', default='both'): optionally_keyed_by(
        'test-platform', 'project',
        Any(bool, 'both')),

Comment 13

[Kim Moir [:kmoir] ET]

Hmm, I tried this today but it didn't work so I'm debugging.  Dustin why do you refer to by-platform for the changes to taskcluster/ci/test/tests.yml

but the taskcluster/taskgraph/transforms/tests.py refers to project?

Comment 14

[Dustin J. Mitchell [:dustin] (he/him)]

I typed the wrong thing, sorry. In my defense, they both start with "p" :)

                # only run e10s on inbound
                by-project:
                    mozilla-inbound: true
                    default: false

Comment 15

[Kim Moir [:kmoir] ET]

Attachment: bug1339604lr.patch

Here is my most recent patch which is not working

The problem is that the schema expects a bool value for 

   "In test {!r}:".format(test['test-name']))
  File "/Users/kmoir/hg/mozilla-inbound/taskcluster/taskgraph/util/schema.py", line 25, in validate_schema
    raise Exception('\n'.join(msg) + '\n' + pprint.pformat(obj))
Exception: In test 'reftest-stylo':
expected bool for dictionary value @ data['e10s']['by-test-platform']['linux64-stylo/debug']
expected bool for dictionary value @ data['e10s']['by-test-platform']['linux64-stylo/opt']

So the I think the schema here is wrong

Required('e10s', default='both'): optionally_keyed_by(
        'test-platform', 'project',
        Any(bool, 'both')),

but I don't understand how it should be changed.  It seems like project should be nested under test-platform, not at the same level in the schema

Comment 16

[Dustin J. Mitchell [:dustin] (he/him)]

Hm, I didn't implement optionally_keyed_by the way I thought I did.  Let me see if I can fix that..

Comment 17

[Kim Moir [:kmoir] ET]

Attachment: savemoney3.patch

Aside from getting the test transforms sorted, we should disable the builds on anything other than these branches to save money in the interim.  I noticed the stylo builds are running on some project branches that have no need for them.

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: Bug 1339604: Allow nesting of keyed-by values;

Review commit: https://reviewboard.mozilla.org/r/113206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113206/

Comment 19

[Kim Moir [:kmoir] ET]

Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114768

Wow, that is quite a patch. Thank you so much!

So I tried this with the patches I have in the bug.  The error ocurred

 File "/Users/kmoir/hg/mozilla-inbound/taskcluster/taskgraph/util/schema.py", line 138, in resolve_keyed_by
    keyed_by, key, field, item_name))
Exception: No project matching 'try' nor 'default' found while determining item e10s in reftest-stylo

This is because I have this in the tests.yml for stylo
e10s:
    by-test-platform:
        linux64-stylo/opt:
            by-project:
                mozilla-inbound: false
 
 If I switch mozilla-inbound to try it works.  So I think there needs to some additional values added to the defaults.

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

(In reply to Kim Moir [:kmoir] from comment #19)
> e10s:
>     by-test-platform:
>         linux64-stylo/opt:
>             by-project:
>                 mozilla-inbound: false

I think that last bit is the issue: if the platform is linux64-stylo/opt, and project is anything but mozilla-inbound, then it doesn't say what e10s should be set to.  If you add a default value, it should work:

                  default: both   # or 'true' or whatever you would like to take effect for other projects

Comment 21

[Kim Moir [:kmoir] ET]

Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114918

Ah, that fixes it then.  Thanks Dustin.

Comment 22

[Kim Moir [:kmoir] ET]

Attachment: bug1339604lr2.patch

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8838577 [details] [diff] [review]
bug1339604lr2.patch

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

::: taskcluster/ci/test/tests.yml
@@ +941,5 @@
> +            linux64-stylo/opt:
> +                by-project:
> +                    mozilla-inbound: false
> +                    default: both
> +            linux64-stylo/debug: 

trailing space

Comment 24

[Kim Moir [:kmoir] ET]

Comment on attachment 8838279 [details]
Bug 1339604: Allow nesting of keyed-by values;

https://reviewboard.mozilla.org/r/113206/#review114986

Comment 25

[Kim Moir [:kmoir] ET]

Attachment: bug1339604lr3.patch

fixed whitespace

Comment 26

[Bobby Holley (:bholley)]

Thanks for working on this!

To make sure I understand correctly: This will also enable the reftest-stylo job on autoland, right? We need to turn that on as soon as possible so that we can prevent regressions (more keep popping up), so please land your patch directly to the autoland repo if you're able to. :-)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbcbdd68493
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin

Comment 28

[Kim Moir [:kmoir] ET]

Could I have this patch 

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fbcbdd68493

cherry picked and landed on autoland?

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95fa240ac387
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin

Comment 30

[Kim Moir [:kmoir] ET]

Attachment: bug1339604moretests.patch

patch to fix the issue where reftests don't run on m-c etc. See 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1041546e9d721ec47f4ccd1c530f0ce892888b8d&selectedJob=78414573

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 8838710 [details] [diff] [review]
bug1339604moretests.patch

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

lgtm, though note that the reftest jobs are already running on try, so the try push doesn't necessarily prove anything. Might want somebody who understand the automation stuff to give it a look-over.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe83738221a
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=bholley

Comment 33

[Kim Moir [:kmoir] ET]

Attachment: bug1339604m-i.patch

noticed that stylo opt reftests are still running on inbound, and only need Reftest-debug-e10s there according to previous comments

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f8aecb0f44f
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=kmoir DONTBUILD

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/02a96520e63c
stylo builds + tests should only run on limited branches to reduce budget impact r=dustin a=merge
https://hg.mozilla.org/mozilla-central/rev/0930fdc4cf8e
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=bholley a=merge

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/d11c29c1db3a
stylo builds + tests should only run on m-c and stylo branch to reduce budget impact r=kmoir a=merge

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02a96520e63c
https://hg.mozilla.org/mozilla-central/rev/0930fdc4cf8e

Comment 38

[Kim Moir [:kmoir] ET]

Verified on treeherder that this looks good now.  only e10s debug stylo reftests on m-i, both e10s and non-e10s stylo reftests on stylo, autoland and m-c repos.

Comment 39

[Bobby Holley (:bholley)]

(In reply to Kim Moir [:kmoir] from comment #38)
> Verified on treeherder that this looks good now.  only e10s debug stylo
> reftests on m-i, both e10s and non-e10s stylo reftests on stylo, autoland
> and m-c repos.

Great! Thanks for helping us to get this right. :-)

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95fa240ac387

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fbcbdd68493
https://hg.mozilla.org/mozilla-central/rev/2fe83738221a
https://hg.mozilla.org/mozilla-central/rev/4f8aecb0f44f