|
|
DescriptionTelemetry: Fix exact_matches in telemetry.test_runner.
Previously, if exact_matches was True but there were no exact matches,
it would return fuzzy matches instead. This is not what the caller asked
for.
GPU test names are changed to match the buildbot config, since they were
previously relying on fuzzy matching.
SHERIFFS: May cause errors like:
No test named "some_test_name".
Available tests are:
...
If so, this CL can be safely reverted.
BUG=413334, 413442
Committed: https://crrev.com/566ed9f6e71cea313c88e8f259eca76581b72d67
Cr-Commit-Position: refs/heads/master@{#294757}
Committed: https://crrev.com/c258c3a307eed2114ffb95d06d15a5ec6e0f538e
Cr-Commit-Position: refs/heads/master@{#295888}
Patch Set 1 #Patch Set 2 : rename gpu tests to match buildbot config #
Total comments: 2
Patch Set 3 : move page set class references into CreatePageSet to make autogenerated name skip the dot #Patch Set 4 : move memory.py to memory_test.py to make the autogenerated name match the buildbot config #
Messages
Total messages: 47 (12 generated)
jbroman@chromium.org changed reviewers: + nduca@chromium.org, nednguyen@google.com
I've been having some trouble running Telemetry on the top_25 pageset, since after r293973 (https://codereview.chromium.org/551883003) I have to specify it by name and not filename, but the intersection of several command-line parsing bugs causes this to break the command-line parser (as it builds the parser with the SmoothnessTop25 benchmark as the test, even if I'm just using RasterizeAndRecordMicro on top_25, but then invokes the correct command). This resolves the symptoms I'm observing by ensuring that exact_matches means "exact matches", and thus prevents "smoothness.top_25" from being considered an exact match for "top_25".
nednguyen@google.com changed reviewers: + ariblue@google.com - nduca@chromium.org
nednguyen@google.com changed reviewers: + dtu@chromium.org
lgtm
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/560153004/1
The CQ bit was unchecked by jbroman@chromium.org
On 2014/09/11 20:59:02, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patchset/560153004/1 Oh cool, this somehow breaks gpu tests. Will investigate.
On 2014/09/11 21:57:26, jbroman wrote: > On 2014/09/11 20:59:02, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patchset/560153004/1 > > Oh cool, this somehow breaks gpu tests. Will investigate. Oh, the GPU bots are relying on fuzzy matching for the test names. The buildbot config specifies "pixel" instead of "pixel.pixel_tests", "gpu_process" instead of "gpu_process.gpu_process_tests", and "gpu_rasterization" instead of "gpu_rasterization.gpu_rasterization_tests". They're relying on this being fuzzy even exact_matches=True is specified. But since it's not smart enough to know the difference between the test name and the page set name, that means that some other change will be needed to make both this and my command work. Not sure whether I should change the buildbot configs to specify the full benchmark name here, or whether I need to do a more aggressive refactor to make Telemetry deal with this correctly.
nednguyen@google.com changed reviewers: + kbr@chromium.org
I prefer changing the buildbot configs to specify the full benchmark name here. I think supporting the fuzzy matching brings a lot of headaches in the future without adding any substantial value, so it would be great if we can kill it. Do you want to create a bug for this so it's convenient to track the effort in case the changes are revert because some other bots relied on fuzzy matching?
On 2014/09/11 22:27:52, nednguyen wrote: > I prefer changing the buildbot configs to specify the full > benchmark name here. > > I think supporting the fuzzy matching brings a lot of headaches in the future > without adding any substantial value, so it would be great if we can kill it. Do > you want to create a bug for this so it's convenient to track the effort in case > the changes are revert because some other bots relied on fuzzy matching? On bug 413442, kbr@ suggested that he would prefer changing the names of these benchmarks to match the buildbot config, rather than the reverse. I've modified this CL to do that by overriding the Name class method. This CL now does that as well, this seems to satisfy the GPU bots. Personally I think *all* tests should be explicitly named (PEP20 says "explicit is better than implicit", after all), but that would be part of a larger refactor. Since this CL now modifies content/test/gpu/, it also requires L G T M from kbr@.
On 2014/09/12 14:11:25, jbroman wrote: > On 2014/09/11 22:27:52, nednguyen wrote: > > I prefer changing the buildbot configs to specify the full > > benchmark name here. > > > > I think supporting the fuzzy matching brings a lot of headaches in the future > > without adding any substantial value, so it would be great if we can kill it. > Do > > you want to create a bug for this so it's convenient to track the effort in > case > > the changes are revert because some other bots relied on fuzzy matching? > > On bug 413442, kbr@ suggested that he would prefer changing the names of these > benchmarks to match the buildbot config, rather than the reverse. I've modified > this CL to do that by overriding the Name class method. > > This CL now does that as well, this seems to satisfy the GPU bots. Personally I > think *all* tests should be explicitly named (PEP20 says "explicit is better > than implicit", after all), but that would be part of a larger refactor. > > Since this CL now modifies content/test/gpu/, it also requires L G T M from > kbr@. Note that we also have the perfbot that only run the benchmarks after you submitted. I suspect that some may rely on the fuzzy matching. But we don't have a way to run the trybot with same config as perfbot, so you may want to write in your description s.t like "FEEL FREE TO REVERT THIS IF BOT RUNS FAIL WITH MESSAGE LOG "No test named "....""
nednguyen@google.com changed reviewers: + tonyg@chromium.org
On 2014/09/12 15:11:09, nednguyen wrote: > On 2014/09/12 14:11:25, jbroman wrote: > > On 2014/09/11 22:27:52, nednguyen wrote: > > > I prefer changing the buildbot configs to specify the full > > > benchmark name here. > > > > > > I think supporting the fuzzy matching brings a lot of headaches in the > future > > > without adding any substantial value, so it would be great if we can kill > it. > > Do > > > you want to create a bug for this so it's convenient to track the effort in > > case > > > the changes are revert because some other bots relied on fuzzy matching? > > > > On bug 413442, kbr@ suggested that he would prefer changing the names of these > > benchmarks to match the buildbot config, rather than the reverse. I've > modified > > this CL to do that by overriding the Name class method. > > > > This CL now does that as well, this seems to satisfy the GPU bots. Personally > I > > think *all* tests should be explicitly named (PEP20 says "explicit is better > > than implicit", after all), but that would be part of a larger refactor. > > > > Since this CL now modifies content/test/gpu/, it also requires L G T M from > > kbr@. > > Note that we also have the perfbot that only run the benchmarks after you > submitted. I suspect that some may rely on the fuzzy matching. But we don't have > a way to run the trybot with same config as perfbot, so you may want to write in > your description s.t like "FEEL FREE TO REVERT THIS IF BOT RUNS FAIL WITH > MESSAGE LOG "No test named "...."" Done.
On 2014/09/12 15:14:37, jbroman wrote: > On 2014/09/12 15:11:09, nednguyen wrote: > > On 2014/09/12 14:11:25, jbroman wrote: > > > On 2014/09/11 22:27:52, nednguyen wrote: > > > > I prefer changing the buildbot configs to specify the full > > > > benchmark name here. > > > > > > > > I think supporting the fuzzy matching brings a lot of headaches in the > > future > > > > without adding any substantial value, so it would be great if we can kill > > it. > > > Do > > > > you want to create a bug for this so it's convenient to track the effort > in > > > case > > > > the changes are revert because some other bots relied on fuzzy matching? > > > > > > On bug 413442, kbr@ suggested that he would prefer changing the names of > these > > > benchmarks to match the buildbot config, rather than the reverse. I've > > modified > > > this CL to do that by overriding the Name class method. > > > > > > This CL now does that as well, this seems to satisfy the GPU bots. > Personally > > I > > > think *all* tests should be explicitly named (PEP20 says "explicit is better > > > than implicit", after all), but that would be part of a larger refactor. > > > > > > Since this CL now modifies content/test/gpu/, it also requires L G T M from > > > kbr@. > > > > Note that we also have the perfbot that only run the benchmarks after you > > submitted. I suspect that some may rely on the fuzzy matching. But we don't > have > > a way to run the trybot with same config as perfbot, so you may want to write > in > > your description s.t like "FEEL FREE TO REVERT THIS IF BOT RUNS FAIL WITH > > MESSAGE LOG "No test named "...."" > > Done. ping kbr@?
I'd appreciate it if you could take an alternative approach. LGTM to unblock your work. Next week I'll be on business travel and slow to review. https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... File content/test/gpu/gpu_tests/gpu_process.py (right): https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... content/test/gpu/gpu_tests/gpu_process.py:39: page_set = page_sets.GpuProcessTestsPageSet Could you please instead just delete the page_set class variables from these four tests and instantiate them directly in CreatePageSet? I tried this and it seemed to solve the problem without adding four new Name methods.
https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... File content/test/gpu/gpu_tests/gpu_process.py (right): https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... content/test/gpu/gpu_tests/gpu_process.py:39: page_set = page_sets.GpuProcessTestsPageSet On 2014/09/13 05:57:12, Ken Russell wrote: > Could you please instead just delete the page_set class variables from these > four tests and instantiate them directly in CreatePageSet? I tried this and it > seemed to solve the problem without adding four new Name methods. FYI, this will also require changing the name of the memory test from "Memory" to "MemoryTest" so that it's magically generated name becomes "memory_test". The other three classes (Pixel, GpuProcess, and GpuRasterization) should have names that match the buildbot configs autogenerated.
On 2014/09/13 18:57:34, jbroman wrote: > https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... > File content/test/gpu/gpu_tests/gpu_process.py (right): > > https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... > content/test/gpu/gpu_tests/gpu_process.py:39: page_set = > page_sets.GpuProcessTestsPageSet > On 2014/09/13 05:57:12, Ken Russell wrote: > > Could you please instead just delete the page_set class variables from these > > four tests and instantiate them directly in CreatePageSet? I tried this and it > > seemed to solve the problem without adding four new Name methods. > > FYI, this will also require changing the name of the memory test from "Memory" > to "MemoryTest" so that it's magically generated name becomes "memory_test". > > The other three classes (Pixel, GpuProcess, and GpuRasterization) should have > names that match the buildbot configs autogenerated. Whoops, it's the module name, not the class name, that's used.
On 2014/09/13 19:24:48, jbroman wrote: > On 2014/09/13 18:57:34, jbroman wrote: > > > https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... > > File content/test/gpu/gpu_tests/gpu_process.py (right): > > > > > https://codereview.chromium.org/560153004/diff/20001/content/test/gpu/gpu_tes... > > content/test/gpu/gpu_tests/gpu_process.py:39: page_set = > > page_sets.GpuProcessTestsPageSet > > On 2014/09/13 05:57:12, Ken Russell wrote: > > > Could you please instead just delete the page_set class variables from these > > > four tests and instantiate them directly in CreatePageSet? I tried this and > it > > > seemed to solve the problem without adding four new Name methods. > > > > FYI, this will also require changing the name of the memory test from "Memory" > > to "MemoryTest" so that it's magically generated name becomes "memory_test". > > > > The other three classes (Pixel, GpuProcess, and GpuRasterization) should have > > names that match the buildbot configs autogenerated. > > Whoops, it's the module name, not the class name, that's used. Thanks for cleaning this up. LGTM again.
The CQ bit was checked by jbroman@chromium.org
The CQ bit was unchecked by jbroman@chromium.org
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/560153004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 3fbb31c269107128c4d2e3b20311a3978a6c0059
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/566ed9f6e71cea313c88e8f259eca76581b72d67 Cr-Commit-Position: refs/heads/master@{#294757}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/555703005/ by dsinclair@chromium.org. The reason for reverting is: Can't find the webrtc tests. http://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf%20%28....
On 2014/09/15 13:27:50, dsinclair wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/555703005/ by mailto:dsinclair@chromium.org. > > The reason for reverting is: Can't find the webrtc tests. > > http://build.chromium.org/p/chromium.perf/builders/Win%207%20x64%20Perf%20%28.... nednguyen: any preference for how I deal with the webrtc test? At the moment, there is a measurement called "web_rtc" and a benchmark called "webrtc.webrtc_cases" (because measurement names are generated by un-camelcasing the class name, and measurement names are derived from the module name). It is currently referring to the latter as "webrtc", which happens to work because of the way the fuzzy matching works. Applying the same fix as for the GPU tests would leave us with a measurement called "web_rtc" and corresponding benchmark called "webrtc", which seems even more confusing to me, though it will work.
On 2014/09/15 13:38:44, jbroman wrote: > because measurement names are generated by un-camelcasing > the class name, and measurement names are derived from the module name err, *benchmark* names are derived from the module name
Sorry, this is my fault. I chose this naming scheme to be consistent with how build steps on the waterfall are named. I think now that this restriction is artificial. I think our end-state should be module.ClassName, the same way unit tests are named. This is fairly explicit and readily understood, with no camel-casing magic. Unfortunately, when we first moved to our current naming system, we learned that a big rename is a lengthy and painful data migration for the perf dashboard. So there hasn't been a strong enough forcing function to get over that hump. I also agree that we should move away from fuzzy name usage on the bots and then kill it. For webrtc, we can land a correction to the test name in the build scripts, if you're willing to wait a bit.
On 2014/09/15 21:22:03, dtu wrote: > Sorry, this is my fault. I chose this naming scheme to be consistent with how > build steps on the waterfall are named. > I think now that this restriction is artificial. I think our end-state should be > module.ClassName, the same way unit tests are named. This is fairly explicit and > readily understood, with no camel-casing magic. Unfortunately, when we first > moved to our current naming system, we learned that a big rename is a lengthy > and painful data migration for the perf dashboard. So there hasn't been a strong > enough forcing function to get over that hump. > > I also agree that we should move away from fuzzy name usage on the bots and then > kill it. > > For webrtc, we can land a correction to the test name in the build scripts, if > you're willing to wait a bit. I'm not sure what's involved in perfbot build script changes. It's not super-urgent for me personally, but I would like to get a fix for this landed this week.
On 2014/09/16 14:51:24, jbroman wrote: > > I'm not sure what's involved in perfbot build script changes. See https://code.google.com/p/chromium/issues/detail?id=413442#c8 for a pointer to the top-level script.
On 2014/09/16 14:54:11, Ken Russell wrote: > On 2014/09/16 14:51:24, jbroman wrote: > > > > I'm not sure what's involved in perfbot build script changes. > > See https://code.google.com/p/chromium/issues/detail?id=413442#c8 for a pointer > to the top-level script. Sure, but: - it looks like it has to be changed at least in master.cfg and chromium_factory.py, not sure if there are other places (as it's hard to code search for "webrtc" without getting tons of results) - do these names end up getting processed elsewhere by some other tool? will some graph go haywire if I change this string? - what's the process for getting this changed? do masters need restarting? who does that and when? I've never touched these configs before, so I'm quite out of my element doing this myself.
In light of the discussion about changing these names again soon, because I'm not confident that I can reliably reconfigure the perf bots the first time, and because we already have several such aliases... I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' in run_benchmark. This should keep the perf bots happy, and still lets us fix the fuzzy-vs-exact matching bug. WDYT?
On 2014/09/18 15:56:38, jbroman wrote: > In light of the discussion about changing these names again soon, because I'm > not confident that I can reliably reconfigure the perf bots the first time, and > because we already have several such aliases... > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' in > run_benchmark. This should keep the perf bots happy, and still lets us fix the > fuzzy-vs-exact matching bug. > > WDYT? I would want we to kill those test_aliases, so I prefer the option of modifying bot config. If you want to fix the issue of not able to run some measurement locally, I can help fixing test runner so it distinguishes benchmark vs measurement based on the number of arguments.
On 2014/09/18 15:56:38, jbroman wrote: > In light of the discussion about changing these names again soon, because I'm > not confident that I can reliably reconfigure the perf bots the first time, and > because we already have several such aliases... > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' in > run_benchmark. This should keep the perf bots happy, and still lets us fix the > fuzzy-vs-exact matching bug. > > WDYT? Adding the alias seems fine to me.
On 2014/09/18 16:05:29, Ken Russell wrote: > On 2014/09/18 15:56:38, jbroman wrote: > > In light of the discussion about changing these names again soon, because I'm > > not confident that I can reliably reconfigure the perf bots the first time, > and > > because we already have several such aliases... > > > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' in > > run_benchmark. This should keep the perf bots happy, and still lets us fix the > > fuzzy-vs-exact matching bug. > > > > WDYT? > > Adding the alias seems fine to me. Actually, looks like there's a master restart tonight to move over to recipes. https://code.google.com/p/chromium/issues/detail?id=388885 After the restart, all the bots will discover and use the full canonical names (not aliases, not hard-coded names) You can land this change as-is. lgtm Yes, we do need to migrate the perf dashboard data over, but that's not that urgent - mostly for historical data.
On 2014/09/19 21:33:26, dtu wrote: > On 2014/09/18 16:05:29, Ken Russell wrote: > > On 2014/09/18 15:56:38, jbroman wrote: > > > In light of the discussion about changing these names again soon, because > I'm > > > not confident that I can reliably reconfigure the perf bots the first time, > > and > > > because we already have several such aliases... > > > > > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' in > > > run_benchmark. This should keep the perf bots happy, and still lets us fix > the > > > fuzzy-vs-exact matching bug. > > > > > > WDYT? > > > > Adding the alias seems fine to me. > > Actually, looks like there's a master restart tonight to move over to recipes. > https://code.google.com/p/chromium/issues/detail?id=388885 > > After the restart, all the bots will discover and use the full canonical names > (not aliases, not hard-coded names) This mean we should be able to kill the aliasing feature after the restart, is that right? > You can land this change as-is. lgtm > > Yes, we do need to migrate the perf dashboard data over, but that's not that > urgent - mostly for historical data.
On 2014/09/19 21:45:38, nednguyen wrote: > On 2014/09/19 21:33:26, dtu wrote: > > On 2014/09/18 16:05:29, Ken Russell wrote: > > > On 2014/09/18 15:56:38, jbroman wrote: > > > > In light of the discussion about changing these names again soon, because > > I'm > > > > not confident that I can reliably reconfigure the perf bots the first > time, > > > and > > > > because we already have several such aliases... > > > > > > > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' > in > > > > run_benchmark. This should keep the perf bots happy, and still lets us fix > > the > > > > fuzzy-vs-exact matching bug. > > > > > > > > WDYT? > > > > > > Adding the alias seems fine to me. > > > > Actually, looks like there's a master restart tonight to move over to recipes. > > https://code.google.com/p/chromium/issues/detail?id=388885 > > > > After the restart, all the bots will discover and use the full canonical names > > (not aliases, not hard-coded names) > This mean we should be able to kill the aliasing feature after the restart, is > that right? Yes! :) We can also remove fuzzy matching. > > > You can land this change as-is. lgtm > > > > Yes, we do need to migrate the perf dashboard data over, but that's not that > > urgent - mostly for historical data.
On 2014/09/19 22:28:22, dtu wrote: > On 2014/09/19 21:45:38, nednguyen wrote: > > On 2014/09/19 21:33:26, dtu wrote: > > > On 2014/09/18 16:05:29, Ken Russell wrote: > > > > On 2014/09/18 15:56:38, jbroman wrote: > > > > > In light of the discussion about changing these names again soon, > because > > > I'm > > > > > not confident that I can reliably reconfigure the perf bots the first > > time, > > > > and > > > > > because we already have several such aliases... > > > > > > > > > > I've added an explicit alias that maps 'webrtc' to 'webrtc.webrtc_cases' > > in > > > > > run_benchmark. This should keep the perf bots happy, and still lets us > fix > > > the > > > > > fuzzy-vs-exact matching bug. > > > > > > > > > > WDYT? > > > > > > > > Adding the alias seems fine to me. > > > > > > Actually, looks like there's a master restart tonight to move over to > recipes. > > > https://code.google.com/p/chromium/issues/detail?id=388885 > > > > > > After the restart, all the bots will discover and use the full canonical > names > > > (not aliases, not hard-coded names) > > This mean we should be able to kill the aliasing feature after the restart, is > > that right? > > Yes! :) We can also remove fuzzy matching. > > > > > > You can land this change as-is. lgtm > > > > > > Yes, we do need to migrate the perf dashboard data over, but that's not that > > > urgent - mostly for historical data. Alright, going to throw the last patchset on this issue back in the CQ and cross my fingers.
On 2014/09/20 20:22:14, jbroman wrote: > On 2014/09/19 22:28:22, dtu wrote: > > On 2014/09/19 21:45:38, nednguyen wrote: > > > On 2014/09/19 21:33:26, dtu wrote: > > > > On 2014/09/18 16:05:29, Ken Russell wrote: > > > > > On 2014/09/18 15:56:38, jbroman wrote: > > > > > > In light of the discussion about changing these names again soon, > > because > > > > I'm > > > > > > not confident that I can reliably reconfigure the perf bots the first > > > time, > > > > > and > > > > > > because we already have several such aliases... > > > > > > > > > > > > I've added an explicit alias that maps 'webrtc' to > 'webrtc.webrtc_cases' > > > in > > > > > > run_benchmark. This should keep the perf bots happy, and still lets us > > fix > > > > the > > > > > > fuzzy-vs-exact matching bug. > > > > > > > > > > > > WDYT? > > > > > > > > > > Adding the alias seems fine to me. > > > > > > > > Actually, looks like there's a master restart tonight to move over to > > recipes. > > > > https://code.google.com/p/chromium/issues/detail?id=388885 > > > > > > > > After the restart, all the bots will discover and use the full canonical > > names > > > > (not aliases, not hard-coded names) > > > This mean we should be able to kill the aliasing feature after the restart, > is > > > that right? > > > > Yes! :) We can also remove fuzzy matching. > > > > > > > > > You can land this change as-is. lgtm > > > > > > > > Yes, we do need to migrate the perf dashboard data over, but that's not > that > > > > urgent - mostly for historical data. > > Alright, going to throw the last patchset on this issue back in the CQ and cross > my fingers. by which I mean patch set 4, since reviewers were not happy with a webrtc alias
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by jbroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/560153004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 01072be991414b6b0f812381e5bb315cc64d9940
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c258c3a307eed2114ffb95d06d15a5ec6e0f538e Cr-Commit-Position: refs/heads/master@{#295888} |