Closed Bug 915870 Opened 7 years ago Closed 6 years ago

Make trychooser smart enough to map an Android x86 suite (e.g. mochitest-7) to a set (e.g. set-2)

Categories

(Release Engineering :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: armenzg)

References

Details

(Keywords: trychooser)

Attachments

(4 files, 1 obsolete file)

This would be cleaner than having to indicate a set-# in the syntax.
Keywords: trychooser
Assignee: nobody → armenzg
Blocks: 917361
No longer blocks: 891959
Priority: -- → P3
I partially got off the hook for something for the summit. I should be able to give a stab at this during this week.
Priority: P3 → P2
I think I found part of the code:
2013-09-18 14:11:13-0700 [-] Found try message in the change comments, ignoring push comments
2013-09-18 14:11:13-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt'], talos=u'none', test=u'plain-reftest-1,plain-reftest-6,robocop-3,xpcshell', user_platforms=set([u'android-x86'])) : try: -b o -p android-x86 -u plain-reftest-1,plain-reftest-6,robocop-3,xpcshell -t none
2013-09-18 14:11:13-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0x182e9cf8>: ['Android 4.2 x86 try build']}

http://mxr.mozilla.org/build/source/buildbotcustom/misc_scheduler.py#65
http://mxr.mozilla.org/build/source/buildbotcustom/try_parser.py#359
http://mxr.mozilla.org/build/source/buildbotcustom/scheduler.py#212
I've created a small test case from where to expand and make things work.

The idea is to do some sort of mapping:
* Turn this ['androidx86-set-1', 'androidx86-set-2', ...]
* into this ['mochitest-1', 'mochitest-2', ...]

That way if someone asks for mochitest-1 it will trigger the appropriate set even if other suites go side-by-side with it.
I'm starting to wonder if I should be going through this painful code when I could simply add an Android x86 section on the trychooser and give indications as to what sets 1 to 8 run.

If we think about it, we already do the same for "mochitest-other".
We don't indicate the name of the suites that run within it.

If we agree that it is fine then this bug will become WORKSFORME as we can trigger Android x86 tests on the try server.
Attached patch allow for "trychooser_suites" (obsolete) — Splinter Review
aki, feel free to pass the review to catlee if you prefer to.

RyanVM, sfink: the "trychooser_suites" gives us the flexibility to match even "plain-reftest-1" instead of "reftest-1" to "androidx86-set-1" if we wanted to.

What this patch does is to match any suite that runs inside of a set and trigger that set (even if the other 3 were not requested for).
Attachment #807952 - Flags: review?(aki)
Attachment #807952 - Flags: feedback?(sphink)
Attachment #807952 - Flags: feedback?(ryanvm)
Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

rubberstamp-if-things-work-in-staging
+ obligatory we-need-to-redo-scheduling comment
+ obligatory trychooser-is-a-hack comment
Attachment #807952 - Flags: review?(aki) → review+
Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

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

I think this is fine for a short-term solution (not that I fully understand the problem, but I have some idea from reading various philor complaints.)

I have started work on a longer-term solution, where I replace the bizarrely-named prettyNames with a dict giving a bunch of different attributes for each builder, to allow matching without flaky string parsing or adding parameters. This will allow a superset of what this patch does. Hopefully, it will also be an easy data structure to export, so we can use it directly in the trychooser UI, the trychooser hg extension, and tbpl at least.

That said, I wouldn't want to hold this patch's functionality up on that (I've no idea how long it'll take to finish). And it'll be easy to replace with my stuff. Heck, I'm very happy to build on top of this, because it'll provide the actual data of what corresponds to what, and I don't know that part.

Do you think this could be extended to do eg "build all the b2g platforms"? That's another outstanding annoyance. An everchanging |-p panda,unagi,inari,...| is pretty lame.
Attachment #807952 - Flags: review?
Attachment #807952 - Flags: review+
Attachment #807952 - Flags: feedback?(sphink)
Attachment #807952 - Flags: feedback+
Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

Argh, submitting my feedback+ seems to have reverted aki's r+.
Attachment #807952 - Flags: review? → review+
> + obligatory we-need-to-redo-scheduling comment
> + obligatory trychooser-is-a-hack comment

Can you please expand on this?

