Judge the benchmark ratchet against a window of main runs - #306
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Reviewer's GuideIntroduce a rolling window of main-branch benchmark samples with noise-aware thresholds for the Rust performance ratchet, and update the CI workflow plus helpers so every completed main run records, publishes, and later reuses that window rather than a single, pass-filtered baseline sample. Sequence diagram for recording and using the rolling benchmark baseline windowsequenceDiagram
actor Developer
participant GitHubActions
participant fetch_main_benchmark_baseline_py as fetch_main_benchmark_baseline.py
participant ratchet_rust_performance_py as ratchet_rust_performance.py
participant update_baseline_history_py as update_baseline_history.py
Developer->>GitHubActions: push main
GitHubActions->>fetch_main_benchmark_baseline_py: run with --run-status completed
fetch_main_benchmark_baseline_py-->>GitHubActions: download main baseline artifact
GitHubActions->>ratchet_rust_performance_py: compare_rust_regressions
activate ratchet_rust_performance_py
ratchet_rust_performance_py->>ratchet_rust_performance_py: load_history(baseline_history)
ratchet_rust_performance_py->>ratchet_rust_performance_py: _baseline_window(baseline, candidate, history)
ratchet_rust_performance_py->>ratchet_rust_performance_py: compare_scenario with median_ratio and noise_tolerance
ratchet_rust_performance_py-->>GitHubActions: ratchet-report.json
deactivate ratchet_rust_performance_py
GitHubActions->>update_baseline_history_py: record this run's sample
activate update_baseline_history_py
update_baseline_history_py->>update_baseline_history_py: load_history(--history)
update_baseline_history_py->>update_baseline_history_py: run_ratios(candidate)
update_baseline_history_py->>update_baseline_history_py: BaselineHistory.appended(sample, window_size)
update_baseline_history_py-->>GitHubActions: main-baseline-history.json
deactivate update_baseline_history_py
GitHubActions->>GitHubActions: upload benchmark-ratchet-main-baseline artifact
Developer->>GitHubActions: open pull_request
GitHubActions->>fetch_main_benchmark_baseline_py: fetch window artifact
GitHubActions->>ratchet_rust_performance_py: compare_rust_regressions using window
ratchet_rust_performance_py-->>GitHubActions: pass/fail based on max(flat_threshold, noise_tolerance)
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77d131a91b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return BenchmarkRunPayload( | ||
| plan=load_plan(args.baseline_plan), | ||
| throughput=load_throughput(args.baseline_throughput), |
There was a problem hiding this comment.
Defer loading the fallback until the history needs it
When the inspected benchmark-ratchet job fails before run_smoke_benchmarks creates the candidate files, the new recording step deliberately carries the existing history forward, and the upload step can consequently publish a valid history-only baseline artifact. On the next run, that history could perform the comparison, but _load_baseline eagerly opens the absent main-plan.json and main-throughput.json first, causing exit code 2 before _baseline_window examines the history; subsequent PR benchmark checks can therefore remain broken after a transient main-branch benchmark failure. Load these fallback files only when no compatible history exists, or carry the previous fallback files into every published artifact.
Useful? React with 👍 / 👎.
|
CI status on this branch:
Until then the coverage is local: unit and property tests over the window and the threshold, behavioural scenarios stating the incident's own numbers, and contract tests over the four workflow declarations that would silently reinstate the bias. All six Worth noting for whoever merges: the first |
A main run measured medium-single-nocb at 0.760 against a baseline of 1.013, passed as a 25% improvement — improvements are not gated — and published that outlier as the next baseline. Every pull request after it compared an ordinary 1.110 against 0.760 and reported a 46% regression, three re-runs included, while the other three scenarios agreed with the baseline to within 0.14. Two properties combined to produce that, and neither fix works alone. One sample was the whole estimate, so its noise was the bar's noise. The bar is now the median of the last seven main-branch samples, which one outlier cannot move, and the flat 30% threshold is joined by a noise band measured from those same samples: a candidate must exceed both. The band comes from the median absolute deviation rather than a standard deviation, which the outlier being tolerated would inflate in proportion to itself, and it is capped, because a band that widened without limit would disable the ratchet silently rather than say the benchmark can no longer measure what it gates on. The sample was also only published if its own run passed, and a run passes when it is faster than the bar — so a fast anomaly was always accepted while the ordinary measurements that would have corrected it were rejected. Every completed main run now records its sample and publishes the artifact, and the fetch asks for completed runs rather than successful ones, since publishing from a failing run achieves nothing while the fetch still filters them out. A window emptied by a benchmark-profile change, or absent on a first run, falls back to the single-sample baseline this replaces, and the report says how many samples backed the verdict. Splits the ratio extraction into benchmarks/ratchet_ratios.py to keep both halves under the file-size cap. Refs #219. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both helpers grew a fifth parameter as the cases accumulated: `_sample` kept a scenario argument no caller ever set, and `_record` threaded the plan and throughput paths separately when every caller had them as a pair. Drop the first and give the second a Candidate for the pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The window makes the bar robust to one noisy run. It cannot make the candidate robust: a pull request is measured once, on whichever runner CI gave it, and until now the only recourse for an unlucky measurement was a human pressing re-run. So a flagged scenario is now measured again in the same job, compared again against the same window, and the two verdicts are intersected: it fails only if it regressed both times. A flake has to land on the same scenario twice to survive, which turns a one-in-N false failure into roughly one-in-N-squared. The second benchmark is spent only on a run that was about to fail, so ordinary runs cost what they did before. Three asymmetries are deliberate. Confirmation may only turn a failure into a pass, since a second chance to fail would double the false failures it exists to halve. A confirmation that could not compare at all leaves the first verdict standing, because the primary comparison succeeded on the same inputs and so an unusable retry is a fault in the retry rather than evidence about the candidate. And the re-measurement writes under its own prefix, so the sample recorded into the window stays the primary measurement — recording the confirming run instead would add a second sample only for the merges that were about to fail, which is a verdict-dependent bias in the samples all over again. An exit code of 2 is malformed input rather than a regression, and fails on the spot rather than spending a benchmark to reread a broken file. Refs #219. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
da653f4 to
99dcdbc
Compare
Summary
Fixes the false-positive mode recorded in #219, in the specific form seen on
#289: a
mainrun measuredmedium-single-nocbat 0.760 against a baseline of1.013, passed as a 25% improvement — improvements are not gated — and published
that outlier as the next baseline. Every pull request after it compared an
ordinary 1.110 against 0.760 and reported a 46% regression, three re-runs
included, while the other three scenarios agreed with the baseline to within
0.14.
Two independent properties produced that, and neither fix works without the
other.
One sample was the whole estimate, so its noise was the bar's noise. The
bar is now the median of the last seven
mainsamples, held inmain-baseline-history.jsoninside the existing baseline artefact. The flat30% threshold is joined by a noise band measured from those same samples, and a
candidate must exceed both: the flat threshold alone cannot tell a slow runner
from a slow change, and the observed spread alone would let a consistent
regression through. Spread comes from the median absolute deviation rather than
a standard deviation, which the outlier being tolerated would inflate in
proportion to itself. The band is capped at 1.00 — past that the benchmark is
saying it cannot measure what it gates on, and an uncapped band would disable
the ratchet silently instead.
The candidate is measured once, on whichever runner CI gave it. The window
makes the bar robust to one noisy run; it cannot make the candidate robust, and
the only recourse for an unlucky measurement was a human pressing re-run. A
flagged scenario is now measured again in the same job and fails only if it
regressed both times, so a flake has to land on the same scenario twice to
survive. The second benchmark is spent only on a run that was about to fail.
Confirmation may only turn a failure into a pass; a confirmation that could not
compare leaves the first verdict standing; and the re-measurement writes under
its own prefix so the sample recorded into the window stays the primary
measurement.
The sample was only published if its own run passed, and a run passes when
it is faster than the bar. A fast anomaly was therefore always accepted while
the ordinary measurements that would have corrected it were the ones rejected —
a bar biased towards the low tail of the noise, and sticky once there. Every
completed
mainrun now records its sample and publishes the artefact(
!cancelled(), notsuccess(), and notalways(): an interrupted run recordsnothing), and the fetch asks GitHub for
completedruns rather thansuccessful ones, since publishing from a failing run achieves nothing whilethe fetch still filters it out.
Review walkthrough
benchmarks/ratchet_history.py— the window, the statistics, and the reasonsfor each choice.
benchmarks/ratchet_rust_performance.py—_baseline_windowprefers thewindow and falls back to the single-sample baseline;
_compare_scenarioapplies
max(flat, noise)..github/workflows/ci.yml— the recording step, the publication conditions,--run-status completed, and the candidate now staged before thecomparison can fail the step.
benchmarks/confirm_regression.py— intersects the two verdicts, and thethree asymmetries that keep the re-measurement from being a second chance to
fail.
benchmarks/update_baseline_history.py— always writes an output file, so arun that measured nothing carries the window forward instead of publishing an
artefact the next run reads as a fresh start.
Compatibility
benchmark_profile_versionorworker_iterationschange, absent on a first run, or lost to an expired artefact, falls back to
the single-sample baseline this replaces.
ratchet-report.jsonreportsbaseline_sample_countso a surprising verdict can be read against theevidence behind it.
window fills over the following merges.
Validation
make check-fmt,make lint,make typecheck,make test,make markdownlint,make nixie: all green.actionlint .github/workflows/ci.yml: clean.cuprum/unittests/test_benchmark_ratchet_history.py(the incident'snumbers as a regression test, plus Hypothesis properties — a candidate at the
median always passes, observed noise may only widen the bar, one arbitrary
sample cannot outvote a majority),
test_benchmark_baseline_history_cli.py,test_benchmark_baseline_publication_contract.py(the workflow declarationsthat would silently reinstate the bias), and
tests/features/benchmark_ratchet_noise.feature.Notes
ci.yml.overhead-bound. It makes the gate honest about its own noise; it cannot
create signal that is not there.
References
Summary by Sourcery
Make the benchmark ratchet resilient to noisy measurements by using a rolling main-branch baseline, recording every completed sample, and confirming failures before rejecting changes.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: