Closed Bug 1587826 Opened Last month Closed Last month

Use transform to split `browsertime` applications

Categories

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

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

No description provided.
Blocks: 1587576
Blocks: 1587828
Blocks: 1585020

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.

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.

Attached patch Taskgraph diffSplinter Review
Comment on attachment 9101365 [details] [diff] [review]
Taskgraph diff

Quick scan of the diff LGTM, thanks!
Attachment #9101365 - Flags: review+

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.

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.

Pushed by ahalberstadt@mozilla.com:
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

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.

(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!

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.

Flags: needinfo?(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.

Flags: needinfo?(ahal)
Regressions: 1589688
You need to log in before you can comment on or make changes to this bug.