You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The benchmark-ratchet CI job (.github/workflows/ci.yml) is prone to false-positive regression failures because the scenarios it measures are dominated by fixed per-run overhead, so the metric it ratchets on — the within-run rust_mean / python_mean ratio — has a poor signal-to-noise ratio and swings past the 30% threshold on runner-to-runner variance alone.
This surfaced on PR #158, where benchmark-ratchet was the only failing check (all other checks green) with:
Investigation showed the failure is not attributable to that PR's changes (or to any recent code change) — it is measurement noise. This issue captures the analysis and proposes making the ratchet robust.
Evidence that the failure is noise, not a real regression
1. The worker is overhead-bound. Reproducing the benchmark locally on the current tree (native extension built with stable, as CI does), the four ratchet scenarios are statistically indistinguishable and payload size barely matters:
rust-65536 396.8 ms ± 2.4 python-65536 398.0 ms ± 4.6
rust-1024 398.6 ms ± 2.7 python-1024 404.6 ms ± 3.7
→ rust/python ratio ≈ 1.00; 1024 vs 65536 payload differ by < 1%
Each pipeline_worker.py invocation carries ~400 ms of fixed cost (interpreter start, cuprum import, subprocess spawn for a 2-stage pipeline). The actual streaming throughput the ratchet cares about is a thin sliver on top, so the rust_mean/python_mean ratio is a ratio of two nearly-equal, overhead-dominated numbers.
2. The failing measurement is physically backwards. The failing CI run reported the rust stream backend as slower than the python backend at 65536 bytes (rust ≈ 1.44× / 1.68× slower than the fastest scenario; python ≈ 1.17× / 1.07×). A Rust splice being slower than the Python pump only makes sense as noise.
3. The baseline is current and comparable.benchmark-ratchetpassed on the most recent push-to-main (the #118 CQRS merge, 22380b7). Because that step passed, the (non-always()) "Upload main benchmark baseline artifact" step ran and refreshed the baseline. So the PR was compared against an up-to-date baseline built from the same code path — the delta is runner variance between the baseline measurement (one cloud runner) and the candidate measurement (a different runner a day later), not a code change.
4. The metric cancels backend-common changes by construction. The ratchet compares the within-run rust/python ratio specifically so absolute machine speed cancels — but that also means only a change that moves the rust path relative to the python path can move the metric. When both backends are overhead-bound and near-parity, the ratio is dominated by whatever per-run jitter the runner happens to exhibit.
Root cause
Benchmark-design signal-to-noise, not a defect in ratchet_rust_performance.py's math (regression_ratio = (candidate_ratio - baseline_ratio) / baseline_ratio is correct). The ratchet scenarios (--payload-bytes {1024,65536} --stages 2 --iterations 20) don't stream enough data, through enough stages, to lift the rust-vs-python signal above process-spawn/import overhead. A 30% threshold on such a metric is inside the noise band across cloud runners.
Recommendations
Immediate: re-run a failing benchmark-ratchet job; it should pass on a fresh runner. Treat a repeated failure across re-runs (same baseline) as real signal.
Make the ratchet robust (main-side): raise the signal above the overhead floor and/or widen the noise tolerance, e.g.:
Larger payloads and/or more pipeline stages so streaming dominates the wall-clock rather than spawn/import.
Higher iteration counts per worker (amortise fixed cost further) and more hyperfine runs for tighter means.
Consider a wider max_regression and/or requiring the regression to persist across repeated samples before failing, so a single noisy runner can't red a PR.
Optional CI hardening: the "Upload main benchmark baseline artifact" step is gated on the ratchet step passing (it is not if: always()). That is arguably correct (don't enshrine a regressed baseline), but it means a flaky failure on main would freeze the baseline until the next green push — worth noting when tuning thresholds.
Notes / non-causes ruled out
rust/rust-toolchain.toml (pins 1.85.0): does not affect the benchmark build. maturin develop --manifest-path rust/cuprum-rust/Cargo.toml runs from the repo root, where rustup resolves the toolchain from the CWD and its ancestors — rust/rust-toolchain.toml is a child directory, so it is not honoured. Verified: maturin from the repo root builds with stable on both baseline and candidate. The pin only applies to commands run insiderust/ (the UI/trybuild tests), which is its intended scope.
Summary
The
benchmark-ratchetCI job (.github/workflows/ci.yml) is prone to false-positive regression failures because the scenarios it measures are dominated by fixed per-run overhead, so the metric it ratchets on — the within-runrust_mean / python_meanratio — has a poor signal-to-noise ratio and swings past the 30% threshold on runner-to-runner variance alone.This surfaced on PR #158, where
benchmark-ratchetwas the only failing check (all other checks green) with:Investigation showed the failure is not attributable to that PR's changes (or to any recent code change) — it is measurement noise. This issue captures the analysis and proposes making the ratchet robust.
Evidence that the failure is noise, not a real regression
1. The worker is overhead-bound. Reproducing the benchmark locally on the current tree (native extension built with
stable, as CI does), the four ratchet scenarios are statistically indistinguishable and payload size barely matters:Each
pipeline_worker.pyinvocation carries ~400 ms of fixed cost (interpreter start,cuprumimport, subprocess spawn for a 2-stage pipeline). The actual streaming throughput the ratchet cares about is a thin sliver on top, so therust_mean/python_meanratio is a ratio of two nearly-equal, overhead-dominated numbers.2. The failing measurement is physically backwards. The failing CI run reported the rust stream backend as slower than the python backend at 65536 bytes (rust ≈ 1.44× / 1.68× slower than the fastest scenario; python ≈ 1.17× / 1.07×). A Rust splice being slower than the Python pump only makes sense as noise.
3. The baseline is current and comparable.
benchmark-ratchetpassed on the most recent push-to-main(the #118 CQRS merge,22380b7). Because that step passed, the (non-always()) "Upload main benchmark baseline artifact" step ran and refreshed the baseline. So the PR was compared against an up-to-date baseline built from the same code path — the delta is runner variance between the baseline measurement (one cloud runner) and the candidate measurement (a different runner a day later), not a code change.4. The metric cancels backend-common changes by construction. The ratchet compares the within-run rust/python ratio specifically so absolute machine speed cancels — but that also means only a change that moves the rust path relative to the python path can move the metric. When both backends are overhead-bound and near-parity, the ratio is dominated by whatever per-run jitter the runner happens to exhibit.
Root cause
Benchmark-design signal-to-noise, not a defect in
ratchet_rust_performance.py's math (regression_ratio = (candidate_ratio - baseline_ratio) / baseline_ratiois correct). The ratchet scenarios (--payload-bytes {1024,65536} --stages 2 --iterations 20) don't stream enough data, through enough stages, to lift the rust-vs-python signal above process-spawn/import overhead. A 30% threshold on such a metric is inside the noise band across cloud runners.Recommendations
benchmark-ratchetjob; it should pass on a fresh runner. Treat a repeated failure across re-runs (same baseline) as real signal.max_regressionand/or requiring the regression to persist across repeated samples before failing, so a single noisy runner can't red a PR.if: always()). That is arguably correct (don't enshrine a regressed baseline), but it means a flaky failure onmainwould freeze the baseline until the next green push — worth noting when tuning thresholds.Notes / non-causes ruled out
rust/rust-toolchain.toml(pins 1.85.0): does not affect the benchmark build.maturin develop --manifest-path rust/cuprum-rust/Cargo.tomlruns from the repo root, where rustup resolves the toolchain from the CWD and its ancestors —rust/rust-toolchain.tomlis a child directory, so it is not honoured. Verified: maturin from the repo root builds withstableon both baseline and candidate. The pin only applies to commands run insiderust/(the UI/trybuild tests), which is its intended scope.SafeCmd.run's single-command path (cuprum/_subprocess_execution.py,_subprocess_stdin.py,_subprocess_timeout.py,sh.py). The benchmark exercisessh.Pipeline.run_sync→_pipeline_spawn→_streams, which does not route through those modules. No streaming-path source (_streams.py,_streams_rs.py,_pipeline_streams.py) changed.References
.github/workflows/ci.yml(benchmark-ratchet)benchmarks/pipeline_worker.py,benchmarks/ratchet_rust_performance.py,benchmarks/ratchet_types.py