Armen [:armenzg] Assignee
	Description • 11 years ago

This would be cleaner than having to indicate a set-# in the syntax.

	Armen [:armenzg] Assignee
	Comment 1 • 11 years ago

I partially got off the hook for something for the summit. I should be able to give a stab at this during this week.

	Armen [:armenzg] Assignee
	Comment 2 • 11 years ago

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

	Armen [:armenzg] Assignee
	Comment 3 • 11 years ago

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.

	Armen [:armenzg] Assignee
	Comment 4 • 11 years ago

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.

	Armen [:armenzg] Assignee
	Comment 5 • 11 years ago

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).

	Aki Sasaki (not active)
	Comment 6 • 11 years ago

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

	Steve Fink [:sfink] [:s:]
	Comment 7 • 11 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 8 • 11 years ago

Comment on attachment 807952 [details] [diff] [review]
allow for "trychooser_suites"

Argh, submitting my feedback+ seems to have reverted aki's r+.

	Armen [:armenzg] Assignee
	Comment 9 • 11 years ago

> + 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.

	Armen [:armenzg] Assignee
	Comment 10 • 11 years ago

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

	Steve Fink [:sfink] [:s:]
	Comment 11 • 11 years ago

Yes, please include it. It doesn't change functionality; it only changes what dump_master.py will spit out for comparing masters.

	Aki Sasaki (not active)
	Comment 12 • 11 years ago

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

	Armen [:armenzg] Assignee
	Comment 15 • 11 years ago

Live on production.

Not ready to be tested until the builbot-configs patch lands and goes live.

	Armen [:armenzg] Assignee
	Comment 16 • 11 years ago

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.

	bhearsum@mozilla.com (:bhearsum)
	Comment 17 • 11 years ago

something here is in production

	Armen [:armenzg] Assignee
	Comment 18 • 11 years ago

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.

	Armen [:armenzg] Assignee
	Comment 19 • 11 years ago

This code is now live.

We're now waiting on the trychooser code to be updated (bug 922242).

	Aki Sasaki (not active)
	Comment 20 • 11 years ago

Backed out due to bug 922405: http://hg.mozilla.org/build/buildbotcustom/rev/af386435532d

Also noticed the debug prints still in there.

	Armen [:armenzg] Assignee
	Comment 21 • 11 years ago

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>: []}

	Armen [:armenzg] Assignee
	Comment 22 • 11 years ago

- 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']}

	Aki Sasaki (not active)
	Comment 23 • 11 years ago

Comment on attachment 812653 [details] [diff] [review]
allow for "trychooser_suites"

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

    |if not buildersWithSetsMap:|  ?

	Armen [:armenzg] Assignee
	Comment 24 • 11 years ago

(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

	Armen [:armenzg] Assignee
	Comment 25 • 11 years ago

Comment on attachment 812653 [details] [diff] [review]
allow for "trychooser_suites"

https://hg.mozilla.org/build/buildbotcustom/rev/8f4ab71ba7d4

	Aki Sasaki (not active)
	Comment 26 • 11 years ago

(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
{}
[]

	John Hopkins (:jhopkins)
	Comment 28 • 11 years ago

in production

	Armen [:armenzg] Assignee
	Comment 29 • 11 years ago

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

	Armen [:armenzg] Assignee
	Comment 30 • 11 years ago

The last bit of this bug is now live.