(In reply to Steve Fink [:sfink] from comment #7)
> Do you think this could be extended to do eg "build all the b2g platforms"?
> That's another outstanding annoyance. An everchanging |-p
> panda,unagi,inari,...| is pretty lame.
>
We could have another hack like that. Worth filing separately if your solution would take a while to come.
aki, what did you mean with this?
> + obligatory we-need-to-redo-scheduling comment
> + obligatory trychooser-is-a-hack comment

aki, do I need this line? I tested the patch on staging without it and it still worked.

diff --git a/scheduler.py b/scheduler.py
--- a/scheduler.py
+++ b/scheduler.py
@@ -135,27 +135,28 @@ class PersistentScheduler(BaseScheduler)
         # Try again in a bit
         self.lastCheck = now()
         return now() + self.pollInterval
 
 
 class BuilderChooserScheduler(MultiScheduler):
     compare_attrs = MultiScheduler.compare_attrs + (
         'chooserFunc', 'prettyNames',
-        'unittestPrettyNames', 'unittestSuites', 'talosSuites', 'buildbotBranch')
+        'unittestPrettyNames', 'unittestSuites', 'talosSuites', 'buildbotBranch', 'buildersWithSetsMap')
 
     def __init__(
Flags: needinfo?(aki)
Yes, please include it. It doesn't change functionality; it only changes what dump_master.py will spit out for comparing masters.
Flags: needinfo?(aki)
(In reply to Armen Zambrano [:armenzg] (Release Engineering) (EDT/UTC-4) from comment #9)
> > + obligatory we-need-to-redo-scheduling comment
> > + obligatory trychooser-is-a-hack comment
> 
> Can you please expand on this?

I think trychooser works to some degree, and is useful when it works, but the real solution would involve rewriting our scheduling so we can schedule try jobs in a more... integrated way.

(In reply to Armen Zambrano [:armenzg] (Release Engineering) (EDT/UTC-4) from comment #10)
> aki, do I need this line? I tested the patch on staging without it and it
> still worked.
> 
> diff --git a/scheduler.py b/scheduler.py
> --- a/scheduler.py
> +++ b/scheduler.py
> @@ -135,27 +135,28 @@ class PersistentScheduler(BaseScheduler)
>          # Try again in a bit
>          self.lastCheck = now()
>          return now() + self.pollInterval
>  
>  
>  class BuilderChooserScheduler(MultiScheduler):
>      compare_attrs = MultiScheduler.compare_attrs + (
>          'chooserFunc', 'prettyNames',
> -        'unittestPrettyNames', 'unittestSuites', 'talosSuites',
> 'buildbotBranch')
> +        'unittestPrettyNames', 'unittestSuites', 'talosSuites',
> 'buildbotBranch', 'buildersWithSetsMap')
>  
>      def __init__(

I think it might be needed for comparing schedulers, either as sfink says in comment 11, or possibly for reconfigs.
Attachment #807952 - Flags: feedback?(ryanvm) → checked-in+
Attachment #809232 - Flags: review?(aki)
Attachment #809233 - Flags: review?(emorley)
Live on production.

Not ready to be tested until the builbot-configs patch lands and goes live.
Attachment #809232 - Flags: review?(aki) → review+
Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

I backed this out this morning as some masters were failing.
I will fix it later this week.
Attachment #807952 - Flags: checked-in+ → checked-in-
Attachment #809233 - Flags: review?(emorley) → review+
Attachment #809232 - Flags: checked-in+
Attachment #809233 - Flags: checked-in+
something here is in production
Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

I just changed != {} for != None.
The builbotcustom tests passed locally.
I will keep an eye on test-masters before dropping off for the day.
Attachment #807952 - Flags: checked-in- → checked-in+
Depends on: 922242
This code is now live.

We're now waiting on the trychooser code to be updated (bug 922242).
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 922405
Backed out due to bug 922405: http://hg.mozilla.org/build/buildbotcustom/rev/af386435532d

Also noticed the debug prints still in there.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Removed debug prints.

We can see test=[] in the TryChooser OPTIONS message.
I bet this got broken when I switched from {} to None in the if condition to fix the buildbotcustom tests.

I will re-test.

Adding some logs:
2013-09-30 12:05:43-0700 [-] received a Change with when > now; assuming the change happened now
2013-09-30 12:05:43-0700 [-] Looking at changes: [<buildbot.changes.changes.Change instance at 0xa76d368>]
2013-09-30 12:05:43-0700 [-] Found try message in the change comments, ignoring push comments
2013-09-30 12:05:43-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt', 'debug'], talos=u'none', test=[], user_platforms=set([u'win32', u'macosx64', u'linux'])) : try: -b do -p linux,macosx64,win32 -u xpcshell,mochitests -t none
2013-09-30 12:05:43-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0xa76d368>: []}
- Removed debug code.
- Change the following:
if buildersWithSetsMap != None:
if buildersWithSetsMap != None and buildersWithSetsMap != {}:

I tested a platform with "trysyntax_suites" defined and another without it.

test_try_parser.py should be fixed as it reported all tests passing.

With the old code, when doing a sendchange to my test master I get this:
2013-10-01 07:39:18-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt'], talos='none', test=['androidx86-set-1'], user_platforms=set(['android-x86'])) : try: -b o -p android-x86 -u mochitest-1 -t none
2013-10-01 07:39:18-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0x790fd88>: ['Android 4.2 x86 Emulator try opt test androidx86-set-1']}

With the new code, when doing a sendchange to my test master I get this:
2013-10-01 07:38:06-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt'], talos='none', test=['androidx86-set-1'], user_platforms=set(['android-x86'])) : try: -b o -p android-x86 -u mochitest-1 -t none
2013-10-01 07:38:06-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0x63e8950>: ['Android 4.2 x86 Emulator try opt test androidx86-set-1']}


