Put the benchmark speedup table on the run summary page - #1872
Open
sbryngelson wants to merge 2 commits into
Open
Put the benchmark speedup table on the run summary page#1872sbryngelson wants to merge 2 commits into
sbryngelson wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds a GitHub Actions step summary output for benchmark speedup results so they’re visible on the job summary page (instead of only in step logs).
Changes:
- Introduces a
_write_step_summary(...)helper to render the benchmark speedup table and sub-threshold warnings into$GITHUB_STEP_SUMMARY. - Collects per-row table data and warning strings during
bench_diffexecution and writes them after printing the Rich table. - Adds
MFC_BENCH_SUMMARY_LABELto the bench workflow step to label the matrix leg in the summary heading.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| toolchain/mfc/bench.py | Collects table rows/warnings and writes a Markdown summary to $GITHUB_STEP_SUMMARY. |
| .github/workflows/bench.yml | Sets an env var to label each matrix leg in the job summary output. |
Suppressed comments (1)
toolchain/mfc/bench.py:1
- The exec/grind thresholds are now duplicated across comparison logic and multiple human-readable strings (and use slightly different formatting:
0.9vs0.90). To avoid drift if thresholds change, define constants (e.g.,EXEC_SPEEDUP_MIN,GRIND_SPEEDUP_MIN) and use them both in the comparisons and in the warning/summary messages (formatting them consistently).
import dataclasses
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+225
to
+226
| with open(path, "a", encoding="utf-8") as f: | ||
| f.write("\n".join(lines)) |
| cons.unindent() | ||
|
|
||
|
|
||
| def _write_step_summary(lhs_path: str, rhs_path: str, rows: list, warnings: list): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The speedup table only exists in the step log today, so reading it means opening the right step of the right matrix leg. This also writes it to
$GITHUB_STEP_SUMMARY, which GitHub renders inline on the job page.Rendered:
Benchmark: phoenix gpu acc
Speedups from
master/bench-gpu-acc.yamltopr/bench-gpu-acc.yaml; greater than 1 is faster.5eq_rk3_weno3_hllcibmBelow threshold
5eq_rk3_weno3_hllcNotes on the shape of it:
table.add_rowis called, so the summary cannot drift from the table printed to stdout - there is one source for both.exec < 0.90,grind < 0.95) are collected and listed underneath. Those are the actionable part and are currently interleaved with everything else in the log.MFC_BENCH_SUMMARY_LABELcarries the matrix leg (cluster, device, interface) into the heading. Each leg writes its own summary, and the leg is not otherwise identifiable on the page.GITHUB_STEP_SUMMARYis unset and the function returns immediately, so local./mfc.sh bench_diffis unchanged.Testing
./mfc.sh precheckpasses 7/7. The writer was exercised directly with a stub row set and a warning, against a temp file standing in for$GITHUB_STEP_SUMMARY- output above is that run, pasted verbatim - and again with the variable unset to confirm it is a no-op locally.What I have not done is run it through a real benchmark job; that needs a cluster allocation and a master-vs-PR pair. The failure mode if something is off is a missing or malformed summary, not a failed benchmark, since the write happens after the table is printed and the exit status is unchanged.