589847 - Either auto-generate valid_builders.py or expand try_parser to pull from config.py

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

In order to do the try specific build/test/talos selection, there is currently a flat file called valid_builders.py that give the lists of DESKTOP_PLATFORMS, MOBILE_PLATFORMS, TALOS_SUITES, TEST_SUITES, and PRETTY_NAMES in order to help try_parser.py validate and generate the custom builder set to be scheduled.

To keep this from getting out of sync with try configs (and ultimately m-c configs) we need it to be auto-generated from config.py or to have try_parser.py import config.py and use that to validate/generate.

Comment 1

[Chris AtLee [:catlee]]

I think everything you need is in config.py.  PLATFORMS, SUITES, and UNITTEST_SUITES have all the values.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Yes, and then also we need the PRETTY_NAME mapping from base_name for each platform

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: [WIP] removing valid_builders.py

So this is working in my local staging setup for both the builders as well as the talos/test sendchanges.

It will need more extensive testing in staging and a re-working of the try_parser unit test to makes sure that various use cases are tested before putting this in production but I wanted to show how I was doing it for feedback before going too far into that side of things.

Comment 4

[Chris AtLee [:catlee]]

Comment on attachment 485939 [details] [diff] [review]
[WIP] removing valid_builders.py

there are a bunch of places where we're generating the same pretty name. e.g. "%s build" % base_name appears a bunch of places.  can you factor that out into a new variable?

There's also a TODO left in there, are you still working on that piece?

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: [very close] removes valid_builders.py from both try_parser and test_try_parser

So I've run a lot of tests on this and right now there is just one little bug that I couldn't figure out - when I run it on the test master, sending in unittest values, a platform with more than one slave_platform (in my test case macosx64 which has two) I ended up with adding request for all the 10.5 unittests requested and then double the 10.6 builders.  So that's what is happening on lines 47/48 because it was the only way I could find to get around this and get just one set of unittest builders for each slave_platform.  I can't figure out where the second set is being generated.

Comment 6

[Chris AtLee [:catlee]]

Comment on attachment 490814 [details] [diff] [review]
[very close] removes valid_builders.py from both try_parser and test_try_parser

>diff --git a/misc.py b/misc.py
>--- a/misc.py
>+++ b/misc.py
>@@ -484,40 +484,48 @@ def generateBranchObjects(config, name):
>     }
>     builders = []
>     unittestBuilders = []
>     triggeredUnittestBuilders = []
>     nightlyBuilders = []
>     xulrunnerNightlyBuilders = []
>     debugBuilders = []
>     weeklyBuilders = []
>+    # prettyNames is a mapping to pass to the try_parser for validation
>+    PRETTY_NAME = '%s build'
>+    prettyNames = {}
>+    unittestPrettyNames = {}
>+    unittestSuites = []
>     # These dicts provides mapping between en-US dep and nightly scheduler names
>     # to l10n dep and l10n nightly scheduler names. It's filled out just below here.
>     l10nBuilders = {}
>     l10nNightlyBuilders = {}
>     pollInterval = config.get('pollInterval', 60)
>     l10nPollInterval = config.get('l10nPollInterval', 5*60)
>     # generate a list of builders, nightly builders (names must be different)
>     # for easy access
>     for platform in config['platforms'].keys():
>         pf = config['platforms'][platform]
>         base_name = pf['base_name']

Can you put "pretty_name = PRETTY_NAME % base_name" here?  It'll get rid of the same expansion 4 times below.

>         if platform.endswith("-debug"):
>-            debugBuilders.append('%s build' % base_name)
>+            debugBuilders.append(PRETTY_NAME % base_name)
>+            prettyNames[platform] = PRETTY_NAME % base_name
>             # Debug unittests
>             if pf.get('enable_unittests'):
>                 test_builders = []
>                 base_name = config['platforms'][platform.replace("-debug", "")]['base_name']
>                 for suites_name, suites in config['unittest_suites']:
>+                    unittestPrettyNames[platform] = '%s debug test' % base_name
>                     test_builders.extend(generateTestBuilderNames('%s debug test' % base_name, suites_name, suites))
>                 triggeredUnittestBuilders.append(('%s-%s-unittest' % (name, platform), test_builders, config.get('enable_merging', True)))
>             # Skip l10n, unit tests and nightlies for debug builds
>             continue
>         else:
>-            builders.append('%s build' % base_name)
>+            builders.append(PRETTY_NAME % base_name)
>+            prettyNames[platform] = PRETTY_NAME % base_name
> 

