Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446853002:
Write results2.html for TBMv2 benchmarks if --output-format=html. (Closed)
|
Created:
4 years, 1 month ago by benjhayden Modified:
4 years, 1 month ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionWrite results2.html for TBMv2 benchmarks if --output-format=html.
BUG=catapult:#2860
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/bea54e53a27aa0697b4bf7bbe31f89366906b0d6
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : refactor HtmlOutputFormatter #
Total comments: 1
Patch Set 4 : remove self._existing_results, GetCombinedResults, GetResults #Patch Set 5 : restore GetResults for test #Patch Set 6 : restore GetCombinedResults for test #Messages
Total messages: 28 (11 generated)
Description was changed from ========== Write results2.html for TBMv2 benchmarks. BUG=catapult:#2860 ========== to ========== Write results2.html for TBMv2 benchmarks if --output-format=html. BUG=catapult:#2860 ==========
benjhayden@chromium.org changed reviewers: + eakuefner@chromium.org, nednguyen@google.com
PTAL :-)
On 2016/10/24 23:21:43, benjhayden wrote: > PTAL :-) why not do this at html_output_formatter, i.e: if page_test_results.value_set, then delegate to html2_output_formatter?
On 2016/10/24 at 23:26:31, nednguyen wrote: > On 2016/10/24 23:21:43, benjhayden wrote: > > PTAL :-) > > why not do this at html_output_formatter, i.e: if page_test_results.value_set, then delegate to html2_output_formatter? HtmlOutputFormatter only gets the PageTestResults when Format() is called, but by then it's already tried to read existing results using results(1).html logic. It seems like it should use results2.html logic to read existing results, right?
On 2016/10/24 at 23:29:14, benjhayden wrote: > On 2016/10/24 at 23:26:31, nednguyen wrote: > > On 2016/10/24 23:21:43, benjhayden wrote: > > > PTAL :-) > > > > why not do this at html_output_formatter, i.e: if page_test_results.value_set, then delegate to html2_output_formatter? > > HtmlOutputFormatter only gets the PageTestResults when Format() is called, but by then it's already tried to read existing results using results(1).html logic. It seems like it should use results2.html logic to read existing results, right? I have to go catch my bus but it looks like there's a bug: AttributeError: type object 'TimelineBasedMeasurement' has no attribute 'GetTimelineBasedMetrics'
On 2016/10/24 23:34:10, benjhayden wrote: > On 2016/10/24 at 23:29:14, benjhayden wrote: > > On 2016/10/24 at 23:26:31, nednguyen wrote: > > > On 2016/10/24 23:21:43, benjhayden wrote: > > > > PTAL :-) > > > > > > why not do this at html_output_formatter, i.e: if > page_test_results.value_set, then delegate to html2_output_formatter? > > > > HtmlOutputFormatter only gets the PageTestResults when Format() is called, but > by then it's already tried to read existing results using results(1).html logic. > It seems like it should use results2.html logic to read existing results, right? > > I have to go catch my bus but it looks like there's a bug: > AttributeError: type object 'TimelineBasedMeasurement' has no attribute > 'GetTimelineBasedMetrics' I see, this is fine. lgtm
Thanks! I assume it's ok if Benchmark creates tbm Options earlier in the process? Name preferences for Benchmark.is_tbm or Benchmark.tbm_options? I ran one story from PCV2 with --output-format=html and it wrote a results2.html named "results.html" as expected.
On 2016/10/25 03:34:09, benjhayden wrote: > Thanks! > > I assume it's ok if Benchmark creates tbm Options earlier in the process? What do you mean by earlier? Would this prevent us from having benchmar creates TBM Options through commandline flag in the future? > > Name preferences for Benchmark.is_tbm or Benchmark.tbm_options? > > I ran one story from PCV2 with --output-format=html and it wrote a results2.html > named "results.html" as expected.
On 2016/10/25 at 10:09:37, nednguyen wrote: > On 2016/10/25 03:34:09, benjhayden wrote: > > Thanks! > > > > I assume it's ok if Benchmark creates tbm Options earlier in the process? > > What do you mean by earlier? Would this prevent us from having benchmar creates TBM Options through commandline flag in the future? > > > > Name preferences for Benchmark.is_tbm or Benchmark.tbm_options? > > > > I ran one story from PCV2 with --output-format=html and it wrote a results2.html > > named "results.html" as expected. I don't think that Benchmark is the right place to surface this stuff. It seems like we're working around the fundamental problem that the output formatters read existing results when they are constructed. I agree with Ned that PageTestResults is the right place to query for this information, and I think we can do it by making the OFs read the existing results instead in the Format method.
On 2016/10/25 at 17:23:30, eakuefner wrote: > On 2016/10/25 at 10:09:37, nednguyen wrote: > > On 2016/10/25 03:34:09, benjhayden wrote: > > > Thanks! > > > > > > I assume it's ok if Benchmark creates tbm Options earlier in the process? > > > > What do you mean by earlier? Would this prevent us from having benchmar creates TBM Options through commandline flag in the future? > > > > > > Name preferences for Benchmark.is_tbm or Benchmark.tbm_options? > > > > > > I ran one story from PCV2 with --output-format=html and it wrote a results2.html > > > named "results.html" as expected. > > I don't think that Benchmark is the right place to surface this stuff. It seems like we're working around the fundamental problem that the output formatters read existing results when they are constructed. I agree with Ned that PageTestResults is the right place to query for this information, and I think we can do it by making the OFs read the existing results instead in the Format method. Thanks! Done, PTAL :-)
lgtm
lgtm https://codereview.chromium.org/2446853002/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/results/html_output_formatter.py (right): https://codereview.chromium.org/2446853002/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/results/html_output_formatter.py:49: self._existing_results = [] nits: there is no need to keep this self._existing_results as member
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2446853002/#ps60001 (title: "remove self._existing_results, GetCombinedResults, GetResults")
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
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2446853002/#ps80001 (title: "restore GetResults for test")
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
Try jobs failed on following builders: Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2446853002/#ps100001 (title: "restore GetCombinedResults for test")
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 ========== Write results2.html for TBMv2 benchmarks if --output-format=html. BUG=catapult:#2860 ========== to ========== Write results2.html for TBMv2 benchmarks if --output-format=html. BUG=catapult:#2860 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
