diff --git a/.github/workflows/_checks.yml b/.github/workflows/_checks.yml index 536972f..12a3f71 100644 --- a/.github/workflows/_checks.yml +++ b/.github/workflows/_checks.yml @@ -25,20 +25,6 @@ jobs: - uses: astral-sh/setup-uv@v8.2.0 - run: just docs-build - bench: - runs-on: ubuntu-latest - timeout-minutes: 20 - steps: - - uses: actions/checkout@v6 - - uses: extractions/setup-just@v4 - - uses: astral-sh/setup-uv@v8.2.0 - # uv.lock is git-ignored (repo convention), so a clean checkout lacks it. - # `just bench-check` builds the compose `application` image, whose Dockerfile - # does `COPY uv.lock` + `uv sync --frozen`; regenerate the lock first so that - # build has it. (The pytest job runs `uv sync` directly and needs no compose.) - - run: uv lock - - run: just bench-check - pytest: runs-on: ubuntu-latest strategy: diff --git a/.github/workflows/benchmarks.yml b/.github/workflows/benchmarks.yml new file mode 100644 index 0000000..c53034e --- /dev/null +++ b/.github/workflows/benchmarks.yml @@ -0,0 +1,55 @@ +name: benchmarks + +on: + push: + branches: [main] + pull_request: {} + +permissions: + contents: read + pull-requests: write + +concurrency: + group: benchmarks-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + bench: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v6 + - uses: extractions/setup-just@v4 + - uses: astral-sh/setup-uv@v8.2.0 + # uv.lock is git-ignored (repo convention), so a clean checkout lacks it. + # `just bench-report` builds the compose `application` image, whose Dockerfile + # does `COPY uv.lock` + `uv sync --frozen`; regenerate the lock first. + - run: uv lock + - name: Run the gate and capture the markdown report + shell: bash + run: | + # `docker compose run` leaks buildkit image-build output onto stdout in + # CI, so a naive capture mixes it into the comment. The report always + # begins with the `## Benchmark gate` sentinel and the container prints + # it last, so slice from that heading to EOF -- independent of how + # compose routes its own noise. Own the exit code manually: `set +e` + # (shell: bash runs with -e) so a failing gate still lets us post the + # report, then re-exit with the gate's status so the build still fails. + set +e + just bench-report > "$RUNNER_TEMP/raw.log" + status=$? + sed -n '/^## Benchmark gate/,$p' "$RUNNER_TEMP/raw.log" \ + | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + exit "$status" + - name: Comment the report on the PR + # always(): comment even when the gate failed -- that is when it matters most. + # continue-on-error: fork PRs get a read-only token and cannot comment; the + # gate above already decided the build, and the run Summary still has the table. + if: always() && github.event_name == 'pull_request' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh pr comment "${{ github.event.pull_request.number }}" \ + --edit-last --create-if-none \ + --body-file "$RUNNER_TEMP/bench-report.md" diff --git a/Justfile b/Justfile index 7c1272b..1653e3a 100644 --- a/Justfile +++ b/Justfile @@ -52,6 +52,10 @@ bench *args: down && down bench-check: down && down docker compose run application uv run python -m benchmarks check +# Gate + emit the markdown report to stdout (CI PR comment). -T keeps stdout clean. +bench-report: down && down + docker compose run -T application uv run python -m benchmarks check --markdown + # Serve docs at http://127.0.0.1:8000 with hot-reload on save. docs-serve: uvx --with-requirements docs/requirements.txt mkdocs serve diff --git a/benchmarks/__main__.py b/benchmarks/__main__.py index 7e5483c..7c67b8e 100644 --- a/benchmarks/__main__.py +++ b/benchmarks/__main__.py @@ -12,7 +12,7 @@ import sys from benchmarks.config import DEFAULT_DSN, RunConfig -from benchmarks.report import compare, format_table, to_baseline +from benchmarks.report import compare, format_markdown, format_table, to_baseline from benchmarks.workload import RunResult, make_engine, run_consumer, run_producer @@ -66,7 +66,7 @@ async def _run_sweep(dsn: str, messages: int, repeats: int) -> list[RunResult]: return results -def _check(dsn: str) -> int: +def _check(dsn: str, *, markdown: bool = False) -> int: """Run the sweep at the baseline's message count and gate the counters.""" if not BASELINE_PATH.exists(): sys.stdout.write(f"no baseline at {BASELINE_PATH}; run `just bench --write-baseline` first\n") @@ -77,8 +77,12 @@ def _check(dsn: str) -> int: # so repeats=1 -- repeating them only burns CI time. messages = int(next(iter(baseline["runs"].values()))["messages"]) results = asyncio.run(_run_sweep(dsn, messages, repeats=1)) - sys.stdout.write(format_table(results) + "\n") failures = compare(to_baseline(results), baseline) + if markdown: + # Python owns the whole PR-comment body; the workflow just posts stdout. + sys.stdout.write(format_markdown(results, failures) + "\n") + return 1 if failures else 0 + sys.stdout.write(format_table(results) + "\n") if failures: sys.stdout.write("\nBENCHMARK GATE FAILED:\n") for failure in failures: @@ -106,12 +110,13 @@ def main() -> int: # uses 1: the gated counters are deterministic, so repeating them only burns CI time. parser.add_argument("--repeats", type=int, default=3) parser.add_argument("--write-baseline", action="store_true") + parser.add_argument("--markdown", action="store_true") args = parser.parse_args() dsn = os.environ.get("POSTGRES_DSN", DEFAULT_DSN) if args.command == "check": - return _check(dsn) + return _check(dsn, markdown=args.markdown) return _run(dsn, args.messages, args.repeats, write_baseline=args.write_baseline) diff --git a/benchmarks/report.py b/benchmarks/report.py index 073f055..ee683e0 100644 --- a/benchmarks/report.py +++ b/benchmarks/report.py @@ -127,6 +127,37 @@ def format_table(results: list[RunResult]) -> str: return "\n".join(lines) +def format_markdown(results: list[RunResult], failures: list[str]) -> str: + """Render the sweep as a GitHub-flavored Markdown comment body. + + Heading + verdict + table + a condensed gated-vs-informational footnote. + ``failures`` is ``compare()``'s output: empty means the gate passed. This + function only formats -- the exit code is the caller's, so the comment can be + posted on both pass and fail. + """ + verdict = "✅ gate passed" if not failures else "❌ gate FAILED" + lines = ["## Benchmark gate", "", verdict, ""] + if failures: + lines.extend(f"- {failure}" for failure in failures) + lines.append("") + lines.append("| scenario | msg/s | delete/msg | WALrec/msg | WALB/msg | fpi | upd | del | dead_tup |") + lines.append("| --- | --: | --: | --: | --: | --: | --: | --: | --: |") + for r in results: + m = normalize(r) + lines.append( + f"| {_key(r)} | {m['msgs_per_second']:.0f} | {m['delete_calls_per_msg']:.3f} " + f"| {m['wal_records_per_msg']:.2f} | {m['wal_bytes_per_msg']:.0f} | {m['wal_fpi']:.0f} " + f"| {m['tup_upd']:.0f} | {m['tup_del']:.0f} | {m['dead_tup']:.0f} |", + ) + lines.append("") + lines.append( + "_Gated (fails the build): `delete_calls` + tuple counters (upd/del/ins) + the " + "producer's `insert_calls`/`select_calls`, exact; `wal_records` within a 10% band. " + "msg/s, WAL bytes and total calls are informational (timing/FPI noise)._", + ) + return "\n".join(lines) + + def compare(current: dict[str, typing.Any], baseline: dict[str, typing.Any]) -> list[str]: """Diff current raw totals against the baseline. Empty list means pass. diff --git a/planning/changes/2026-07-17.03-benchmark-pr-comment.md b/planning/changes/2026-07-17.03-benchmark-pr-comment.md new file mode 100644 index 0000000..c1ec8f4 --- /dev/null +++ b/planning/changes/2026-07-17.03-benchmark-pr-comment.md @@ -0,0 +1,195 @@ +--- +summary: Post the existing `bench-check` sweep table as a sticky PR comment (and run Summary). A new `benchmarks check --markdown` mode owns the comment body; a scoped `benchmarks.yml` workflow captures it via stdout and posts with `gh pr comment`. The counter gate is unchanged; the bench job moves out of `_checks.yml` so `pull-requests: write` stays scoped. +--- + +# Design: Surface the benchmark gate as a PR comment + +## Summary + +The benchmark gate (`just bench-check`) already computes a per-message sweep +table on every PR, but its output dies in the CI log where no one reads it. This +change surfaces that table as a **sticky PR comment** and on the run's Summary +page, without touching the gate itself. Python gains a `benchmarks check +--markdown` mode that renders the table plus a pass/fail verdict as a Markdown +document; a new, permission-scoped `benchmarks.yml` workflow captures that +document from stdout and posts it with `gh pr comment`. No third-party actions, +no new metrics, no timing trend. + +## Motivation + +The prompt was "modern-di comments its benchmark results on PRs — can we do the +same?" modern-di's mechanism +(`benchmark-action/github-action-benchmark@v1`) does **not** transfer: it speaks +pytest-benchmark *timing* JSON, keeps a weak cached-from-`main` baseline, and +alerts on a 150% wall-clock threshold — exactly the timing-gate model +[the harness design](2026-07-14.01-benchmark-harness.md) rejected as "a flake +generator." Our gate is the stronger design: deterministic Postgres counters +against a committed, reviewed `baseline.json`. + +So only the **PR comment** is worth copying, not the machinery behind it. Today +`bench-check`'s `format_table` output — the sweep a reviewer would actually want +when weighing a perf change — is visible only to whoever expands the CI log. + +## Design + +### `benchmarks check --markdown` + +`report.py` gains `format_markdown(results, failures)` beside the existing +`format_table`. `__main__.py`'s `check` command takes a `--markdown` flag; when +set, `_check` writes a Markdown document to stdout **instead of** the plain +table, and returns the **same** exit code (0 pass / 1 fail) — the gate is +untouched. The document: + +- an `## Benchmark gate` heading; +- a verdict line — `✅ gate passed` or `❌ gate FAILED`, followed by the failure + bullets `compare()` already produces; +- the sweep reflowed as a GitHub Markdown table (same columns as `format_table`); +- a condensed gated-vs-informational footnote. + +Python owns the whole comment body, so it is unit-testable alongside the existing +`format_table` test and the YAML never builds strings. + +### `benchmarks.yml` + +A new top-level workflow — **not** an edit to `_checks.yml`. `_checks.yml` is a +reusable workflow `uses:`-called from `ci.yml`; granting it `pull-requests: +write` would widen the token for every job in that file (lint, docs, pytest). A +separate workflow scopes the write token to the one job that needs it (this is +why modern-di is structured the same way). + +```yaml +name: benchmarks +on: + push: { branches: [main] } + pull_request: {} +permissions: + contents: read + pull-requests: write +jobs: + bench: + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v6 + - uses: extractions/setup-just@v4 + - uses: astral-sh/setup-uv@v8.2.0 + - run: uv lock # uv.lock is git-ignored; compose build needs it + - name: Run gate, capture markdown + shell: bash + run: | + set +e # own the exit code; -e would skip the post + just bench-report > "$RUNNER_TEMP/raw.log" + status=$? # the gate's real result + sed -n '/^## Benchmark gate/,$p' "$RUNNER_TEMP/raw.log" \ + | tee "$GITHUB_STEP_SUMMARY" "$RUNNER_TEMP/bench-report.md" + exit "$status" # re-fail the build on drift + - name: Comment on PR + if: always() && github.event_name == 'pull_request' + continue-on-error: true # fork PRs get a read-only token; degrade quietly + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + gh pr comment "${{ github.event.pull_request.number }}" \ + --edit-last --create-if-none \ + --body-file "$RUNNER_TEMP/bench-report.md" +``` + +The `bench` job is **deleted from `_checks.yml`** and lands here (its `uv lock` +step and 20-minute timeout carry over verbatim) so it does not run twice. `main` +is unprotected with no required status checks, so moving the job breaks nothing. + +Two hazards, both hit on the first live PR and both handled above: + +1. **Compose leaks build noise onto stdout.** `-T` suppresses the TTY but does + **not** separate `docker compose run`'s buildkit image-build output, which in + CI lands on stdout — the first live comment was ~120 lines of `#8 extracting …` + before the table. So the capture does not trust stdout hygiene: it slices the + report from its `## Benchmark gate` sentinel to EOF. The container prints the + report last and nothing follows it on stdout (compose's post-`down` writes to + stderr), so heading-to-EOF is exactly the Markdown, independent of how compose + routes its own noise. +2. **`GITHUB_STEP_SUMMARY` is per-step.** It is a **different path in every + step**, so a comment step reading it directly would find its own empty summary + and `gh pr comment` would fail on a blank body (silently, under + `continue-on-error`). The sliced report is teed to two sinks: + `$GITHUB_STEP_SUMMARY` (the run's Summary page, free and visible even on `push` + and fork PRs where no comment posts) and `$RUNNER_TEMP/bench-report.md`, the + file the comment step reads. `RUNNER_TEMP` persists across steps within the job + and sits outside the checked-out tree, so it leaves no stray workspace file. + +The exit code is owned by hand — `set +e`, capture the gate's status from the +redirect, re-`exit` it after teeing — because piping through `sed`/`tee` would +otherwise mask a failing gate, and because a failing gate must still post its +`❌` report (the slice runs regardless) before failing the build. +`--edit-last --create-if-none` makes the comment sticky (all flags verified +present in the runner's preinstalled gh 2.96.0). A top-level `concurrency` group +(`benchmarks-${{ github.head_ref || github.run_id }}`, `cancel-in-progress`) +stops rapid PR pushes from racing two `--edit-last` posts. + +### `bench-report` recipe + +``` +# Gate + emit the markdown report to stdout (CI PR comment). -T keeps stdout clean. +bench-report: down && down + docker compose run -T application uv run python -m benchmarks check --markdown +``` + +`-T` disables the pseudo-TTY so the Markdown carries no terminal control +characters. It does **not** by itself guarantee clean stdout — see hazard 1 +above — which is why the workflow slices the report rather than trusting the +recipe's stdout wholesale. `benchmarks check` still returns 1 on drift, and the +workflow re-exits that status, so the gate keeps biting. `bench-check` stays +as-is for local dev; `bench-report` is the CI entry point. + +**Capture, not a written file.** An earlier idea wrote the Markdown to a file in +the `.:/code` bind mount. Rejected: the Dockerfile runs as `USER runner`, whose +uid differs from the Linux runner's workspace owner, so that write can hit +permission-denied in CI (it works on macOS only because Docker Desktop's FUSE +mount ignores uid/gid). Capturing the container's stdout sidesteps the uid +problem; the sentinel slice then handles compose's stdout noise. + +## Non-goals + +- **No `benchmark-action` / timing trend.** Wall-clock stays ungated; the design + doc's reasoning holds. +- **No new metrics or gate change.** Same `EXACT_KEYS` / `TOLERANT_KEYS`, same + `baseline.json`, same exit codes. +- **No `pull_request_target`.** It would let fork PRs comment but runs untrusted + PR code with a write token. With 0 forks and 40/40 same-repo PRs, the fork case + is theoretical; the comment step `continue-on-error`s and the Summary page + still shows the table. +- **No weekly scheduled run.** `benchmarks.yml` triggers only on `push`-to-main + and `pull_request`, not on `scheduled.yml`'s Monday cron. Moving `bench` out of + the reusable `_checks.yml` (to scope the write token) means the weekly + dependency-drift check no longer runs the counter gate — a dep bump that + regresses the counters is caught on the next PR, not on Monday. Accepted + deliberately as the cost of the scoped token; re-add a `schedule` trigger here + if drift regressions start slipping through. + +## Testing + +- **Unit** (`tests/test_benchmarks.py`, no Postgres): `format_markdown` renders a + passing run with `✅` and a GitHub table; a failing run renders `❌` plus the + failure bullets. Beside the existing `test_format_table_*`. +- **Local integration**: `just bench-report` prints clean Markdown to stdout and + exits 0 against the committed baseline. +- **Gate still bites**: reuse the harness doc's method (extra `SELECT 1` per + message) → `bench-report` exits non-zero and the Markdown carries `❌`; revert. +- `--cov-fail-under=100` undisturbed: `benchmarks/*` stays in `[tool.coverage.run] + omit`; new tests import the pure functions like the existing ones. +- **Live comment post**: validated on this feature's own PR (#148). The first + attempt surfaced hazard 1 (compose build noise on stdout); after the sentinel + slice the sticky comment renders the clean table. + +## Risk + +- **Compose build noise on stdout** (materialized) — `docker compose run` puts + buildkit output on stdout in CI, so a naive capture posts a wall of build logs. + Mitigated by slicing from the `## Benchmark gate` sentinel to EOF, which is + independent of compose's stream routing. +- **Masking the gate exit** (medium × high) — piping through `sed`/`tee`, or + Actions' default `bash -e`, would drop the gate's non-zero status. Mitigated by + owning the exit code by hand (`set +e`, capture from the stdout redirect, + re-`exit`) and asserted by the gate-still-bites test. +- **Comment step failing the build on fork PRs** (low × low today) — mitigated by + `continue-on-error` + the Summary-page fallback. diff --git a/tests/test_benchmarks.py b/tests/test_benchmarks.py index 9cfb2d8..73a3b08 100644 --- a/tests/test_benchmarks.py +++ b/tests/test_benchmarks.py @@ -11,7 +11,7 @@ from benchmarks.config import RunConfig from benchmarks.probes import ProbeResult -from benchmarks.report import EXACT_KEYS, compare, format_table, normalize, to_baseline +from benchmarks.report import EXACT_KEYS, compare, format_markdown, format_table, normalize, to_baseline from benchmarks.workload import RunResult @@ -193,3 +193,22 @@ def test_format_table_labels_gated_vs_informational() -> None: assert "INFORMATIONAL" in table assert "wal_records" in table assert "delete_calls" in table + + +def test_format_markdown_renders_passing_verdict_and_table() -> None: + body = format_markdown([_result(), _producer()], []) + assert body.startswith("## Benchmark gate") + assert "✅ gate passed" in body + # Both runs render as rows. + assert "consumer/w1/b100" in body + assert "producer/w1/b100" in body + # GitHub table shape: a header row and a delimiter row. + assert "| scenario |" in body + assert "| --- |" in body + + +def test_format_markdown_renders_failing_verdict_and_bullets() -> None: + failures = ["consumer/w1/b100: delete_calls changed (exact-gated): baseline 500 -> current 501"] + body = format_markdown([_result()], failures) + assert "❌ gate FAILED" in body + assert "- consumer/w1/b100: delete_calls changed" in body