> def generateTalosBranchObjects(branch, branch_config, PLATFORMS, SUITES,
>         ACTIVE_UNITTEST_PLATFORMS, factory_class=TalosFactory):
>     branchObjects = {'schedulers': [], 'builders': [], 'status': [], 'change_source': []}
>     branch_builders = {}
>     all_test_builders = {}
>     all_builders = []
>+    # prettyNames is a mapping to pass to the try_parser for validation
>+    prettyNames = {}
> 
>     buildBranch = branch_config['build_branch']
>     talosCmd = branch_config['talos_command']
> 
>     for platform, platform_config in PLATFORMS.items():
>         if platform_config.get('is_mobile', False):
>             branchName = branch_config['mobile_branch_name']
>             tinderboxTree = branch_config['mobile_tinderbox_tree']
>@@ -1967,16 +1984,22 @@ def generateTalosBranchObjects(branch, b
> 
>         if tinderboxTree not in branch_builders:
>             branch_builders[tinderboxTree] = []
>         if tinderboxTree not in all_test_builders:
>             all_test_builders[tinderboxTree] = []
> 
>         for slave_platform in platform_config['slave_platforms']:
>             platform_name = platform_config[slave_platform]['name']
>+            # this is to handle how a platform has more than one slave platform
>+            if prettyNames.has_key(platform):
>+                prettyNames[platform] = [prettyNames[platform]]
>+                prettyNames[platform].append(platform_name)
>+            else:
>+                prettyNames[platform] = platform_name

So prettyNames[platform] can contain either a list of platform names or a single platform name?  Can you change it so that it always contains a list, even if there's only one platform?

>diff --git a/test/test_try_parser.py b/test/test_try_parser.py
>--- a/test/test_try_parser.py
>+++ b/test/test_try_parser.py
>@@ -1,167 +1,150 @@
>-from buildbotcustom.try_parser import TryParser
>+from ..try_parser import TryParser

Why change to relative imports here?

>-
>-def getTestBuilders(platforms, testType, tests, builderNames, buildTypes):
>+def getTestBuilders(platforms, testType, tests, builderNames, buildTypes, prettyNames, unittestPrettyNames):
>     testBuilders = []
>     # for all possible suites, add in the builderNames for that platform
>     if tests != 'none':
>         if testType == "test":
>             for buildType in buildTypes:
>                 for platform in platforms:
>-                    for test in tests:
>-                        # we only do opt unittests on Rev3 WINNT 6.1, the debug tests still run on pm02
>-                        if platform == 'win32':
>-                          if buildType == 'debug':
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES['desktop_win32'], buildType, testType, test)
>-                          else:
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform][1], buildType, testType, test)
>+                    if platform in prettyNames.keys():
>+                        if type(prettyNames[platform]) is list:

As above, let's make sure that this always is a list, and we can simplify this block a bit.

>+                            for test in tests:
>+                                for slave_platform in prettyNames[platform]:
>+                                    custom_builder = "%s tryserver %s %s %s" % (slave_platform, buildType, testType, test)
>+                                    # note: can anyone else see why without the 'not in' the second slave platform is doubled?
>+                                    if custom_builder in (builderNames) and custom_builder not in testBuilders:
>+                                        testBuilders.extend([custom_builder])
>                         else:
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform], buildType, testType, test)

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

* I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error on base_name not existing yet. 

* prettyNames[platform] can contain either a list of platform names or a
single platform name because in the parser I want to be able to find out if there is more than one slave_platform for a single platform by checking if it is a list type. so I was doing that on purpose. if I make every platform a list then I suppose I can check for length in the parser instead - is that a better way to do this?

* i can remove the relative import, that was for local testing

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to comment #7)
> * I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error
> on base_name not existing yet. 

and I see now that is not what you were suggesting. carrying on.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: [tested] removing valid_builders.py from try_parser

addressed all comments from last review and tested on local builder master and tests master to make sure I'm still getting the builders I want.

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 491259 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser

As per our 1x1, let's not lose the various test aliases we already have (e.g. 'mochitest-1' or 'mochitests')

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: [tested] removing valid_builders.py from try_parser new and improved

Completely re-wrote the unittests for this and actually have useful tests that cover 100% of try_parser

Also ran against local builder_master and tests_master to double-check.

Comment 12

[Chris AtLee [:catlee]]

Comment on attachment 491967 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser new and improved

Looks good!

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 491967 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser new and improved

http://hg.mozilla.org/build/buildbotcustom/rev/ba4d2c17b0b6