Skip to content

Add the metric in question to markdown table #1398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

rylev
Copy link
Member

@rylev rylev commented Aug 12, 2022

As was brought up in Zulip, it would be helpful to specify which metric is being summarized in the markdown summary tables.

This looks at the first result in either the primary or secondary results for the metric name. We may want to consider some way of modeling that summaries should never have mixed metric kinds.

@rylev rylev requested a review from Kobzol August 12, 2022 13:17
@rylev rylev marked this pull request as ready for review August 12, 2022 13:17
@Kobzol
Copy link
Contributor

Kobzol commented Aug 12, 2022

I think that it's a bit extraneous for the PR tables, as there it can be clearly seen what is the metric? Could we add it only to the triage reports?

Could you please post an example of a table into a comment here so that we can better imagine how it looks? Thanks!

@rylev
Copy link
Member Author

rylev commented Aug 12, 2022

I pushed a change to only display the metric in the triage report. The GitHub comments remain unchanged.

The table will look like this when the metric type is included (note: the only change is the upper left cell):

(instructions:u) mean[^1] max count[^2]
Regressions ❌
(primary)
150.0% 200.0% 2
Regressions ❌
(secondary)
100.0% 100.0% 1
Improvements ✅
(primary)
-62.5% -75.0% 2
Improvements ✅
(secondary)
-66.7% -66.7% 1
All ❌✅ (primary) 43.8% 200.0% 4

When the metric type is not included it will look just like before.

@nnethercote
Copy link
Contributor

Is it reasonable to show the metric in the GitHub comments too? There are already quite a few different code paths for different cases, I'd suggest avoiding adding more.

@klensy
Copy link
Contributor

klensy commented Aug 14, 2022

So, it's basically my whining at #1385 (comment).

@nnethercote
Copy link
Contributor

I think adding the metric to the table is fine, I'm just suggesting it always be there.

@rylev
Copy link
Member Author

rylev commented Aug 15, 2022

Is it reasonable to show the metric in the GitHub comments too? There are already quite a few different code paths for different cases, I'd suggest avoiding adding more.

I think it's certainly reasonable, but it doesn't really simplify anything. We already differentiate the GitHub comments and triage flow. Unifying whether the metric is shown doesn't get rid of that. It does remove one function argument, but it doesn't really simplify the code for determining the metric name as we still have to deal with the case where no test case result is present.

I think we should make the decision on whether having that info is useful in the GitHub comments or not.

@nnethercote
Copy link
Contributor

I won't block this, feel free to go ahead.

@rylev rylev requested a review from Kobzol August 15, 2022 11:08
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the call sites and the bools are currently only used to basically distinguish triage from GH comments (true/false and false/true). So we could just pass an enum that distinguishes these two situations, instead of the lower-level bool attributes. But I wouldn't do that now, only if the code gets even more complicated later, then it would be nice to be refactored.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants