Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2384243005:
Add initial CL for RandomOrder chromium.fyi bot (Closed)
DescriptionAdd initial CL for RandomOrder chromium.fyi bot
This is a set of 2 CLs:
1. build CL (this one) - https://codereview.chromium.org/2384243005/
2. src CL - https://codereview.chromium.org/2404573002
BUG=652357
Committed: https://chromium.googlesource.com/chromium/tools/build/+/fc1d3b8658c112563b0c3ae7f6e3303d908fcee1
Patch Set 1 #Patch Set 2 : Add new RandomOrder chromium.expected file #Patch Set 3 : Update build machine based on new bot allocation #
Total comments: 6
Patch Set 4 : Address CL feedback #
Total comments: 6
Patch Set 5 : Remove --seed since it now defaults to unixtime #Patch Set 6 : Train new expectations #
Messages
Total messages: 30 (10 generated)
jeffcarp@chromium.org changed reviewers: + qyearsley@chromium.org
Description was changed from ========== [Do not merge] Add initial CL for RandomOrder chromium.fyi bot BUG=652357 ========== to ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL (forthcoming - BUG=652357 ==========
Description was changed from ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL (forthcoming - BUG=652357 ========== to ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL (forthcoming) - BUG=652357 ==========
Description was changed from ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL (forthcoming) - BUG=652357 ========== to ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL - https://codereview.chromium.org/2404573002 BUG=652357 ==========
https://codereview.chromium.org/2384243005/diff/40001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2384243005/diff/40001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:1047: b_chromium_linux_webkit_randomorder= { Nit: space after = https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/chromium/... File scripts/slave/chromium/layout_test_wrapper.py (right): https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/chromium/... scripts/slave/chromium/layout_test_wrapper.py:98: # TODO: Do we need to add --order and --seed here? Ah, good question -- I think we do. Note, I think that this layout tests wrapper script is only used for some platforms currently. https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:203: '--seed=$(date +%s)', Hm, I suspect that this may not actually be expanded the way we want it to... Do you think it would work to do '--seed=%d' % time.time() ? Or maybe we have to modify run-webkit-tests to be able to take --seed=time or something?
https://codereview.chromium.org/2384243005/diff/40001/masters/master.chromium... File masters/master.chromium.fyi/master.cfg (right): https://codereview.chromium.org/2384243005/diff/40001/masters/master.chromium... masters/master.chromium.fyi/master.cfg:1047: b_chromium_linux_webkit_randomorder= { On 2016/10/07 at 21:56:58, qyearsley wrote: > Nit: space after = Thanks! https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/chromium/... File scripts/slave/chromium/layout_test_wrapper.py (right): https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/chromium/... scripts/slave/chromium/layout_test_wrapper.py:98: # TODO: Do we need to add --order and --seed here? On 2016/10/07 at 21:56:58, qyearsley wrote: > Ah, good question -- I think we do. Note, I think that this layout tests wrapper script is only used for some platforms currently. Added. https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2384243005/diff/40001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:203: '--seed=$(date +%s)', On 2016/10/07 at 21:56:58, qyearsley wrote: > Hm, I suspect that this may not actually be expanded the way we want it to... > > Do you think it would work to do '--seed=%d' % time.time() ? Or maybe we have to modify run-webkit-tests to be able to take --seed=time or something? Good catch, I don't know, can we just try it and see if it breaks?
Minus the comment I just made I think this should be good to go. https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json:7: "[BUILDER_CACHE]/WebKit_Linux___RandomOrder", This doesn't look right, I'll take a look at it.
I think that this LGTM to try to submit to see what happens, although I suspect that there's a good chance that $(date +%s) won't work and so we're likely to want to modify run-webkit-tests to be able to set the seed to the current timestamp in a follow-up CL. https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json:7: "[BUILDER_CACHE]/WebKit_Linux___RandomOrder", On 2016/10/10 at 22:08:01, jeffcarp wrote: > This doesn't look right, I'll take a look at it. I think that this is right -- the diff says: copy from scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___WPTServe.json And so the diff shows the difference from that file, but this is a new added file. Which is probably right.
Sounds good. Also last week I added a patch that replaces $(date +%s) with your Python-based suggestion. Going to merge. Next step after both are merged is to file a master restart bug? https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... File scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipes/c... scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___RandomOrder.json:7: "[BUILDER_CACHE]/WebKit_Linux___RandomOrder", On 2016/10/10 at 22:32:14, qyearsley wrote: > On 2016/10/10 at 22:08:01, jeffcarp wrote: > > This doesn't look right, I'll take a look at it. > > I think that this is right -- the diff says: > > copy from scripts/slave/recipes/chromium.expected/full_chromium_fyi_WebKit_Linux___WPTServe.json > > And so the diff shows the difference from that file, but this is a new added file. Which is probably right. Oops good catch, I mistook that for the real filename.
On 2016/10/10 at 22:43:21, jeffcarp wrote: > Sounds good. Also last week I added a patch that replaces $(date +%s) with your Python-based suggestion. Going to merge. Next step after both are merged is to file a master restart bug? > Yep, in addition to the mb_config.pyl change (http://crrev.com/2404573002). :-)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org
This CL should be reviewed by Dirk as well before submitting, I believe; Dirk, does this look correct?
lgtm, apart from the problem I note below that probably defeats the whole point :). On a related note, we should get rid of layout_test_wrapper.py. I got rid of it partially (we don't use it on windows) but never got around to getting rid of it on the other platforms. Maybe one of you can pick that up? https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:204: '--seed=%d' % time.time(), I believe this seed will change only when the buildbot master is restarted, which I'd guess is not your intent (i.e., it won't be very random).
> On a related note, we should get rid of layout_test_wrapper.py Sounds good. Found the bug for that https://bugs.chromium.org/p/chromium/issues/detail?id=618791 https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:204: '--seed=%d' % time.time(), On 2016/10/10 at 23:30:22, Dirk Pranke wrote: > I believe this seed will change only when the buildbot master is restarted, which I'd guess is not your intent (i.e., it won't be very random). Got it. In that case it sounds like we'll need to add some logic to run-webkit-tests?
On 2016/10/11 00:21:58, jeffcarp wrote: > > On a related note, we should get rid of layout_test_wrapper.py > > Sounds good. Found the bug for that > https://bugs.chromium.org/p/chromium/issues/detail?id=618791 > > https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... > File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): > > https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... > scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:204: '--seed=%d' % > time.time(), > On 2016/10/10 at 23:30:22, Dirk Pranke wrote: > > I believe this seed will change only when the buildbot master is restarted, > which I'd guess is not your intent (i.e., it won't be very random). > > Got it. In that case it sounds like we'll need to add some logic to > run-webkit-tests? Yes, that would be the easiest way to implement this, and is probably an argument for changing the default value used for --seed from a constant to something like the timestamp. (Then you just wouldn't pass a seed along). I don't remember if we talked about the tradeoffs here before, but check w/ blink-infra, maybe?
Just discussed with Quinten - we were talking about adding a special value, e.g. --seed=time. Another option could be to add an entirely new flag, something like --seed-time. I can send a message to blink-infra about it. On Mon, Oct 10, 2016 at 5:22 PM <jeffcarp@chromium.org> wrote: > > On a related note, we should get rid of layout_test_wrapper.py > > Sounds good. Found the bug for that > https://bugs.chromium.org/p/chromium/issues/detail?id=618791 > > > > > https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... > File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py > (right): > > > https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... > scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:204: > '--seed=%d' % time.time(), > On 2016/10/10 at 23:30:22, Dirk Pranke wrote: > > I believe this seed will change only when the buildbot master is > restarted, which I'd guess is not your intent (i.e., it won't be very > random). > > Got it. In that case it sounds like we'll need to add some logic to > run-webkit-tests? > > https://codereview.chromium.org/2384243005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/11 00:30:03, chromium-reviews wrote: > Just discussed with Quinten - we were talking about adding a special value, > e.g. --seed=time. Another option could be to add an entirely new flag, > something like --seed-time. I can send a message to blink-infra about it. I be a bit surprised if anyone relied on --order=random actually producing the same "random" order every time, but I could be wrong. I'd rather change the default behavior than add yet another option if we can get away with it, but let's find out.
On 2016/10/11 at 00:32:39, dpranke wrote: > On 2016/10/11 00:30:03, chromium-reviews wrote: > > Just discussed with Quinten - we were talking about adding a special value, > > e.g. --seed=time. Another option could be to add an entirely new flag, > > something like --seed-time. I can send a message to blink-infra about it. > > I be a bit surprised if anyone relied on --order=random actually > producing the same "random" order every time, but I could be wrong. I'd > rather change the default behavior than add yet another option if we can get > away with it, but let's find out. That sounds good to me. I think tansell@ may also prefer changing the default behavior for --order=random with no --seed given to be be seeded based on current time.
On 2016/10/11 at 21:33:47, qyearsley wrote: > On 2016/10/11 at 00:32:39, dpranke wrote: > > On 2016/10/11 00:30:03, chromium-reviews wrote: > > > Just discussed with Quinten - we were talking about adding a special value, > > > e.g. --seed=time. Another option could be to add an entirely new flag, > > > something like --seed-time. I can send a message to blink-infra about it. > > > > I be a bit surprised if anyone relied on --order=random actually > > producing the same "random" order every time, but I could be wrong. I'd > > rather change the default behavior than add yet another option if we can get > > away with it, but let's find out. > > That sounds good to me. I think tansell@ may also prefer changing the default behavior for --order=random with no --seed given to be be seeded based on current time. Update on this CL: we're putting the FYI bot on hold until we make good progress toward getting random order runs green locally.
https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right): https://codereview.chromium.org/2384243005/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:204: '--seed=%d' % time.time(), On 2016/10/11 at 00:21:58, jeffcarp wrote: > On 2016/10/10 at 23:30:22, Dirk Pranke wrote: > > I believe this seed will change only when the buildbot master is restarted, which I'd guess is not your intent (i.e., it won't be very random). > > Got it. In that case it sounds like we'll need to add some logic to run-webkit-tests? I removed the --seed flag in anticipation of https://codereview.chromium.org/2453513002
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2384243005/#ps80001 (title: "Remove --seed since it now defaults to unixtime")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2384243005/#ps100001 (title: "Train new expectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL - https://codereview.chromium.org/2404573002 BUG=652357 ========== to ========== Add initial CL for RandomOrder chromium.fyi bot This is a set of 2 CLs: 1. build CL (this one) - https://codereview.chromium.org/2384243005/ 2. src CL - https://codereview.chromium.org/2404573002 BUG=652357 Committed: https://chromium.googlesource.com/chromium/tools/build/+/fc1d3b8658c112563b0c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/tools/build/+/fc1d3b8658c112563b0c...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2456053008/ by jeffcarp@chromium.org. The reason for reverting is: Missed adding the new flags to the bottom of the file as well. http://crbug.com/660503. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
