Benchmark workflow improvements#513
Conversation
Compare the least-performant base against the best-performing head using the confidence-interval bounds instead of the medians, so a regression is only flagged when the intervals do not overlap. Surface each sample's confidence-interval spread in the report so reviewers can see how noisy a benchmark is. Also fail 'benchcheck report' on genuine result-file read errors instead of silently returning empty content.
There was a problem hiding this comment.
Pull request overview
This PR improves the benchcheck benchmark workflow by making regression detection more variance-robust (using confidence-interval bounds to avoid flagging noisy overlaps) and by treating genuine result-file read failures in benchcheck report as fatal errors rather than silently swallowing them.
Changes:
- Update regression detection to compare base upper CI vs head lower CI and carry CI spread metadata into regression output.
- Make
benchcheck reportpropagate real read errors (while still treating missing optional files as non-errors). - Add targeted tests for the new file-reading behavior and the variance-robust regression logic.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/benchcheck/check.go | Switch regression comparison to CI-bound overlap logic and include CI spread strings in regression output. |
| cmd/benchcheck/report.go | Change helper readers to return errors and fail report on genuine read errors (while allowing missing files). |
| cmd/benchcheck/report_test.go | Add unit tests validating missing-vs-genuine read error behavior for report helpers. |
| cmd/benchcheck/main_test.go | Add regression tests covering overlapping vs separated distributions and ensuring spread strings are populated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot address PR feedback |
Done in the latest commit. Both issues addressed in
|
|
Do you have an example of this running, for broad context? |
| // A missing result file is expected (a job may not produce one); a genuine | ||
| // read error is not, and must fail the report rather than be swallowed. | ||
| if readErr != nil { |
There was a problem hiding this comment.
Higher-level question: is it really a good design to intentionally be missing files? Could the system be set up to always create the expected file set, e.g. create an empty failures.txt file if there are no failures?
From a human side, it's simpler IMO to always create the files: an intentionally written empty file is more convincing than a file not existing. Also, does this process delete an existing failures.txt file if there are no failures, or does it leave it alone? You could end up thinking that you have errors even if you don't in that case. Always writing the file avoids that question and potential quirk of using deletion APIs rather than file-write APIs in some cases.
There was a problem hiding this comment.
So in check.go it already always writes regressions.txt/failures.txt unconditionally, and the workflow pre-seeds status.json to benchmark_error:true. So empty files are the norm and nothing gets deleted. The only case where a file is genuinely absent is a job that crashes before check runs — which the pre-seeded status.json already reports as an error. The tolerant read just keeps report from failing on a partial/aborted artifact upload rather than being a design that relies on absence.
There was a problem hiding this comment.
The tolerant read just keeps report from failing on a partial/aborted artifact upload
Hmm, well, when would that happen? Why is it desirable to treat a partial/aborted artifact upload this way?
It's good that it doesn't rely on file absence, but that just means I'm not seeing a reason remaining to handle missing files in a special way rather than fail.
There was a problem hiding this comment.
(Commenting on this as part of a review to make it more obvious.)
There was a problem hiding this comment.
You're right — the partial-upload rationale doesn't really hold up. I guess the only genuine reason report tolerated a missing .txt was the case where the workflow pre-seeds status.json to benchmark_error:true, but check is what writes the other two, so if a job died before check ran they'd be absent while report still had to render the error comment.
I changed it so that with absence no longer a normal state, report now reads those two files strictly — a missing file is a hard error — via a new readTrimmedFile. The tolerant readTrimmedContentIfExists stays only for the optional -job-urls input, whose absence really does just mean "not provided".
Yes sorry forgot to link microsoft/go-crypto-winnative#187 |
dagood
left a comment
dagood
left a comment
There was a problem hiding this comment.
Nice, up to you if it makes sense to do anything now/later about the nonexistent file handling.
Rename readFileContent to readTrimmedContentIfExists and loadJobURLs to readJobURLsFileIfExists so the names convey that a missing file is tolerated. readJobURLsFileIfExists now reuses readTrimmedContentIfExists for the read/trim/not-exist handling.
dagood
added
the
release-on-merge
dagood
left a comment
dagood
left a comment
There was a problem hiding this comment.
Still looks fine to me, just not sure the missing file handling approach makes sense.
| // A missing result file is expected (a job may not produce one); a genuine | ||
| // read error is not, and must fail the report rather than be swallowed. | ||
| if readErr != nil { |
There was a problem hiding this comment.
(Commenting on this as part of a review to make it more obvious.)
The bench job now seeds empty failures.txt and regressions.txt alongside status.json, so every uploaded artifact has the full result set even if the job dies before benchcheck runs. report now reads those files strictly (a missing file is a hard error) via a new readTrimmedFile helper, while the optional job-URLs input keeps using the tolerant readTrimmedContentIfExists.
Compare the least-performant base against the best-performing head using the confidence-interval bounds instead of the medians, so a regression is only flagged when the intervals do not overlap. Surface each sample's confidence-interval spread in the report so reviewers can see how noisy a benchmark is.
Also fail 'benchcheck report' on genuine result-file read errors instead of silently returning empty content.