Repository navigation
Harden pairwise baselines and split verdicts - #26
Merged
Merged
Conversation
The clean run exposed three benchmark-harness issues rather than setup-java findings. First, GitHub concurrency groups only keep one running and one pending run. The three baseline runs for a workflow shared one fixed group, so dispatching v5.6.0, v5.2.0 and v4.9.1 together cancelled the middle pending comparison. Include the comparison identity in the group so valid concurrent dispatches are not dropped by the platform. Second, restore-based pairwise workflows require both refs to compute the same cache key for the same tree. v4.9.1 and main do not: the candidate arm missed a cache entry seeded by v4.9.1, so Action overhead correctly flagged the run as suspect. Remove v4.9.1 from Action overhead and Transfer overlap, keep it in Cache save where every slot writes a fresh key anyway, and mark v4.9.1 as measured-but-not-ranked in the sweep. The benchmark limitation is tracked in #25. Third, Action overhead printed Holm-adjusted p-values in its split table but kept the unadjusted verdict. Recompute the split verdict with the adjusted p-value so the table cannot say "improvement" next to an adjusted p-value above 0.05. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens the benchmark harness to avoid misleading pairwise comparisons and inconsistent statistical reporting, focusing on workflow concurrency behavior, baseline eligibility, and verdict computation.
Changes:
- Make workflow
concurrency.groupkeys include comparison identifiers to prevent unintended cancellation when dispatching multiple baseline runs. - Mark
v4.9.1as non-comparable for restore-based ranking and remove it from restore-based pairwise workflow baseline options (while keeping it for cache-save and sweep reporting). - Fix action-overhead split verdicts to classify using Holm-adjusted p-values, and update/add tests and README accordingly.
Show a summary per file
| File | Description |
|---|---|
| scripts/report.test.mjs | Updates report tests to reflect v4.9.1 no longer being ranked/comparable. |
| scripts/report.mjs | Marks v4.9.1 as non-comparable with an explanatory reason. |
| scripts/report-action-overhead.test.mjs | Adds coverage ensuring split verdicts use Holm-adjusted p-values for classification. |
| scripts/report-action-overhead.mjs | Computes displayed verdicts for split rows using adjusted p-values via classify(). |
| scripts/cache-save-slot.test.mjs | Test formatting adjustments (no functional change). |
| README.md | Updates baseline guidance and ranking explanation to reflect v4.9.1 limitations and workflow baseline availability. |
| .github/workflows/transfer-overlap.yml | Removes v4.9.1 from restore-based baselines; updates concurrency group key. |
| .github/workflows/cache-value.yml | Updates concurrency group key to include setup-java ref. |
| .github/workflows/cache-save.yml | Keeps v4.9.1 baseline; updates concurrency group key to include comparison identity. |
| .github/workflows/action-overhead.yml | Removes v4.9.1 from restore-based baselines; updates concurrency group key. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Lite
| # fixed group silently cancelled the middle baseline when the three comparison | ||
| # runs were dispatched together, so the comparison identity has to be part of | ||
| # the group. | ||
| group: action-overhead-${{ inputs.baseline-ref }}-${{ inputs.candidate-ref }} |
| # fixed group silently cancelled the middle baseline when the three comparison | ||
| # runs were dispatched together, so the comparison identity has to be part of | ||
| # the group. | ||
| group: transfer-overlap-${{ inputs.baseline-ref }}-${{ inputs.candidate-ref }} |
|
|
||
| concurrency: | ||
| group: cache-value | ||
| group: cache-value-${{ inputs.setup-java-ref }} |
| # fixed group silently cancelled the middle baseline when the three comparison | ||
| # runs were dispatched together, so the comparison identity has to be part of | ||
| # the group. | ||
| group: cache-save-${{ inputs.baseline-ref }}-${{ inputs.candidate-ref }}-${{ inputs.fixture-mib }} |
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 clean run exposed three benchmark-harness issues rather than setup-java findings.
v5.6.0,v5.2.0andv4.9.1together cancelled the middle pending comparison. The comparison identity is now part of the group.maindo not: the candidate arm missed a cache entry seeded by v4.9.1, so Action overhead correctly flagged the run as suspect. v4.9.1 remains in Cache save, where every slot writes a fresh key anyway, and is measured-but-not-ranked in the sweep. Tracked in Benchmark note: setup-java main and v4.9.1 compute different Maven cache keys for the same tree #25.improvementnext to an adjusted p-value above 0.05. Split verdicts are now classified with the adjusted p-value.Validation: YAML parse, 66 tests, embedded-node check, and shellcheck all pass.