Fail the benchmark job when performance regresses - #73
Open
Kayvan-Zahiri wants to merge 2 commits into
Open
Conversation
The CI benchmark job saved results and uploaded them, but never compared them, so a slowdown could land without anything noticing. Adds a benchmark-compare target that runs the suite and exits non-zero when the median regresses past a threshold, and points the GPU job at it, with the previous baseline restored from the Actions cache. Caches from the default branch are readable by pull requests, so a PR is compared against the last run recorded on main. The target records a baseline instead of comparing when no saved run exists. That case needs handling: --benchmark-compare-fail without a loadable comparison is a usage error and exits 4, which would break the first run and every cache miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VVVbEALa5xcixrx3C3r5Xz
Code Review by Qodo
1. Later pull request commits evade checks
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
89
to
+90
| - name: Run CUDA benchmark | ||
| run: make benchmark | ||
| run: make benchmark-compare |
There was a problem hiding this comment.
1. Later pull request commits evade checks 🐞 Bug ≡ Correctness
The new make benchmark-compare step is hosted in a workflow whose pull_request.types omits synchronize. After a pull request is opened or marked ready, any regression added by a later push is never evaluated by this gate unless someone manually dispatches the workflow.
Agent Prompt
## Issue description
The benchmark regression gate does not run when new commits are pushed to an existing pull request because the workflow excludes the `synchronize` event.
## Issue Context
The comparison step therefore only evaluates the commit present when the pull request is opened, reopened, or marked ready, unless the workflow is manually dispatched.
## Fix Focus Areas
- .github/workflows/test_gpu.yml[7-10]
- .github/workflows/test_gpu.yml[89-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 0de9577 |
A pull request that saved its own benchmark into the restorable cache would become the baseline for the next run on that same ref, so a series of sub-threshold slowdowns could each pass while compounding past the threshold. Restore is now read-only and only a push to main saves. The plot and upload steps also ran on success only, so a regression skipped exactly the artifacts needed to diagnose it. They now run whenever results exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VVVbEALa5xcixrx3C3r5Xz
Author
|
Two of the three were real and are fixed in 921b655. Cache poisoning. Correct, and it would have made the gate useless over time. A pull request saved Lost diagnostics. Also correct: the plot and upload steps were success-only, so a regression skipped exactly the artifacts you would want to look at. They now run on The Still untested against real hardware: I have no GPU runner, so the 25% default is reasoned rather than measured. |
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
1 participant
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.
Closes #16.
The benchmark job saves results and uploads them as an artifact, but nothing compares them, so a slowdown can land without failing anything. This adds the comparison.
make benchmark-compareruns the suite and exits non-zero when the median regresses pastBENCHMARK_REGRESSION_THRESHOLD(defaultmedian:25%). The GPU job now calls it and restores the previous run from the Actions cache first. Caches from the default branch are readable by pull requests, so a PR is compared against the last baseline recorded onmain.Two things I checked rather than assumed:
--benchmark-compare-failwithout a loadable comparison is a usage error:ERROR: --benchmark-compare-fail requires valid --benchmark-compare, exit 4. That would break the first run and every cache miss, so the target records a baseline instead of comparing when.benchmarksis empty. Verified under/bin/sh, since recipes do not use my shell.Field 'median' has failed PercentageRegressionCheck: 213.38 > 25.0and exit 1.I could not run the GPU job itself, so the threshold is a starting point rather than a measured one. 25% is deliberately loose: the runner is shared, and a tighter bound would fail on neighbours rather than on the change. It is a make variable so you can tighten it once you see real numbers, and I am happy to change the default or the comparison field if you would rather gate on
min.