From 11bf576cc18d3404e2ffbbf9d7abd4b2d97744d7 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 24 Oct 2026 11:04:04 +1100 Subject: [PATCH 1/4] ci(perf): compare against the pull request's base commit and name skipped endpoints The perf job now compares against github.event.pull_request.base.sha rather than the moving base branch, passed through env rather than expanded into the script. perf-compare.sh names each endpoint that failed on the base in a warning and the job summary, because it has no baseline and is left out of the comparison. It fails when the base run produced no report at all. --- .github/workflows/ci.yml | 7 +++++-- backend/scripts/perf-compare.sh | 27 +++++++++++++++++++++++++-- backend/tests/perf/README.md | 2 ++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1695baf9..86d5aef1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,8 +113,10 @@ jobs: # The databases depend on the climate-ref version on each side, which the lock files pin. - name: Key the synthetic database cache id: perf-db + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - key=$(cat uv.lock <(git show "origin/${{ github.base_ref }}:backend/uv.lock") | sha256sum | cut -c1-16) + key=$(cat uv.lock <(git show "$BASE_SHA:backend/uv.lock") | sha256sum | cut -c1-16) echo "key=perf-db-$key" >> "$GITHUB_OUTPUT" - name: Cache the synthetic databases @@ -126,5 +128,6 @@ jobs: - name: Compare query performance against the base branch env: REF_PERF_ROUNDS: "10" + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | - scripts/perf-compare.sh "origin/${{ github.base_ref }}" + scripts/perf-compare.sh "$BASE_SHA" diff --git a/backend/scripts/perf-compare.sh b/backend/scripts/perf-compare.sh index 94ff3612..8fa0d868 100755 --- a/backend/scripts/perf-compare.sh +++ b/backend/scripts/perf-compare.sh @@ -24,8 +24,31 @@ git -C "$BACKEND" worktree add --quiet --detach "$WORK/base" "$BASE" uv sync --locked --quiet uv run --with pytest-benchmark pytest "$BACKEND/tests/perf" \ --rootdir="$BACKEND" --confcutdir="$BACKEND/tests/perf" -p no:cacheprovider -q \ - --benchmark-storage="$STORAGE" --benchmark-save=base -) || echo "::warning::some performance tests failed on $BASE, comparing the rest" + --benchmark-storage="$STORAGE" --benchmark-save=base --junitxml="$WORK/base.xml" +) || true + +# Name each endpoint that failed on the base, because it has no baseline and is not compared. +python3 - "$WORK/base.xml" "$BASE" <<'EOF' +import os +import sys +import xml.etree.ElementTree as ET + +report, base = sys.argv[1:] +if not os.path.exists(report): + sys.exit(f"::error::the performance tests did not run on {base}") +lines = [] +for case in ET.parse(report).iter("testcase"): + problem = case.find("failure") + if problem is None: + problem = case.find("error") + if problem is not None: + name, reason = case.get("name"), (problem.get("message") or "failed").splitlines()[0][:200] + print(f"::warning title=Not compared::{name} failed on {base}: {reason.replace('%', '%25')}") + lines.append(f"- `{name}`: {reason}") +if lines and "GITHUB_STEP_SUMMARY" in os.environ: + with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as summary: + summary.write(f"### Not compared, because they failed on {base}\n\n" + "\n".join(lines) + "\n") +EOF cd "$BACKEND" uv run pytest tests/perf -q \ diff --git a/backend/tests/perf/README.md b/backend/tests/perf/README.md index 13784f1a..e4920f81 100644 --- a/backend/tests/perf/README.md +++ b/backend/tests/perf/README.md @@ -30,6 +30,8 @@ make perf-compare BASE=v0.8.2 # against any ref This checks out the base in a temporary worktree and runs this branch's tests against both. It fails when an endpoint's median is more than `REF_PERF_THRESHOLD` percent slower than on the base, default 30. +An endpoint that fails on the base, such as one added by this branch, is not compared. +Each one is named in a warning. Running the same code twice locally drifts by up to about 20%, so a smaller threshold is noise. CI runs this on every pull request, and caches the databases until either lock file changes. From ce3409655528dbc4b2179ccd02da7e74a22fff41 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 24 Oct 2026 11:04:22 +1100 Subject: [PATCH 2/4] docs: add changelog for #149 --- changelog/149.trivial.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/149.trivial.md diff --git a/changelog/149.trivial.md b/changelog/149.trivial.md new file mode 100644 index 00000000..259ebb6c --- /dev/null +++ b/changelog/149.trivial.md @@ -0,0 +1 @@ +The CI performance comparison now uses the pull request's base commit, and names each endpoint it could not compare. From 9eccf3d699b55912300f7b14ca643302d6cc8c51 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 24 Oct 2026 11:09:28 +1100 Subject: [PATCH 3/4] ci(perf): name endpoints skipped on the base A base without the synthetic database generator skips every endpoint, which left the job green while comparing nothing. Skipped endpoints are now named in the warnings and job summary like failed ones. --- backend/scripts/perf-compare.sh | 16 ++++++++-------- backend/tests/perf/README.md | 2 +- changelog/149.trivial.md | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/backend/scripts/perf-compare.sh b/backend/scripts/perf-compare.sh index 8fa0d868..c070d0f4 100755 --- a/backend/scripts/perf-compare.sh +++ b/backend/scripts/perf-compare.sh @@ -27,7 +27,7 @@ git -C "$BACKEND" worktree add --quiet --detach "$WORK/base" "$BASE" --benchmark-storage="$STORAGE" --benchmark-save=base --junitxml="$WORK/base.xml" ) || true -# Name each endpoint that failed on the base, because it has no baseline and is not compared. +# Name each endpoint that failed or skipped on the base, because it has no baseline and is not compared. python3 - "$WORK/base.xml" "$BASE" <<'EOF' import os import sys @@ -38,16 +38,16 @@ if not os.path.exists(report): sys.exit(f"::error::the performance tests did not run on {base}") lines = [] for case in ET.parse(report).iter("testcase"): - problem = case.find("failure") - if problem is None: - problem = case.find("error") - if problem is not None: - name, reason = case.get("name"), (problem.get("message") or "failed").splitlines()[0][:200] - print(f"::warning title=Not compared::{name} failed on {base}: {reason.replace('%', '%25')}") + problems = [p for tag in ("failure", "error", "skipped") if (p := case.find(tag)) is not None] + if problems: + problem = problems[0] + name, reason = case.get("name"), (problem.get("message") or problem.tag).splitlines()[0][:200] + print(f"::warning title=Not compared::{name} on {base}: {reason.replace('%', '%25')}") lines.append(f"- `{name}`: {reason}") if lines and "GITHUB_STEP_SUMMARY" in os.environ: with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as summary: - summary.write(f"### Not compared, because they failed on {base}\n\n" + "\n".join(lines) + "\n") + summary.write(f"### Not compared, because they failed or skipped on {base}\n\n") + summary.write("\n".join(lines) + "\n") EOF cd "$BACKEND" diff --git a/backend/tests/perf/README.md b/backend/tests/perf/README.md index e4920f81..dd64e547 100644 --- a/backend/tests/perf/README.md +++ b/backend/tests/perf/README.md @@ -30,7 +30,7 @@ make perf-compare BASE=v0.8.2 # against any ref This checks out the base in a temporary worktree and runs this branch's tests against both. It fails when an endpoint's median is more than `REF_PERF_THRESHOLD` percent slower than on the base, default 30. -An endpoint that fails on the base, such as one added by this branch, is not compared. +An endpoint that fails or skips on the base, such as one added by this branch, is not compared. Each one is named in a warning. Running the same code twice locally drifts by up to about 20%, so a smaller threshold is noise. CI runs this on every pull request, and caches the databases until either lock file changes. diff --git a/changelog/149.trivial.md b/changelog/149.trivial.md index 259ebb6c..ad4fbb09 100644 --- a/changelog/149.trivial.md +++ b/changelog/149.trivial.md @@ -1 +1 @@ -The CI performance comparison now uses the pull request's base commit, and names each endpoint it could not compare. +The CI performance comparison now uses the pull request's base commit, and names each endpoint it could not compare, including when the base cannot run the tests. From 9a449240582d8f2b2cf2902184cd8d4a88f70711 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Sat, 24 Oct 2026 11:15:56 +1100 Subject: [PATCH 4/4] ci(perf): write a table of the timings to the job summary Adds scripts/perf-report.py, which lists each endpoint's base and branch medians and the change, marking those over the threshold, in the job summary and the log. It takes over naming the endpoints that have no baseline. perf-compare.sh writes the report even when the comparison fails, then exits with its status. --- backend/scripts/perf-compare.sh | 32 +++--------- backend/scripts/perf-report.py | 86 +++++++++++++++++++++++++++++++++ backend/tests/perf/README.md | 1 + changelog/149.trivial.md | 2 +- 4 files changed, 95 insertions(+), 26 deletions(-) create mode 100644 backend/scripts/perf-report.py diff --git a/backend/scripts/perf-compare.sh b/backend/scripts/perf-compare.sh index c070d0f4..a90cd263 100755 --- a/backend/scripts/perf-compare.sh +++ b/backend/scripts/perf-compare.sh @@ -24,34 +24,16 @@ git -C "$BACKEND" worktree add --quiet --detach "$WORK/base" "$BASE" uv sync --locked --quiet uv run --with pytest-benchmark pytest "$BACKEND/tests/perf" \ --rootdir="$BACKEND" --confcutdir="$BACKEND/tests/perf" -p no:cacheprovider -q \ - --benchmark-storage="$STORAGE" --benchmark-save=base --junitxml="$WORK/base.xml" + --benchmark-storage="$STORAGE" --benchmark-save=base \ + --benchmark-json="$WORK/base.json" --junitxml="$WORK/base.xml" ) || true -# Name each endpoint that failed or skipped on the base, because it has no baseline and is not compared. -python3 - "$WORK/base.xml" "$BASE" <<'EOF' -import os -import sys -import xml.etree.ElementTree as ET - -report, base = sys.argv[1:] -if not os.path.exists(report): - sys.exit(f"::error::the performance tests did not run on {base}") -lines = [] -for case in ET.parse(report).iter("testcase"): - problems = [p for tag in ("failure", "error", "skipped") if (p := case.find(tag)) is not None] - if problems: - problem = problems[0] - name, reason = case.get("name"), (problem.get("message") or problem.tag).splitlines()[0][:200] - print(f"::warning title=Not compared::{name} on {base}: {reason.replace('%', '%25')}") - lines.append(f"- `{name}`: {reason}") -if lines and "GITHUB_STEP_SUMMARY" in os.environ: - with open(os.environ["GITHUB_STEP_SUMMARY"], "a") as summary: - summary.write(f"### Not compared, because they failed or skipped on {base}\n\n") - summary.write("\n".join(lines) + "\n") -EOF - cd "$BACKEND" +status=0 uv run pytest tests/perf -q \ --benchmark-storage="$STORAGE" --benchmark-compare \ --benchmark-compare-fail="median:${REF_PERF_THRESHOLD:-30}%" \ - --benchmark-columns=median,iqr,rounds + --benchmark-columns=median,iqr,rounds --benchmark-json="$WORK/head.json" || status=$? + +python3 scripts/perf-report.py "$WORK" "$BASE" "${REF_PERF_THRESHOLD:-30}" +exit "$status" diff --git a/backend/scripts/perf-report.py b/backend/scripts/perf-report.py new file mode 100644 index 00000000..f0b4517f --- /dev/null +++ b/backend/scripts/perf-report.py @@ -0,0 +1,86 @@ +""" +Summarise a `perf-compare.sh` run as a Markdown table of medians, base against this checkout. + +Usage: python3 scripts/perf-report.py WORK_DIR BASE THRESHOLD + +Reads `base.json`, `base.xml` and `head.json` from WORK_DIR. +The table goes to stdout, and to the job summary when run in GitHub Actions. +Each endpoint with no baseline is also named in a warning annotation. +""" + +import json +import os +import re +import sys +import xml.etree.ElementTree as ET +from pathlib import Path + +_PARAM = re.compile(r"\[(.*)\]$") + + +def _endpoint(name: str) -> str: + match = _PARAM.search(name) + return match.group(1) if match else name + + +def _medians(path: Path) -> dict[str, float]: + if not path.exists(): + return {} + benchmarks = json.loads(path.read_text())["benchmarks"] + return {_endpoint(b["name"]): b["stats"]["median"] * 1000 for b in benchmarks} + + +def _not_run(report: Path) -> dict[str, str]: + """Why each endpoint has no baseline, from the base run's JUnit report.""" + reasons = {} + for case in ET.parse(report).iter("testcase"): # noqa: S314, written by our own pytest run + problems = [p for tag in ("failure", "error", "skipped") if (p := case.find(tag)) is not None] + if problems: + message = problems[0].get("message") or problems[0].tag + reasons[_endpoint(case.get("name", ""))] = message.splitlines()[0][:200].replace("|", "\\|") + return reasons + + +def main() -> None: + work, base, threshold = Path(sys.argv[1]), sys.argv[2], float(sys.argv[3]) + if re.fullmatch(r"[0-9a-f]{40}", base): + base = base[:12] + if not (work / "base.xml").exists(): + sys.exit(f"::error::the performance tests did not run on {base}") + + before, after = _medians(work / "base.json"), _medians(work / "head.json") + not_run = _not_run(work / "base.xml") + for endpoint, reason in not_run.items(): + print(f"::warning title=Not compared::{endpoint} on {base}: {reason.replace('%', '%25')}") + + rounds, scale = os.environ.get("REF_PERF_ROUNDS", "5"), os.environ.get("REF_PERF_SCALE", "0.1") + lines = [ + f"### API performance against `{base}`", + "", + f"Median of {rounds} rounds at scale {scale}. More than {threshold:g}% slower fails the job.", + "", + ] + if not after: + lines.append("No endpoint ran on this branch.") + else: + lines += ["| Endpoint | Base (ms) | This branch (ms) | Change |", "| --- | ---: | ---: | ---: |"] + for endpoint in sorted(after): + if endpoint not in before: + reason = not_run.get(endpoint, "no result") + lines.append(f"| `{endpoint}` | not compared | {after[endpoint]:.1f} | {reason} |") + continue + change = round((after[endpoint] / before[endpoint] - 1) * 100) + flag = " **slower**" if change > threshold else "" + lines.append( + f"| `{endpoint}` | {before[endpoint]:.1f} | {after[endpoint]:.1f} | {change:+d}%{flag} |" + ) + + table = "\n".join(lines) + "\n" + print(table) + if summary := os.environ.get("GITHUB_STEP_SUMMARY"): + with open(summary, "a") as f: + f.write(table) + + +if __name__ == "__main__": + main() diff --git a/backend/tests/perf/README.md b/backend/tests/perf/README.md index dd64e547..2f039b8d 100644 --- a/backend/tests/perf/README.md +++ b/backend/tests/perf/README.md @@ -32,6 +32,7 @@ This checks out the base in a temporary worktree and runs this branch's tests ag It fails when an endpoint's median is more than `REF_PERF_THRESHOLD` percent slower than on the base, default 30. An endpoint that fails or skips on the base, such as one added by this branch, is not compared. Each one is named in a warning. +CI writes a table of every endpoint's medians and change to the job summary, from `scripts/perf-report.py`. Running the same code twice locally drifts by up to about 20%, so a smaller threshold is noise. CI runs this on every pull request, and caches the databases until either lock file changes. diff --git a/changelog/149.trivial.md b/changelog/149.trivial.md index ad4fbb09..2ffcb0d0 100644 --- a/changelog/149.trivial.md +++ b/changelog/149.trivial.md @@ -1 +1 @@ -The CI performance comparison now uses the pull request's base commit, and names each endpoint it could not compare, including when the base cannot run the tests. +The CI performance comparison now uses the pull request's base commit, and writes a table of every endpoint's timings to the job summary, naming each endpoint it could not compare.