Repository navigation
ci: stop running the micro-benchmark comparison on every PR - #1490
Merged
Merged
Conversation
A GitHub-hosted runner is not a quiet machine. It is shared, its neighbours are invisible to us, and we cannot verify it was idle for the window we timed. Under that noise this job posted "Benchmark Regression Detected" on PRs that had changed nothing relevant. That is worse than publishing nothing: a red flag that fires on noise trains readers to ignore it, so the header stops carrying information and a real regression posted the same way would be dismissed too. Timing base-then-PR inside one job and reporting % change does cancel some runner-to-runner variation, but it cannot cancel within-job contention -- the two halves still run at different moments against different neighbours. The thresholds were calibrated against ~27% worst-case measured noise, and regressions worth catching are often smaller than that, so the signal sat below the noise floor rather than above it. Nothing is lost by making this manual: the workflow never gated anything (its own header says so). The real regression gates are the throughput floors and dispatch-reachability tests in crates/onnx-genai-bench/tests/profile_native.rs, which run on real hardware and are unaffected by this change. The job is kept and is now workflow_dispatch-only, taking a PR number as input; since a manual run checks out the default branch rather than the PR, it now resolves the head sha and base ref via `gh pr view` and fetches the head explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1490 +/- ##
==========================================
+ Coverage 82.10% 82.64% +0.54%
==========================================
Files 12 12
Lines 5471 5475 +4
Branches 5471 5475 +4
==========================================
+ Hits 4492 4525 +33
+ Misses 780 757 -23
+ Partials 199 193 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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
Benchmarksworkflow posted a🔴 Benchmark Regression Detectedcomment on PRs that had changed nothing relevant. The cause is not the comparison logic — it is the machine.A GitHub-hosted runner is not a quiet machine. It is shared, its neighbours are invisible to us, and we cannot verify it was idle for the window we timed.
Why this is worse than publishing nothing. A red flag that fires on noise trains readers to ignore it. Once the header stops carrying information, a real regression posted the same way gets dismissed too — so the failure mode is not "a useless comment", it is "a gate nobody believes".
Why the base-vs-PR design did not rescue it. Timing base first and PR second in the same job cancels some runner-to-runner variation, but it cannot cancel within-job contention: the two halves still run at different moments against different neighbours. The thresholds were calibrated against ~27% worst-case measured noise, and regressions worth catching are often smaller than that — so the signal sat below the noise floor, not above it.
Nothing is lost. This workflow never gated anything; its own header says
Does NOT block CI. The real regression gates are the throughput floors and dispatch-reachability tests incrates/onnx-genai-bench/tests/profile_native.rs, which run on real hardware and are untouched here.What changed
pull_requesttrigger; the job is nowworkflow_dispatch-only, taking a PR number as input.gh pr viewand fetches the head explicitly.github.event.pull_request.*references (concurrency group, comment lookup, comment post) to use the input.The workflow was also disabled in the Actions UI (
gh workflow disable) so the noise stopped immediately rather than waiting on this merge.Verified
python -c "import yaml; yaml.safe_load(...)"→ parses; triggers are exactly['workflow_dispatch'].github.event.pull_requestreferences in the file.benchmark.ymlis the only workflow that runscargo bench/criterion— grep across.github/workflows/*.ymlconfirms no other PR-triggered benchmark job exists.Not verified: the manual
workflow_dispatchpath has not been executed end-to-end (it needs a real dispatch on a macOS runner). The PR-metadata resolution is a straightforward substitution, but it is untested — worth a trial dispatch before anyone relies on it.