Jobs with "file_patterns" should get considered on try even if no try syntax is specified

RESOLVED FIXED in mozilla53

Status

Taskcluster
Scheduler
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla53

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
I recently filed a bug to "stop scheduling all jobs on try if there is no try syntax", so I see the irony here.

But jobs that have a "file_patterns" clause are meant to be "smart". I.e, they can determine for themselves when they need to run and when they don't. This means there is no try syntax for them, as developers never need to worry about when to run them.

However, they don't get considered when a developer pushes to try without a try syntax, and I believe they should be (if they don't require any parent tasks). Otherwise, the only way to run them on try is to use a dummy try syntax that schedules jobs you don't need.
(Assignee)

Comment 1

2 years ago
Here's an example to illustrate what I mean:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa12a92670ed09471916ce8d2e1f8f058ff94201

In that push, I was only interested in the ES job. But in order to get it to run, I had to schedule that linux64 build/xpcshell job. I should be able to just push with an empty try message and get the ES job automatically (if I touched the right files).
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
Here is the above patch in action:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e46ea575c1297745ad86ed051af14c5bc7ae82

Notice there's no try syntax, but the f8 and tg tasks were scheduled anyway because I modified python files under /taskcluster. One edge case might be a "when" task that has dependencies.. I don't know if such tasks exist, but with this patch, their dependencies will also be scheduled. I'm not sure if that is a good or bad thing.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED

Comment 4

2 years ago
mozreview-review
Comment on attachment 8809610 [details]
Bug 1275943 - Schedule tasks with a 'when' clause regardless of try syntax,

https://reviewboard.mozilla.org/r/92152/#review92330

I trust your judgement more than mine for try usage patterns, so if you think this is a good idea, r+ for the implementation.

From what I can tell, this basically makes the `-j` option a no-op?

::: taskcluster/taskgraph/try_option_syntax.py:525
(Diff revision 1)
>                  return False
>              if 'only_chunks' in test and attr('test_chunk') not in test['only_chunks']:
>                  return False
>              return True
>  
> +        # tasks with a 'when' clause should be scheduled regardless of try syntax

"should be included in the target set .. They may later be optimized away"
Attachment #8809610 - Flags: review?(dustin) → review+
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8809610 [details]
Bug 1275943 - Schedule tasks with a 'when' clause regardless of try syntax,

https://reviewboard.mozilla.org/r/92152/#review92330

Ah, I didn't even know I could use -j to schedule lint jobs! I don't know if this makes -j obsolete or not, is it exclusively used for jobs with "when" clauses? It's probable that this at least removes one of the main use cases of -j though.

But either way, I still think this is a good idea. For example, imagine a developer makes a change under `taskcluster/taskgraph` but doesn't know how to run the 'tg' tests (or that they exist). This way, they can just push to try knowing the right thing will happen. On the flip side, if someone makes a change under `taskcluster/taskgraph`, I can't really imagine a scenario we *wouldn't* want to run the 'tg' tests on a try push.

From a more ideological sense, this is a baby step towards a world where try syntax isn't required and CI is smart enough to figure out what to run based on what changed.

p.s this comment is more for archaeology than trying to convince you :). I'll also do a newsgroup post before landing.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Here's gps' commit message when he added "-j":
https://hg.mozilla.org/mozilla-central/annotate/623765c2381e/testing/taskcluster/taskcluster_graph/commit_parser.py#l261

So I think tasks that have a "when" clause are a subset of non-build/test tasks (i.e ones scheduled with -j). I also think, coincidentally, those two sets are currently identical. Also note, that -j tasks are "all" scheduled by default anyway unless "-j" is explicitly passed in.

This makes me feel confident that there is a bug here. They *should* be added when pushing to try without syntax. After reading that commit message, I now think my patch doesn't go quite far enough. It should schedule all "job" tasks, whether or not they have a "when" clause. Feel free to needinfo gps if you want confirmation about all this, but I'm confident this is the right thing to do and accept all blame if anyone gets angered :)

Uploading new patch which I think you'll like much better shortly.. Sorry in advance for the review churn!
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8809610 - Attachment is obsolete: true

Comment 9

2 years ago
mozreview-review
Comment on attachment 8810516 [details]
Bug 1275943 - Ensure job tasks are added to target set even if no try syntax specified,

https://reviewboard.mozilla.org/r/92790/#review93120

I think the better fix here would be to make all of the default values when `try:` does not appear in the commit message match those that are applied when individual options are omitted, rather than carefully address the differences between `[]` and `None`.  I admit I never really thought about the no-syntax situation (I thought one or both of `mach try` and the hg hook would reject that, in fact.. it certainly rejects things with bogus `-p` options), so those defaults are really just wrong and never given much thought.

That said, this patch works :)
Attachment #8810516 - Flags: review?(dustin) → review+

Comment 10

2 years ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3b28797c209
Ensure job tasks are added to target set even if no try syntax specified, r=dustin

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3b28797c209
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.