Bug 86916

Summary: garden-o-matic should not fetch from debug bots if it also knows about the release bots
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
post a block of json instead, add tests
none
Patch
none
fix w/ review feedback abarth: review+

Dirk Pranke
Reported 2012-05-18 15:52:26 PDT
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"
Attachments
Patch (8.46 KB, patch)
2012-05-21 15:17 PDT, Dirk Pranke
no flags
post a block of json instead, add tests (11.29 KB, patch)
2012-05-21 17:29 PDT, Dirk Pranke
no flags
Patch (7.55 KB, patch)
2012-05-23 15:54 PDT, Dirk Pranke
no flags
fix w/ review feedback (7.35 KB, patch)
2012-05-24 15:45 PDT, Dirk Pranke
abarth: review+
Ojan Vafai
Comment 1 2012-05-18 15:58:39 PDT
Seems fine to me. The release bots in general should be more likely to be caught up to tip of tree.
Ojan Vafai
Comment 2 2012-05-18 15:59:39 PDT
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
Comment 3 2012-05-18 16:00:29 PDT
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
Comment 4 2012-05-20 20:49:49 PDT
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
Comment 5 2012-05-21 15:17:18 PDT
Dirk Pranke
Comment 6 2012-05-21 15:18:27 PDT
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 7 2012-05-21 15:31:43 PDT
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
Comment 8 2012-05-21 15:33:28 PDT
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
Comment 9 2012-05-21 15:38:10 PDT
(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 10 2012-05-21 15:41:00 PDT
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
Comment 11 2012-05-21 15:41:55 PDT
Lol...sorry for the barrage of redundant comments. :)
Dirk Pranke
Comment 12 2012-05-21 17:29:59 PDT
Created attachment 143146 [details] post a block of json instead, add tests
Dirk Pranke
Comment 13 2012-05-21 17:31:32 PDT
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 14 2012-05-21 17:38:39 PDT
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 15 2012-05-21 18:50:53 PDT
Comment on attachment 143146 [details] post a block of json instead, add tests Looks great!
Dirk Pranke
Comment 16 2012-05-23 13:34:57 PDT
Dirk Pranke
Comment 17 2012-05-23 15:54:04 PDT
Adam Barth
Comment 18 2012-05-24 15:36:37 PDT
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
Comment 19 2012-05-24 15:45:48 PDT
Created attachment 143908 [details] fix w/ review feedback
Adam Barth
Comment 20 2012-05-24 15:47:30 PDT
Comment on attachment 143908 [details] fix w/ review feedback Oh, much nicer. Thanks.
Ojan Vafai
Comment 21 2012-05-24 16:00:51 PDT
Comment on attachment 143908 [details] fix w/ review feedback Yay! LGTM as well.
Dirk Pranke
Comment 22 2012-05-24 17:03:23 PDT
Note You need to log in before you can comment on or make changes to this bug.