1587826 - Use transform to split `browsertime` applications

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task, P3)

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1587826 - [ci] Create a separate transform file for 'raptor' transforms

Comment 2

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1587826 - [ci] Use a transform to split raptor apps

Depends on D48978

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1587826 - [raptor] Rename raptor configs to <test>-cold

Depends on D48979

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1587826 - [ci] Use a transform to split raptor cold pageload tests

Depends on D48980

Comment 5

[Andrew Halberstadt [:ahal]]

I tested these changes one commit at a time by diffing the taskgraph. There are some expected changes/improvements, but nothing that should change the tasks materially. I'm happy to provide taskgraph diffs between any of these commits on request.

Comment 6

[Andrew Halberstadt [:ahal]]

I only focused on browsertime as there are relatively few of these for now. I wanted to focus there so we don't start following the same precedent as raptor. But these transforms can easily be used to simplify the existing raptor configs.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Taskgraph diff

Comment 8

[Robert Wood [:rwood]]

Comment on attachment 9101365 [details] [diff] [review]
Taskgraph diff

Quick scan of the diff LGTM, thanks!

Comment 9

[Andrew Halberstadt [:ahal]]

I forgot to update the test names in raptor.ini. So just waiting for another successful try run and then I'll land. I also see some failures in the mobile tasks.. but seeing as those are tier 3 they might be pre-existing. I'll make sure to confirm.

Comment 10

[Andrew Halberstadt [:ahal]]

Here's my try push:
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&tier=1%2C2%2C3&author=ahalberstadt%40mozilla.com&fromchange=40ddec328853cb2f9ecb28b0b9373ab2bd06b3d8&tochange=9a72b3ccbab132ebbbc485abfc021205e7203749

The bottom push is unchanged from mozilla-central, and the top is with all these changes. So all of the failures are previously existing and aren't caused by this series.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6848e0fe8aa
[ci] Create a separate transform file for 'raptor' transforms r=tomprince
https://hg.mozilla.org/integration/autoland/rev/316ae6bba580
[ci] Use a transform to split raptor apps r=tomprince,perftest-reviewers,rwood
https://hg.mozilla.org/integration/autoland/rev/52b1060ca5f0
[raptor] Rename raptor configs to <test>-cold r=perftest-reviewers,rwood
https://hg.mozilla.org/integration/autoland/rev/41bf040c31a2
[ci] Use a transform to split raptor cold pageload tests r=perftest-reviewers,rwood

Comment 12

[Andrew Halberstadt [:ahal]]

I'm not going to work on simplifying the configs anymore, but I hope this can provide a good framework going forward. I'd suggest you folks also create a transform for the -profiling tasks (should be very similar to the cold one). It shouldn't be too difficult to convert the raptor tasks to use these transforms either. Though if they will be removed shortly it's probably not worth it.

Comment 13

[Robert Wood [:rwood]]

(In reply to Andrew Halberstadt [:ahal] from comment #12)

I'm not going to work on simplifying the configs anymore, but I hope this can provide a good framework going forward. I'd suggest you folks also create a transform for the -profiling tasks (should be very similar to the cold one). It shouldn't be too difficult to convert the raptor tasks to use these transforms either. Though if they will be removed shortly it's probably not worth it.

Thanks Andrew, much appreciated!

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a6848e0fe8aa
https://hg.mozilla.org/mozilla-central/rev/316ae6bba580
https://hg.mozilla.org/mozilla-central/rev/52b1060ca5f0
https://hg.mozilla.org/mozilla-central/rev/41bf040c31a2

Comment 15

[Greg Mierzwinski [:sparky]]

One issue I've found while trying to setup chrome desktop support on browsertime is that we can't disable warm page load tests and have cold page load tests (from what I can tell). :ahal is there a method that let's us do this or do we need to add a warm field to allow this behaviour?

Another issue (which I can get around) is that the test-sets can't be split by app with the single task definition. I have to create another task specifically for that app to be able to change the test-platforms that the browser runs on. Again, this isn't really problematic since there is a way around it, but I thought I'd mention it here.

Comment 16

[Andrew Halberstadt [:ahal]]

(In reply to Greg Mierzwinski [:sparky] from comment #15)

One issue I've found while trying to setup chrome desktop support on browsertime is that we can't disable warm page load tests and have cold page load tests (from what I can tell). :ahal is there a method that let's us do this or do we need to add a warm field to allow this behaviour?

You can use a tri-state variable. So instead of cold: <bool>, you could change it to pageload: {"cold", "warm", "both"} (with a default to warm). The e10s attribute in the tests.py transforms does something similar.

Another issue (which I can get around) is that the test-sets can't be split by app with the single task definition. I have to create another task specifically for that app to be able to change the test-platforms that the browser runs on. Again, this isn't really problematic since there is a way around it, but I thought I'd mention it here.

Yeah, I don't really like how we use test-sets and test-platforms to map tests to platforms. I filed bug 1541420 to get rid of these files. But in the meantime rather than create a new task, I'd again try to stick to transforms here. So I'd recommend:

1. Create an optional limit-platforms key which is a list of platforms (as defined in test-platforms.yml). You can optionally support pattern matching here.
2. Add it to the handle_keyed_by_app list and make sure the transform that processes it comes afterwards.
3. In the transform, if test["test-platform"] doesn't exist in test["limit-platforms"], then continue without yielding the test. Otherwise yield the test.

This way we can have the limit-platforms key kind of override the definitions in test-platforms.yml and in such a way that we can use by-app with it. This isn't ideal, but I think it's a bit nicer than duplicating tasks by app. And once bug 1587826 is fixed this key wouldn't be needed anymore.

If you do decide to implement this, I think you might as well put it in the tests.py transforms. Non raptor tasks could benefit from it as well.