From ee50a237ed46ad8ae59a25519d92c631bfb2c7f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 21:06:41 +0000 Subject: [PATCH] Make the benchmark CI guard robust against run-to-run noise The benchmark job compared a single master run against a single PR run with --benchmark-compare-fail=mean:5%, a gate that sits below the noise floor of GitHub-hosted runners (observ measured 10-30% swings between runs of identical code). Replace it with the design observ landed in fork-tongue/observ#181 and #187: * Disable GC during timed rounds (--benchmark-disable-gc) and add the median column, which is what the guard gates on. * Pin PYTHONHASHSEED=0 in CI so dict/set timings are comparable between processes. * Run master and PR benchmarks interleaved, 3 runs each (A/B/A/B/A/B), instead of once each. * Replace --benchmark-compare-fail with benchmarks/compare_runs.py, which only fails a benchmark when the fastest PR run is more than 25% slower than the slowest master run (per-benchmark medians), so a lucky or unlucky process on either side can't produce a false positive. Results are also written to the GitHub job summary. The 25% threshold is deliberately a tripwire for gross accidental regressions, not a precision instrument; smaller deltas should be measured with repeated local runs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_016Mu9vEBwU4fQLi8ZgkgdS2 --- .github/workflows/benchmark.yml | 53 ++++++---- benchmarks/compare_runs.py | 175 ++++++++++++++++++++++++++++++++ pyproject.toml | 6 +- 3 files changed, 213 insertions(+), 21 deletions(-) create mode 100644 benchmarks/compare_runs.py diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index a5380e0..9cffb20 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -26,26 +26,39 @@ jobs: - name: Install dependencies run: uv sync --group observ - # On PRs: run benchmarks twice (PR code vs master code) and compare + # Single runs are too noisy to gate on (see benchmarks/compare_runs.py), + # so master and PR code are benchmarked interleaved, three runs each, + # with hash randomization pinned so dict/set timings are comparable + # between processes. The master runs are `|| true` so a baseline that + # can't run (e.g. benchmarks exercising new APIs) skips the compare + # instead of failing the job. - name: Run benchmarks + env: + PYTHONHASHSEED: "0" run: | - # Checkout master version of patchdiff directory git fetch origin master - git checkout origin/master -- patchdiff/ - - # Run benchmarks with master code as baseline - uv run --no-sync pytest benchmarks/benchmark.py \ - --benchmark-only \ - --benchmark-save=master \ - --benchmark-sort=mean || true - - # Restore PR code - git checkout HEAD -- patchdiff/ - - # Run benchmarks on PR code and compare - uv run --no-sync pytest benchmarks/benchmark.py \ - --benchmark-only \ - --benchmark-compare \ - --benchmark-compare-fail=mean:5% \ - --benchmark-save=branch \ - --benchmark-sort=mean + + for i in 1 2 3; do + git checkout origin/master -- patchdiff/ + uv run --no-sync pytest benchmarks/benchmark.py \ + --benchmark-only \ + --benchmark-save=master$i \ + --benchmark-sort=mean || true + + git checkout HEAD -- patchdiff/ + uv run --no-sync pytest benchmarks/benchmark.py \ + --benchmark-only \ + --benchmark-save=branch$i \ + --benchmark-sort=mean + done + + # The 25% threshold is a tripwire for gross accidental regressions, + # not a precision instrument: CI noise between runs of identical code + # reaches 10-30%. Smaller deltas should be measured with deliberate + # repeated local runs. + - name: Compare benchmarks + run: | + uv run --no-sync python benchmarks/compare_runs.py \ + --baseline '.benchmarks/*/*_master?.json' \ + --branch '.benchmarks/*/*_branch?.json' \ + --threshold 0.25 diff --git a/benchmarks/compare_runs.py b/benchmarks/compare_runs.py new file mode 100644 index 0000000..1f7808f --- /dev/null +++ b/benchmarks/compare_runs.py @@ -0,0 +1,175 @@ +""" +Compare two sets of pytest-benchmark runs (baseline vs branch) and fail +when a benchmark has regressed. + +Single benchmark runs are too noisy to gate on: process-level effects +(hash randomization, memory layout, CPU frequency, runner load) shift +individual timings by 20-40% between runs of identical code. Instead of +comparing one run against one run, this script expects several +interleaved runs per side and only reports a regression when the branch +is *consistently* slower than the baseline: + + fail if min(branch medians) > max(baseline medians) * (1 + threshold) + +i.e. the fastest branch run must be slower than the slowest baseline +run by more than the threshold. A whole-process outlier on either side +then can't produce a false positive, because the comparison always uses +the branch's luckiest run against the baseline's unluckiest run. + +Even that gate has a noise floor: on shared CI runners, whole runs of +identical code have been observed to differ by 10-30%. The threshold +therefore has to sit above that floor, which makes this a tripwire for +gross accidental regressions rather than a precision instrument -- +changes too small to trip it should be measured with deliberate +repeated local runs instead. + +Usage: + + python benchmarks/compare_runs.py \ + --baseline '.benchmarks/*/*_master?.json' \ + --branch '.benchmarks/*/*_branch?.json' \ + --threshold 0.25 +""" + +import argparse +import glob +import json +import os +import sys +from statistics import median + + +def load_runs(pattern): + """Load benchmark stats from every file matching the glob pattern. + + Returns a list of runs, each a dict mapping benchmark name to its + median timing in seconds. + """ + runs = [] + for path in sorted(glob.glob(pattern)): + with open(path) as fh: + data = json.load(fh) + runs.append({b["name"]: b["stats"]["median"] for b in data["benchmarks"]}) + return runs + + +def format_time(seconds): + for unit, factor in [("s", 1), ("ms", 1e3), ("us", 1e6), ("ns", 1e9)]: + if seconds * factor >= 1: + return f"{seconds * factor:,.1f}{unit}" + return f"{seconds * 1e9:.2f}ns" + + +def compare(baseline_runs, branch_runs, threshold): + """Compare runs and return (rows, failed_names).""" + baseline_names = set().union(*(r.keys() for r in baseline_runs)) + branch_names = set().union(*(r.keys() for r in branch_runs)) + + rows = [] + failed = [] + for name in sorted(branch_names): + if name not in baseline_names: + rows.append((name, None, None, None, "new")) + continue + base = [r[name] for r in baseline_runs if name in r] + branch = [r[name] for r in branch_runs if name in r] + # Estimated change, for reporting only: middle-of-the-road runs + # on both sides. + change = (median(branch) - median(base)) / median(base) + # Gate: the fastest branch run against the slowest baseline run. + excess = (min(branch) - max(base)) / max(base) + if excess > threshold: + verdict = "FAIL" + failed.append(name) + else: + verdict = "" + rows.append((name, base, branch, change, verdict)) + for name in sorted(baseline_names - branch_names): + rows.append((name, None, None, None, "removed")) + return rows, failed + + +def render_table(rows, markdown=False): + header = ["benchmark", "baseline medians", "branch medians", "change", ""] + body = [] + for name, base, branch, change, verdict in rows: + body.append( + [ + name, + " / ".join(format_time(t) for t in sorted(base)) if base else "-", + " / ".join(format_time(t) for t in sorted(branch)) if branch else "-", + f"{change:+.1%}" if change is not None else "-", + verdict, + ] + ) + if markdown: + lines = [ + "| " + " | ".join(header) + " |", + "|" + "|".join("---" for _ in header) + "|", + ] + lines.extend("| " + " | ".join(row) + " |" for row in body) + return "\n".join(lines) + widths = [max(len(row[i]) for row in [header, *body]) for i in range(len(header))] + lines = [ + " ".join(cell.ljust(width) for cell, width in zip(row, widths)).rstrip() + for row in [header, *body] + ] + lines.insert(1, "-" * max(len(line) for line in lines)) + return "\n".join(lines) + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--baseline", required=True, help="glob for baseline runs") + parser.add_argument("--branch", required=True, help="glob for branch runs") + parser.add_argument( + "--threshold", + type=float, + default=0.25, + help="max allowed consistent slowdown, as a fraction (default: 0.25)", + ) + args = parser.parse_args(argv) + + baseline_runs = load_runs(args.baseline) + branch_runs = load_runs(args.branch) + + if not branch_runs: + print(f"error: no branch runs match {args.branch!r}") + return 2 + if not baseline_runs: + # The baseline couldn't run at all (e.g. the branch's benchmarks + # exercise APIs that don't exist on the baseline yet); there is + # nothing to compare against, so pass. + print(f"warning: no baseline runs match {args.baseline!r}, skipping compare") + return 0 + + rows, failed = compare(baseline_runs, branch_runs, args.threshold) + print( + f"Comparing {len(branch_runs)} branch run(s) against " + f"{len(baseline_runs)} baseline run(s), threshold {args.threshold:.0%}:\n" + ) + print(render_table(rows)) + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if summary_path: + with open(summary_path, "a") as fh: + fh.write("## Benchmark comparison\n\n") + if failed: + fh.write(f"**{len(failed)} benchmark(s) regressed.**\n\n") + fh.write(render_table(rows, markdown=True)) + fh.write("\n") + + if failed: + print( + f"\n{len(failed)} benchmark(s) consistently slower than baseline " + f"by more than {args.threshold:.0%}:" + ) + for name in failed: + print(f" {name}") + return 1 + print("\nNo benchmark consistently regressed beyond the threshold.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/pyproject.toml b/pyproject.toml index 0eab9aa..c0e74cf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,10 +37,14 @@ extend-select = [ [tool.ruff.lint.per-file-ignores] "patchdiff/__init__.py" = ["F401"] +# command line tool that reports through print +"benchmarks/compare_runs.py" = ["T201"] [build-system] requires = ["hatchling"] build-backend = "hatchling.build" [tool.pytest.ini_options] -addopts = "--benchmark-columns='mean, stddev, rounds'" +# GC pauses during timed rounds are a major source of benchmark noise, +# so garbage collection is disabled while benchmarks are being timed. +addopts = "--benchmark-columns='median, mean, stddev, rounds' --benchmark-disable-gc"