Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3001873002:
[Telemetry] Add CanRunOnPlatform to story_runner and benchmark class. (Closed)
|
Created:
3 years, 4 months ago by rnephew (Reviews Here) Modified:
3 years, 3 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Add CanRunOnPlatform to story_runner and benchmark class.
We use use decorators.enabled and decorators.disabled to convey if
the platform was intended on certain platforms. (mobile only
benchmarks as an example). This adds a variable to benchmarks,
SUPPORTED_PLATFORMS, that defaults to ALL. Benchmark owners
can override this declaration in their own benchmarks to enable
the benchmark only on certain platforms. This effort will reuse
the work from StoryExpectations and will use TestConditions to
mark which platforms we wish to support.
BUG=chromium:713222
Review-Url: https://codereview.chromium.org/3001873002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/10b772bb112d592fd2f393af628c2195bb93e0fe
Patch Set 1 #Patch Set 2 : unit tests #
Total comments: 2
Patch Set 3 : Make Method Private #
Total comments: 6
Patch Set 4 : Charlies comments #
Messages
Total messages: 26 (11 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
Description was changed from ========== [Telemetry] Add CanRunOnPlatform to story_runner and benchmark class. We use use decorators.enabled and decorators.disabled to convey if the platform was intended on certain platforms. (mobile only benchmarks as an example). This adds a variable to benchmarks, SUPPORTED_PLATFORMS, that defaults to ALL. Benchmark owners can override this declaration in their own benchmarks to enable the benchmark only on certain platforms. This effort will reuse the work from StoryExpectations and will use TestConditions to mark which platforms we wish to support. BUG=chromium:713222 ========== to ========== [Telemetry] Add CanRunOnPlatform to story_runner and benchmark class. We use use decorators.enabled and decorators.disabled to convey if the platform was intended on certain platforms. (mobile only benchmarks as an example). This adds a variable to benchmarks, SUPPORTED_PLATFORMS, that defaults to ALL. Benchmark owners can override this declaration in their own benchmarks to enable the benchmark only on certain platforms. This effort will reuse the work from StoryExpectations and will use TestConditions to mark which platforms we wish to support. BUG=chromium:713222 ==========
nednguyen@google.com changed reviewers: + ashleymarie@chromium.org
Seems fine to me. Though you may want to check with Ashley about what json test results supposes to contain if the benchmark is disabled. https://codereview.chromium.org/3001873002/diff/20001/telemetry/telemetry/ben... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/3001873002/diff/20001/telemetry/telemetry/ben... telemetry/telemetry/benchmark.py:86: def CanRunOnPlatform(self, platform, finder_options): If this is not supposed to be overridden, can you rename it with _CanRunPlatform?
On 2017/08/18 23:46:53, nednguyen wrote: > Seems fine to me. Though you may want to check with Ashley about what json test > results supposes to contain if the benchmark is disabled. This uses the current existing logic to skip tests, so my guess is nothing will need to be done on the return format front, with any luck anyway.
On 2017/08/21 00:08:24, rnephew (Reviews Here) wrote: > On 2017/08/18 23:46:53, nednguyen wrote: > > Seems fine to me. Though you may want to check with Ashley about what json > test > > results supposes to contain if the benchmark is disabled. > > > This uses the current existing logic to skip tests, so my guess is nothing will > need to be done on the return format front, with any luck anyway. Sounds right to me. It would be a good idea to verify locally by disabling the dummy stable benchmark, running testing/scripts/run_telemetry_benchmark_as_googletest.py tools/perf/run_benchmark dummy_benchmark.stable_benchmark_1 --isolated-script-test-output=test_output.json --browser=system, and looking at what's in test_output.json to make sure it's reasonable
On 2017/08/21 12:28:24, ashleymarie1 wrote:
> On 2017/08/21 00:08:24, rnephew (Reviews Here) wrote:
> > On 2017/08/18 23:46:53, nednguyen wrote:
> > > Seems fine to me. Though you may want to check with Ashley about what json
> > test
> > > results supposes to contain if the benchmark is disabled.
> >
> >
> > This uses the current existing logic to skip tests, so my guess is nothing
> will
> > need to be done on the return format front, with any luck anyway.
>
> Sounds right to me. It would be a good idea to verify locally by disabling the
> dummy stable benchmark, running
> testing/scripts/run_telemetry_benchmark_as_googletest.py
> tools/perf/run_benchmark dummy_benchmark.stable_benchmark_1
> --isolated-script-test-output=test_output.json --browser=system, and looking
at
> what's in test_output.json to make sure it's reasonable
Output is:
{
"enabled": false,
"benchmark_name": "system_health.common_desktop"
}
for --output-format=chartjson
That looks reasonable to me, if its missing anything let me know.
https://codereview.chromium.org/3001873002/diff/20001/telemetry/telemetry/ben... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/3001873002/diff/20001/telemetry/telemetry/ben... telemetry/telemetry/benchmark.py:86: def CanRunOnPlatform(self, platform, finder_options): On 2017/08/18 23:46:53, nednguyen wrote: > If this is not supposed to be overridden, can you rename it with > _CanRunPlatform? Done.
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/08/21 16:26:58, rnephew (Reviews Here) wrote:
> On 2017/08/21 12:28:24, ashleymarie1 wrote:
> > On 2017/08/21 00:08:24, rnephew (Reviews Here) wrote:
> > > On 2017/08/18 23:46:53, nednguyen wrote:
> > > > Seems fine to me. Though you may want to check with Ashley about what
json
> > > test
> > > > results supposes to contain if the benchmark is disabled.
> > >
> > >
> > > This uses the current existing logic to skip tests, so my guess is nothing
> > will
> > > need to be done on the return format front, with any luck anyway.
> >
> > Sounds right to me. It would be a good idea to verify locally by disabling
the
> > dummy stable benchmark, running
> > testing/scripts/run_telemetry_benchmark_as_googletest.py
> > tools/perf/run_benchmark dummy_benchmark.stable_benchmark_1
> > --isolated-script-test-output=test_output.json --browser=system, and looking
> at
> > what's in test_output.json to make sure it's reasonable
>
> Output is:
> {
> "enabled": false,
> "benchmark_name": "system_health.common_desktop"
> }
>
> for --output-format=chartjson
>
> That looks reasonable to me, if its missing anything let me know.
That's the chartjson output right? Can you post the results from
--output-format=json-test-results?
On 2017/08/21 17:57:31, ashleymarie1 wrote:
> On 2017/08/21 16:26:58, rnephew (Reviews Here) wrote:
> > On 2017/08/21 12:28:24, ashleymarie1 wrote:
> > > On 2017/08/21 00:08:24, rnephew (Reviews Here) wrote:
> > > > On 2017/08/18 23:46:53, nednguyen wrote:
> > > > > Seems fine to me. Though you may want to check with Ashley about what
> json
> > > > test
> > > > > results supposes to contain if the benchmark is disabled.
> > > >
> > > >
> > > > This uses the current existing logic to skip tests, so my guess is
nothing
> > > will
> > > > need to be done on the return format front, with any luck anyway.
> > >
> > > Sounds right to me. It would be a good idea to verify locally by disabling
> the
> > > dummy stable benchmark, running
> > > testing/scripts/run_telemetry_benchmark_as_googletest.py
> > > tools/perf/run_benchmark dummy_benchmark.stable_benchmark_1
> > > --isolated-script-test-output=test_output.json --browser=system, and
looking
> > at
> > > what's in test_output.json to make sure it's reasonable
> >
> > Output is:
> > {
> > "enabled": false,
> > "benchmark_name": "system_health.common_desktop"
> > }
> >
> > for --output-format=chartjson
> >
> > That looks reasonable to me, if its missing anything let me know.
>
> That's the chartjson output right? Can you post the results from
> --output-format=json-test-results?
{
"interrupted": false,
"num_failures_by_type": {},
"path_delimiter": "/",
"seconds_since_epoch": 1503338475.337273,
"tests": {},
"version": 3
}
My guess is the fact that tests is empty is indicative enough that nothing ran?
that looks right to me, lgtm but please wait for Charlie
On 2017/08/21 18:02:05, rnephew (Reviews Here) wrote:
> On 2017/08/21 17:57:31, ashleymarie1 wrote:
> > On 2017/08/21 16:26:58, rnephew (Reviews Here) wrote:
> > > On 2017/08/21 12:28:24, ashleymarie1 wrote:
> > > > On 2017/08/21 00:08:24, rnephew (Reviews Here) wrote:
> > > > > On 2017/08/18 23:46:53, nednguyen wrote:
> > > > > > Seems fine to me. Though you may want to check with Ashley about
what
> > json
> > > > > test
> > > > > > results supposes to contain if the benchmark is disabled.
> > > > >
> > > > >
> > > > > This uses the current existing logic to skip tests, so my guess is
> nothing
> > > > will
> > > > > need to be done on the return format front, with any luck anyway.
> > > >
> > > > Sounds right to me. It would be a good idea to verify locally by
disabling
> > the
> > > > dummy stable benchmark, running
> > > > testing/scripts/run_telemetry_benchmark_as_googletest.py
> > > > tools/perf/run_benchmark dummy_benchmark.stable_benchmark_1
> > > > --isolated-script-test-output=test_output.json --browser=system, and
> looking
> > > at
> > > > what's in test_output.json to make sure it's reasonable
> > >
> > > Output is:
> > > {
> > > "enabled": false,
> > > "benchmark_name": "system_health.common_desktop"
> > > }
> > >
> > > for --output-format=chartjson
> > >
> > > That looks reasonable to me, if its missing anything let me know.
> >
> > That's the chartjson output right? Can you post the results from
> > --output-format=json-test-results?
>
> {
> "interrupted": false,
> "num_failures_by_type": {},
> "path_delimiter": "/",
> "seconds_since_epoch": 1503338475.337273,
> "tests": {},
> "version": 3
> }
>
>
> My guess is the fact that tests is empty is indicative enough that nothing
ran?
Yep! That's what I expect it to be; lgtm
https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/ben... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/ben... telemetry/telemetry/benchmark.py:92: if p.ShouldDisable(platform, finder_options): I understand why this works the way that it does, and have no problem implementing it w/ this note in the short term, but it seems like we should file a bug to move to a more sensible world in the future. It seems part of the problem is that, like their private names, expectations.ALL or expectations.ALL_MAC aren't really expectations, but rather test conditions to determine whether a platform meets a given criteria. (I might prefer "platform condition" or "platform filter", given how overloaded the word test is already within Telemetry, though.) They only become expectations once we combine them with what we expect to happen on that platform, e.g. "browse_google should be disabled on ALL_WIN", or "system_health should be enabled on ALL_MAC". However, currently baked into them is the idea that they only exist for disabling things - thus the "ShouldDisable()" method attached to them. It seems like it'd be more sensible to change that method to "MatchesPlatform()" or something. Then this would become: if p.MatchesPlatform(platform, finder_options): return True I don't think that we should do this in this CL, but it does seem like some technical debt that we should file a bug about cleaning up and note that here. https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:286: # when we have fully moved to CanRunonPlatform(). CanRunOnPlatform https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1269: # TODO(rnephew): Remove this test when we no longer use Won't "PermanentlyDisableBenchmark" just become "TemporarilyDisableBenchmark" (or possibly "DisableBenchmark") though?
lgtm after chatting w/ Randy offline and him saying that he'll add comments addressing my concerns before submitting the CL
https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/ben... File telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/ben... telemetry/telemetry/benchmark.py:92: if p.ShouldDisable(platform, finder_options): On 2017/08/22 17:22:18, charliea wrote: > I understand why this works the way that it does, and have no problem > implementing it w/ this note in the short term, but it seems like we should file > a bug to move to a more sensible world in the future. > > It seems part of the problem is that, like their private names, expectations.ALL > or expectations.ALL_MAC aren't really expectations, but rather test conditions > to determine whether a platform meets a given criteria. (I might prefer > "platform condition" or "platform filter", given how overloaded the word test is > already within Telemetry, though.) They only become expectations once we combine > them with what we expect to happen on that platform, e.g. "browse_google should > be disabled on ALL_WIN", or "system_health should be enabled on ALL_MAC". > > However, currently baked into them is the idea that they only exist for > disabling things - thus the "ShouldDisable()" method attached to them. It seems > like it'd be more sensible to change that method to "MatchesPlatform()" or > something. Then this would become: > > if p.MatchesPlatform(platform, finder_options): > return True > > I don't think that we should do this in this CL, but it does seem like some > technical debt that we should file a bug about cleaning up and note that here. The expectations file has the TestCondition class defined inside of it. Those could be moved to their own file. They were designed in a way that was StoryExpectations-centric. A good refactor in a follow up CL would help make things clearer, I agree. I will add a todo to the code. https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner.py:286: # when we have fully moved to CanRunonPlatform(). On 2017/08/22 17:22:18, charliea wrote: > CanRunOnPlatform Done. https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/story_runner_unittest.py (right): https://codereview.chromium.org/3001873002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/story_runner_unittest.py:1269: # TODO(rnephew): Remove this test when we no longer use On 2017/08/22 17:22:18, charliea wrote: > Won't "PermanentlyDisableBenchmark" just become "TemporarilyDisableBenchmark" > (or possibly "DisableBenchmark") though? Reworded to Refactor instead of Remove.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org, ashleymarie@chromium.org Link to the patchset: https://codereview.chromium.org/3001873002/#ps60001 (title: "Charlies comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1503423240620930,
"parent_rev": "db511c4fc59f736425e9fd553673b362a91e17f4", "commit_rev":
"10b772bb112d592fd2f393af628c2195bb93e0fe"}
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Add CanRunOnPlatform to story_runner and benchmark class. We use use decorators.enabled and decorators.disabled to convey if the platform was intended on certain platforms. (mobile only benchmarks as an example). This adds a variable to benchmarks, SUPPORTED_PLATFORMS, that defaults to ALL. Benchmark owners can override this declaration in their own benchmarks to enable the benchmark only on certain platforms. This effort will reuse the work from StoryExpectations and will use TestConditions to mark which platforms we wish to support. BUG=chromium:713222 ========== to ========== [Telemetry] Add CanRunOnPlatform to story_runner and benchmark class. We use use decorators.enabled and decorators.disabled to convey if the platform was intended on certain platforms. (mobile only benchmarks as an example). This adds a variable to benchmarks, SUPPORTED_PLATFORMS, that defaults to ALL. Benchmark owners can override this declaration in their own benchmarks to enable the benchmark only on certain platforms. This effort will reuse the work from StoryExpectations and will use TestConditions to mark which platforms we wish to support. BUG=chromium:713222 Review-Url: https://codereview.chromium.org/3001873002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
