Missing Spidermonkey jobs under taskcluster on try

RESOLVED FIXED in mozilla52

Status

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: bbouvier, Assigned: sfink)

Tracking

unspecified
mozilla52

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
See for instance:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4571c403aaa2

I've used the following try line:

try: -b do -p all -u jittests -t none

So I think it should have run a few spidermonkey bugs, which are run on inbound, including but not limited t: arm, arm64, asan, cgc, msan, p, etc.

Or am I missing something here?
Spidermonkey tasks are treated as "ridealong" tasks, so they are run when either -p linux64 is specified ("riding along"), or they are specified explicitly as a platform (e.g., -p sm-tsan).  Ridealongs are not automatically included with -p all.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Component: General → Task Configuration
Resolution: --- → FIXED
(Reporter)

Comment 2

2 years ago
Shouldn't we at least explain this on trychooser (and repurpose this bug)? I guess I'm not the only one who tripped upon this.
And we don't run them automatically when we touch js/src anymore?
Believe me, try is weirder than you would believe.  And it turns out, everyone thinks it should behave a little differently.  I just replicated the existing behavior using a new mechanism.  It doesn't make much sense, even after staring at it for a few months.

There are a few directions to go here:

 * update trychooser with an explanation (http://hg.mozilla.org/build/tools/file/tip/trychooser)
 * update the try behavior to match what you think it should be (it's all in-tree!)
 * replace the shoe-horning of all these tasks in to "platforms" and debug/opt and so on, and build a more expressive, less surprising task-selection mechanism (/cc ahal)
The existing behavior going back to buildbot is that the jobs would run whenever js/src got touched, and I think that should be preserved or you'll end up with a lot of confused JS developers as we saw here.
There's no support for "whenever" -- that would be "always run (even if -p win32??) except when no files under js/src change".  That said, adjusting these jobs to run-on-project: ['all'] or ['try'] would make these run with -p all.
Flags: needinfo?(sphink)
(Assignee)

Comment 7

2 years ago
Oh, man. I thought I finally understood the new scheduling wrt run-on-projects, 'when', and RIDEALONG_BUILDS, but this bug and a similar issue that fitzgen ran into today show that I don't.

My expectation was that ridealong builds would cover this -- that -p all includes the linux64 platform, and that whenever the linux64 platform was included, all ridealong builds would be included subject to their 'when' clauses.

But then, my (most recent) expectation of run-on-projects was that if a non-try thing was included, that the jobs would be invoked on any push (still subject to 'when'). And if 'try' was included, that those jobs would be included with -p all regardless of RIDEALONG_BUILDS. But I'm realizing that I'm not really sure what run-on-projects: ['try'] means.

I just read the documentation (which is really hard to find at all on readthedocs, and can only be found in the source tree if you know to search for run_on_projects instead of run-on-projects). It seems it is very explicitly only relevant for -p all pushes.

Could we change the ridealong mechanism to line up with my expectation above? As in, '-p all' means 'include linux64' means 'include all linux64 ridealongs'? Or would that change other things too?

As for a better general mechanism, I agree, and have vague but not firm ideas.
Flags: needinfo?(sphink) → needinfo?(dustin)
(Assignee)

Comment 8

2 years ago
Created attachment 8793927 [details] [diff] [review]
Make ridealong builds apply whenever host platform is used

So I'm thinking of something like this, although I don't know how to test it easily? Seems like you had a nice setup for taskgraph diffing.

Apologies for using the name *host* platform. Makes me think of cross-compiling. daddy_platform? build_platform? main_platform? base_platform?
Attachment #8793927 - Flags: feedback?(dustin)
(Assignee)

Updated

2 years ago
Assignee: nobody → sphink
(In reply to Steve Fink [:sfink] [:s:] (PTO Sep23-28) from comment #7)
> My expectation was that ridealong builds would cover this -- that -p all
> includes the linux64 platform, and that whenever the linux64 platform was
> included, all ridealong builds would be included subject to their 'when'
> clauses.

I thought so too, for a while, but on investigation the old (legacy.py) implementation only rode along with -p <explicit-platform>, so that's what I did.

> But then, my (most recent) expectation of run-on-projects was that if a
> non-try thing was included, that the jobs would be invoked on any push
> (still subject to 'when'). And if 'try' was included, that those jobs would
> be included with -p all regardless of RIDEALONG_BUILDS. But I'm realizing
> that I'm not really sure what run-on-projects: ['try'] means.

For the record, the current arrangement is
 * a task's run_on_projects attribute including `try` or `all` means that it will be selected as a target task with `-p all` or with `-p <task platform>`
 * if a task's platform is listed under platform *plat* in RIDEALONG_BUILDS, then the task will be selected with `-p <plat>` [after your patch lands, add .. or `-p all`]
 * in either case, the task will not be run (it will be "optimized away") if nothing under js/src/, etc. has changed in this push

> I just read the documentation (which is really hard to find at all on
> readthedocs, and can only be found in the source tree if you know to search
> for run_on_projects instead of run-on-projects). It seems it is very
> explicitly only relevant for -p all pushes.

I'm sorry-not-sorry about the -/_ thing.  The `-` applies to the job and task descriptions (`-` is natural in YAML), while the `_` is used for attributes.  I wouldn't object to a patch to change all attributes to use `-`, and now would be a good time as I don't think anything outside the source tree is looking at them yet.

I think we should keep the correspondance of `-p all` and `run_on_projects`, as it provides much-requested feature parity with Buildbot's "tryByDefault".

I'm happy to see a patch to improve the docs.. "is really hard to find" is a legitimate critique, but hard to turn into a fix.
Status: RESOLVED → REOPENED
Flags: needinfo?(dustin)
Resolution: FIXED → ---
Attachment #8793927 - Flags: feedback?(dustin) → feedback+
(In reply to Steve Fink [:sfink] [:s:] (PTO Sep23-28) from comment #8)
> So I'm thinking of something like this, although I don't know how to test it
> easily? Seems like you had a nice setup for taskgraph diffing.

http://gecko.readthedocs.io/en/latest/taskcluster/taskcluster/how-tos.html#hacking-task-graphs

> Apologies for using the name *host* platform. Makes me think of
> cross-compiling. daddy_platform? build_platform? main_platform?
> base_platform?

Hah, I missed that in my r+.  It probably should be changed as others will doubtless have the same confusion.  How about lead platform? master platform?  The r+ stands with whatever you choose (or if you decide to stick with "host").
(Assignee)

Comment 11

2 years ago
Created attachment 8793971 [details] [diff] [review]
Run all tier-1 spidermonkey jobs with try: -p all

I'm not really sure if dustin is the right reviewer, but at least he can confirm whether the change accomplishes the desired functionality: all tier-1 spidermonkey builds should be run on try if try: -p all is given.

I sort of wonder if there shouldn't be some kind of cross-check for this -- if something is tier 1, it should really be running by default on try. (And it should be running on integration and release branches, otherwise what's the point?) But that's more of a policy check than a schema integrity check. Still, I wonder if it'd be worth doing until we find an exception?
Attachment #8793971 - Flags: review?(dustin)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8793927 [details] [diff] [review]
Make ridealong builds apply whenever host platform is used

I do not think we want this patch. As I'm realizing, leaving 'try' out of run-on-projects is intended to replicate the old try-by-default:False behavior. And for that, we suppressed the builds when running with -p all.

The mechanism for requesting those builds is rather funky, though. With buildbot, you'd get them if you requested their platform explicitly. Which is pretty messed up, honestly -- just because you want to do a test on win32 or whatever, you get all the weirdo non-tier-1 builds that people have defined. I wouldn't mind ditching that mechanism. While I *do* want these builds if you do -p linux64, that's just because they're tier 1. If you do -p linux64, I'm fine if you don't get the half-baked jobs like sm-tsan. As long as there's a simple mechanism to get them (eg -p sm-tsan), I'm fine. (I'd be fine if it were spelled -j sm-tsan too.)
Attachment #8793927 - Attachment is obsolete: true
+1 to the idea of adding policy checks to the decision task (but that's for another bug)
Oh, now that I read comment 12 again -- you *should* get SM builds with things like `-p sm-tsan`.  For the most part, the key in the YAML is the `-p` argument (aka "try platform").  Is that not the case?
Comment on attachment 8793971 [details] [diff] [review]
Run all tier-1 spidermonkey jobs with try: -p all

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

Looks exactly right
Attachment #8793971 - Flags: review?(dustin) → review+
(Assignee)

Comment 16

2 years ago
(In reply to Dustin J. Mitchell [:dustin] from comment #14)
> Oh, now that I read comment 12 again -- you *should* get SM builds with
> things like `-p sm-tsan`.  For the most part, the key in the YAML is the
> `-p` argument (aka "try platform").  Is that not the case?

Yes, sorry, I was not trying to imply otherwise. The current state of -p sm-tsan is good. The current state of -p linux64 is questionable (it matches historical behavior, but it might make sense to change it at some point). And I was just saying that if we needed to change the canonical way to request just sm-tsan, I'd be fine with that. (eg from its current -p sm-tsan to -j sm-tsan or really anything else.)

For now, the current state is fine from my point of view.

Comment 17

2 years ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90c19ed19a55
Run all tier-1 spidermonkey jobs with -p all try syntax, r=dustin

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90c19ed19a55
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

6 months ago
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.