Now with a platform that does not have "trychooser_suites" defined.

With the old code, when doing a sendchange to my test master I get this:
2013-10-01 07:43:21-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt'], talos='none', test=[], user_platforms=set(['android'])) : try: -b o -p android -u mochitest-1 -t none
2013-10-01 07:43:21-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0x92c2170>: []}

With the new code, when doing a sendchange to my test master I get this:
2013-10-01 07:45:12-0700 [-] TryChooser OPTIONS : MESSAGE Namespace(build=['opt'], talos='none', test=['mochitest-1'], user_platforms=set(['android'])) : try: -b o -p android -u mochitest-1 -t none
2013-10-01 07:45:12-0700 [-] Adding request for {<buildbot.changes.changes.Change instance at 0x6090440>: ['Android 2.2 Tegra try opt test mochitest-1']}
Attachment #812653 - Attachment description: androidx86.buildbotcustom.diff → allow for "trychooser_suites"
Attachment #812653 - Flags: review?(aki)
Attachment #807952 - Attachment is obsolete: true
Comment on attachment 812653 [details] [diff] [review]
allow for "trychooser_suites"

>+    if buildersWithSetsMap != None and buildersWithSetsMap != {}:

    |if not buildersWithSetsMap:|  ?
Attachment #812653 - Flags: review?(aki) → review+
(In reply to Aki Sasaki [:aki] from comment #23)
> Comment on attachment 812653 [details] [diff] [review]
> allow for "trychooser_suites"
> 
> >+    if buildersWithSetsMap != None and buildersWithSetsMap != {}:
> 
>     |if not buildersWithSetsMap:|  ?

I think that is wrong.
In any case, I believe it would be this (not using the "not") :
> if buildersWithSetsMap and buildersWithSetsMap != {}

>>> h = None
>>> print "enter the if clause" if h else "we don't enter"
we don't enter
>>> print "enter the if clause" if not h else "we don't enter"
enter the if clause
(In reply to Armen Zambrano [:armenzg] (Release Engineering) (EDT/UTC-4) from comment #24)
> (In reply to Aki Sasaki [:aki] from comment #23)
> > Comment on attachment 812653 [details] [diff] [review]
> > allow for "trychooser_suites"
> > 
> > >+    if buildersWithSetsMap != None and buildersWithSetsMap != {}:
> > 
> >     |if not buildersWithSetsMap:|  ?
> 
> I think that is wrong.
> In any case, I believe it would be this (not using the "not") :
> > if buildersWithSetsMap and buildersWithSetsMap != {}

>>> for i in (None, {}, [], {'a': 1}):
...     if not i:
...         print i
...
None
{}
[]
Attachment #814902 - Flags: review?(aki) → review+
in production
Comment on attachment 814902 [details] [diff] [review]
adjust how the if clause is defined plus add a comment

https://hg.mozilla.org/build/buildbotcustom/rev/2eaf43f258c5
Attachment #814902 - Flags: checked-in+
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
The last bit of this bug is now live.
Component: Tools → General
You need to log in before you can comment on or make changes to this bug.