Bug 86916 – garden-o-matic should not fetch from debug bots if it also knows about the release bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

i.e., if we have builders for Mac Lion Release and Mac Lion Debug, we should only fetch from the Release bot when rebaselining.

The reason is that we only check in one kind of baseline file, and if the two bots produce different results, one of them has to lose. Seems like it should be the Debug bot.

I suspect making this change would give us a 2x speed up in rebaselining :). Also, it's not clear if we have *just* a debug bot anywhere, so we could probably simplify this to just "we should only rebaseline release bots"

[Ojan Vafai]

Seems fine to me. The release bots in general should be more likely to be caught up to tip of tree.

[Ojan Vafai]

Wait...do we currently grab baselines off both the release and debug bots for the same rebaseline? Sigh. Well...at least it's an easy perf improvement.

[Ojan Vafai]

We should try to make the code that handles this robust though in case there is in the future a bot that only has a debug bot, e.g. for a while, I think we only had a debug mt lion bot.

[Adam Barth]

Probably the easiest thing to do is to run a filter over the list of bots we're going to try to rebaseline to remove redundant ones.

[Dirk Pranke]

Created attachment 143112 [details]
Patch

[Dirk Pranke]

Work in progress ... thoughts? Should I finish removing dead tests, write new tests, and land-this as-is, and then optimize the backend in a separate patch? Or should I optimize the back-end to at least de-dup release and debug in this patch? Or take a different approach altogether?

[Adam Barth]

Comment on attachment 143112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:95
> +        var tests = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.testName; }));
> +        var builders = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.builderName; }));
> +        var suffixes = base.uniquifyArray(base.flattenArray(

Doesn't this lose the association between tests, builders, and suffixes?  For example, what if I want to rebaseline PNG on Mac but TEXT on Win?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:104
> +        net.post(config.kLocalServerURL + '/rebaselineall?' + $.param({
> +            'builders': builders.join(','),
> +            'tests': tests.join(','),
> +            'suffixes': suffixes.join(',')

By the way, another option here is to send the information in the POST body.  That way we can send all the builder/test/suffix triples and we won't lose the association between them.  The server can sort out what to do with them.

[Adam Barth]

I think losing the association is problematic because it limits what we can do in the future in the UI.  It seems better to send the whole list of triples up to the server and then have it process them.  For the first patch, we might want to have the server run webkit-patch a bunch of times in a loop.  Once we've changed the network protocol, we can optimize the server in a subsequent patch.

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 143112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:95
> > +        var tests = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.testName; }));
> > +        var builders = base.uniquifyArray(failureInfoList.map(function(failureInfo) { return failureInfo.builderName; }));
> > +        var suffixes = base.uniquifyArray(base.flattenArray(
> 
> Doesn't this lose the association between tests, builders, and suffixes?  For example, what if I want to rebaseline PNG on Mac but TEXT on Win?
> 

Yes; this was effectively being lost already on the client side, but you are absolutely correct that it's not as flexible as one could hope for. I was erring on the side of simplicity to start with.

I like the post-the-triples idea. I'll implement that and upload another patch.

[Ojan Vafai]

Comment on attachment 143112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143112&action=review

I agree that this is the right architectural direction (i.e. having more of the logic on the python side).

> Tools/ChangeLog:15
> +        (.):

FYI: I usually delete this lines since they bloat the changelog description.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:104
>> +            'suffixes': suffixes.join(',')
> 
> By the way, another option here is to send the information in the POST body.  That way we can send all the builder/test/suffix triples and we won't lose the association between them.  The server can sort out what to do with them.

I think this is the right thing to do. Lets send this information in the POST body as JSON.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:181
> +    argument_names = "TEST_NAMES..."
> +
> +    def __init__(self):
> +        options = [
> +            optparse.make_option('--builders', action='store', default=None,
> +                help='List of buildbots to pull from'),
> +            optparse.make_option('--platform-to-move-to', action='store', default=None,
> +                help='New platform to move existing baselines to'),
> +            optparse.make_option('--suffixes', action='store', default=None,
> +                help='List of output types to rebaseline (as file extension suffixes)'),

As per the above, this would take JSON as it's only argument and --no-optimize as it's only flag. I suppose we could make a command that does the current behavior as well, but I don't really see a use-case for this command outside of supporting garden-o-matic directly.

[Ojan Vafai]

Lol...sorry for the barrage of redundant comments. :)

[Dirk Pranke]

Created attachment 143146 [details]
post a block of json instead, add tests

[Dirk Pranke]

Comment on attachment 143146 [details]
post a block of json instead, add tests

I love it when deleting code gets you more functionality

[Adam Barth]

Comment on attachment 143146 [details]
post a block of json instead, add tests

View in context: https://bugs.webkit.org/attachment.cgi?id=143146&action=review

Looks great.  One nit below.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/checkout.js:100
> +        net.post(config.kLocalServerURL + '/rebaselineall', JSON.stringify(tests), function() { callback() });

function() { callback() }   <--- You can just pass callback directly without needing to create a thunk.

[Ojan Vafai]

Comment on attachment 143146 [details]
post a block of json instead, add tests

Looks great!

[Dirk Pranke]

Committed r118234: <http://trac.webkit.org/changeset/118234>

[Dirk Pranke]

Created attachment 143666 [details]
Patch

[Adam Barth]

Comment on attachment 143666 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143666&action=review

If this doesn't crash in your test case, please add more tests that would trigger the crash.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:160
> +        builders_to_ports = {}

This variable appears to be unused.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:170
> +            fallback_path = port.baseline_search_path()

Doesn't this crash?  port is local to the loop above.

[Dirk Pranke]

Created attachment 143908 [details]
fix w/ review feedback

[Adam Barth]

Comment on attachment 143908 [details]
fix w/ review feedback

Oh, much nicer.  Thanks.

[Ojan Vafai]

Comment on attachment 143908 [details]
fix w/ review feedback

Yay! LGTM as well.

[Dirk Pranke]

Committed r118447: <http://trac.webkit.org/changeset/118447>