Say when a run could not have found anything - #19
Merged
Conversation
The cache save helper declared `manifest` twice in one embedded script, so every slot died on a SyntaxError before it saved anything. Nothing local could have caught it: the code inside `<<'NODE'` is a string to prettier, `node --test` never imports it, and shellcheck sees an opaque heredoc. Rename the colliding binding, and add a check that extracts every embedded block and runs `node --check` over it, wired into `npm test` so the next one fails in a second rather than after a full benchmark run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
The first live run measured caching as 4.5x faster — 22.4s on Spring PetClinic — and reported it as inconclusive. The permutation test is a sign flip over n paired runners, so it has 2^n assignments and a smallest attainable p-value of 2^-n. One of six runners stalled and was discarded, leaving four, whose floor is 0.0625. No effect of any size could have cleared 0.05 in that run. Reporting that as inconclusive invites exactly the wrong reading, because the two cases call for opposite responses: inconclusive means the data did not show an effect, underpowered means the design could not have shown one. Classify it separately and say how many pairs would be needed. Raise the cache value default to ten runners so a couple of stalls cannot take it back under the floor. Intervals printed one decimal, which rendered a 13 ms difference as `0.0s (95% CI -0.0 to 0.0)` in the action overhead report — the scenario built specifically to resolve tens of milliseconds. Print three. Point the transfer overlap baseline at v5.6.0. The concurrent-restore change it exists to detect landed between v5.6.0 and main, and starting from v4.8.0 straddled several unrelated changes whose sum read as no effect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new “significance reachable / pairs needed” math and docs currently don’t match the repo’s two-sided permutation test implementation, which can misclassify 5-pair runs, and the new heredoc checker relies on a Node API (fs.promises.glob) not available on Node 20 LTS without pinning Node.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR refines the benchmark harness’ statistical reporting so it can distinguish “no detected effect” (inconclusive) from “the design could not have detected an effect” (underpowered), and improves report readability/robustness (more precise interval formatting and an added check for embedded Node heredocs).
Changes:
- Add an
underpoweredverdict path (with explanatory text) when a run cannot reach statistical significance due to too few paired runners. - Increase default interval formatting precision and remove duplicate interval estimate printing in the action overhead report.
- Add an
npm testcheck to syntax-validate JavaScript embedded inscripts/*.shheredocs; update workflow defaults and docs accordingly.
File summaries
| File | Description |
|---|---|
| scripts/stats.mjs | Adds underpowered classification helpers and changes default interval formatting precision. |
| scripts/stats.test.mjs | Updates/extends unit tests to cover underpowered classification and new interval formatting. |
| scripts/paired.mjs | Plumbs pair counts into classification and exposes reachability metadata in analyses. |
| scripts/report-action-overhead.mjs | Avoids duplicating interval estimate text in rendered markdown. |
| scripts/check-embedded-node.mjs | New helper to syntax-check Node code embedded in shell heredocs. |
| scripts/cache-save.sh | Renames variables inside embedded Node blocks to avoid redeclaration. |
| README.md | Documents the new underpowered concept and the embedded-heredoc syntax check. |
| package.json | Extends npm test to include the embedded-heredoc syntax check. |
| .github/workflows/transfer-overlap.yml | Updates baseline ref default to v5.6.0 to better isolate the intended change. |
| .github/workflows/cache-value.yml | Adjusts default runner count and documents the rationale. |
Review details
Suppressed comments (1)
scripts/check-embedded-node.mjs:50
- To avoid relying on fs.promises.glob (Node 22+), build the list of scripts/*.sh files via readdir(), which works across Node LTS releases.
const files = [];
for await (const file of glob("scripts/*.sh")) files.push(file);
files.sort();
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+172
to
+184
| // The sign-flip test has only 2^n distinct assignments for n paired | ||
| // observations, so its smallest attainable p-value is 2^-n no matter how large | ||
| // the effect is. At four pairs that floor is 0.0625, above the 0.05 the verdict | ||
| // requires — so a run with four usable runners cannot report a finding even for | ||
| // an effect it measured perfectly. That is a property of the design, not of the | ||
| // data, and it has to be said rather than dressed up as "collect more samples". | ||
| export function significanceReachable(pairCount, alpha = SIGNIFICANCE_LEVEL) { | ||
| return pairCount > 0 && Math.pow(2, -pairCount) <= alpha; | ||
| } | ||
|
|
||
| export function pairsNeededForSignificance(alpha = SIGNIFICANCE_LEVEL) { | ||
| return Math.ceil(Math.log2(1 / alpha)); | ||
| } |
Comment on lines
+163
to
+186
| // A four-runner design cannot clear 0.05 no matter how large the effect is, | ||
| // because the sign-flip test only has sixteen assignments to draw on. Reporting | ||
| // that as "inconclusive" invites someone to read the number anyway. | ||
| test("calls a design underpowered when significance is unreachable", () => { | ||
| assert.equal(significanceReachable(4), false); | ||
| assert.equal(significanceReachable(5), true); | ||
| assert.equal(pairsNeededForSignificance(), 5); | ||
|
|
||
| const decisive = { estimate: -22, low: -28, high: -18 }; | ||
| assert.equal( | ||
| classify(decisive, { pValue: 0.061, pairCount: 4 }), | ||
| "underpowered", | ||
| ); | ||
| assert.equal( | ||
| classify(decisive, { pValue: 0.01, pairCount: 10 }), | ||
| "improvement", | ||
| ); | ||
| }); | ||
|
|
||
| test("explains what an underpowered run can and cannot show", () => { | ||
| const described = describeVerdict("underpowered"); | ||
| assert.match(described, /at least 5/); | ||
| assert.match(described, /may be real/); | ||
| }); |
|
|
||
| **Stalled runners are discarded.** A slot can stall on the cache service for several seconds. Which arm the stall lands on is arbitrary, so that runner contributes an arbitrarily large difference and, with ten runners, one such slot moves the mean by more than any effect being measured. Runners whose _own arm disagrees with itself_ by more than a robust threshold are dropped. That decision is made purely on within-arm spread, which has the same distribution whether or not the arms differ, so unlike filtering on the arm difference it cannot bias the result. Every report lists what it discarded and why. | ||
|
|
||
| **A design that cannot reach significance says so.** The sign-flip test has only 2^n distinct assignments for n paired runners, so its smallest attainable p-value is 2^-n however large the effect is. At four usable runners that floor is 0.0625, above the 0.05 a verdict requires — the run cannot report a finding even for an effect it measured perfectly. Those runs are reported as `underpowered` rather than `inconclusive`, because the two call for opposite readings: `inconclusive` means the data did not show an effect, `underpowered` means the design could not have shown one. The first live run of **Cache value** measured caching as 4.5x faster, 22.4 s, and reported `inconclusive` on four surviving runners; that is what this exists to stop. |
Comment on lines
+15
to
+17
| # The sign-flip test needs at least five usable pairs to reach 0.05 at | ||
| # all, and this workflow discards stalled runners, so a nominal six can | ||
| # land on four and report nothing about a 4.5x effect. Ten leaves room. |
Comment on lines
+11
to
+17
| import { readFile, writeFile, unlink } from "node:fs/promises"; | ||
| import { execFile } from "node:child_process"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { promisify } from "node:util"; | ||
| import { glob } from "node:fs/promises"; | ||
| import { pathToFileURL } from "node:url"; |
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 first live run of the rescoped benchmarks measured Maven caching as 4.5x faster — 22.4 s on Spring PetClinic, and reported it as
inconclusive.That was the harness's fault, not the data's. The permutation test is a sign flip over n paired runners, so it has 2^n assignments and a smallest attainable p-value of 2^-n. One of six runners stalled and was discarded, leaving four, whose floor is 0.0625 — above the 0.05 a verdict requires. No effect of any size could have cleared it.
inconclusiveandunderpoweredcall for opposite responses — the first means the data did not show an effect, the second means the design could not have shown one — so they are now classified separately, and the verdict says how many pairs would be needed.Also in this PR:
0.0s (95% CI -0.0 to 0.0)— in the scenario built specifically to resolve tens of milliseconds.60 tests pass.