[Feat] Automated benchmark regression check in CI (PR vs main)

[prajwal-tech07]

Spinning this out into its own issue (per the dev-meeting discussion) so the CI regression-check has a dedicated home. Builds on the benchmark scripts in #117 and the JSON output in #140 (cc @yuvraajnarula), and follows up the "Automated Performance Regression" part of #137.

Everything below runs entirely on GitHub-hosted runners - no external service or account required.

Goal

On a PR, run the scaling benchmark on both the PR commit and current main, and post a comment showing whether the PR makes graph construction slower, faster, or the same - so regressions are visible before merge.

Key design decision: compare on the same runner

GitHub Actions runners are noisy - identical code can vary 20–50% between runs on different VMs. So comparing a PR against a baseline captured earlier on a different runner would produce frequent false alarms.

Instead I propose running main and the PR back-to-back in the same job, on the same runner, and comparing relative % change rather than absolute seconds. Same hardware for both sides → the noise largely cancels. This also matches the workflow described in the meeting.

Correctness detail: hold the harness constant

When we git checkout main, that reverts both the library and the benchmark script. To compare fairly we keep the benchmark script fixed (the PR's version) and only swap the library under test - i.e. run the same measurement against two installs of the code. Otherwise we'd be measuring with two different rulers.

How it works

on: pull_request (paths: src/.../create/**) + manual dispatch

1. checkout PR head → install lib → run scaling benchmark → pr.json
2. checkout main    → install lib → run same benchmark    → main.json
   (same runner, same job, same benchmark script)
3. compare.py main.json pr.json → markdown delta table
4. post / update a sticky PR comment with the result

Example comment:

grid points	main	PR	Δ
1,024	0.42s	0.43s	+2% ✅
4,096	1.81s	2.30s	+27% ⚠️

The only new code is a small compare.py (parse the two JSONs from #140, compute deltas, render the table, flag on threshold) plus the workflow YAML.

Guardrails against noise

• Compare % change, never absolute time.
• Tolerance threshold — only flag a regression above e.g. +25% and consistent across grid sizes.
• Cap grid sizes to fit the CI time budget (small/medium grids, not 10⁷ nodes).
• Informational comment first, non-blocking - so a noisy perf check doesn't train people to ignore CI. Can be promoted to a hard gate later once the threshold is proven.

Plan

• Phase 0 (prereq): land [Feat] runtime scaling and flamegraph benchmark CLIs #117 + feat: add memory profiling and JSON output to scaling benchmark #140 - the action consumes their script + JSON. (Happy to review feat: add memory profiling and JSON output to scaling benchmark #140; compare.py depends on its JSON schema.)
• Phase 1 (MVP): same-runner A/B workflow + compare.py → sticky PR comment, informational only.
• Phase 2: add repetitions + median, tune the regression threshold, clear ✅/⚠️ verdict line; optionally include the memory delta (using --track-memory from feat: add memory profiling and JSON output to scaling benchmark #140).
• Phase 3 (long-term): add historical trend tracking to also catch slow drift across many PRs (each small enough to pass the per-PR check, but adding up over time). Can be done with github-action-benchmark, which stores history on a gh-pages branch in this repo — still no external service.

Alternatives considered

• Baseline stored from a separate run (github-action-benchmark for the PR check): rejected for the per-PR check because the baseline runs on a different runner → cross-runner noise. Reused in Phase 3 only for trend tracking, where that's fine.

• Instrumentation-based services (e.g. CodSpeed): deterministic measurement, but adds an external vendor/account dependency - avoiding unless the team specifically wants it.

Does this direction look right? If so I'll start on Phase 1 once #117/#140 are in.

[leifdenby]

This all looks great @prajwal-tech07! Thanks for the write-up 🚀

Instead I propose running main and the PR back-to-back in the same job, on the same runner, and comparing relative % change rather than absolute seconds. Same hardware for both sides → the noise largely cancels. This also matches the workflow described in the meeting.

Good idea! I agree, and I had been thinking the same.

When we git checkout main, that reverts both the library and the benchmark script. To compare fairly we keep the benchmark script fixed (the PR's version) and only swap the library under test - i.e. run the same measurement against two installs of the code. Otherwise we'd be measuring with two different rulers.

Ah this is a good point! I hadn't thought of the fact that the implementation of the benchmark itself should be as close to identical across the two commits we're comparing. What we could do here, I think, is implement the benchmark as a python module so that it is available on main (maybe in tests/benchmarks/) and then if any PR modifies the benchmark script that will be clear in the PR too. That way you don't have to check that the two benchmark implementations are the same, if they aren't it will be clear from the files that the PR is changing.

When it comes to installing and executing the benchmark from the two PRs, I am realising that I actually implemented something like this when we were having a discussion on #62 on whether using cugraph would lead to speed-ups. I implemented a simple set of benchmarks there (we shouldn't use these though I don't think, but you could have a look). But we could maybe use the same approach for setting up the envs and launching the benchmarks: https://github.com/AdMub/weather-model-graphs/blob/feat/issue-164-cugraph-support/tests/benchmarks/run_benchmark.sh. Although I'm not sure about what would be the cleanest way of integrating this into the CI. What do you think?

Your example output for the PR comment looks great!

Tolerance threshold — only flag a regression above e.g. +25% and consistent across grid sizes.

I think this is a nice idea to avoid too many comments coming up on the PR. Do you think 25% is a good number though? I feel like that is quite a large increase. Maybe we should say flag for +10%, but we can also tune this based on what the typical variability is here. Maybe start by setting it quite low (say 0.1%) and then we can increase it in the PR you create until we don't see any "noise" :).

Cap grid sizes to fit the CI time budget (small/medium grids, not 10⁷ nodes).

Yes we will have to tune this. My gut feel is that the benchmark should not take more than a few minutes to execute.

• Phase 2: add repetitions + median, tune the regression threshold, clear ✅/⚠️ verdict line;

I am not sure repetitions will be necessary, but let's see :)

• Phase 3 (long-term): add historical trend tracking to also catch slow drift across many PRs (each small enough to pass the per-PR check, but adding up over time). Can be done with github-action-benchmark, which stores history on a gh-pages branch in this repo — still no external service.

I was thinking a really low-level implementation could just be posting comments onto a publicly visible gist in a specific format. Then we can parse these at a later date. Is that too hacky?

• Baseline stored from a separate run (github-action-benchmark for the PR check)

I think this is overkill for now. I agree with you. We can maybe do this at a later date if we see the need.

• Instrumentation-based services (e.g. CodSpeed): deterministic measurement, but adds an external vendor/account dependency - avoiding unless the team specifically wants it.

Agreed. Let's do our own implementation for now. If you find that it is too much to implement and there is another library that does a lot of this (handling setting up the envs, etc) then happy for you to suggest an alternative approach.

[prajwal-tech07]

Thanks @leifdenby! Green light noted - I'll get going. Answering your questions and folding in the decisions:

Benchmark as a module on main - agree, that's cleaner than manually checking the two scripts match. Plan: run the same benchmark module (from the PR checkout) against two installs of the library - one from the PR, one from main - in the same job. The harness stays fixed and we only swap the library, and any PR that touches the benchmark itself will show up in its diff. This depends on the tests/benchmarks/ module from #117 landing on main, so I'll treat that (plus #140's JSON output) as the prerequisite - happy to help get them over the line, and I'll prototype against the branches in the meantime.

Env setup / CI integration (your run_benchmark.sh) - that's a nice pattern; I'd reuse the uv-based setup from it. We can drop the GPU/cugraph half since GitHub-hosted runners are CPU-only. So one job, roughly:

uv venv
uv pip install .            # PR version  → run benchmark → pr.json
uv pip install "git+https://github.com/mllam/weather-model-graphs@main"   # → run same script → main.json
compare.py main.json pr.json → sticky PR comment

Threshold - agree 25% is too coarse. I'll start it very low (~0.1%) and raise it in the PR until the runner's own noise stops tripping it, so we land on a real empirical floor.

Execution time - yep, I'll cap the grid sizes so the whole thing stays within a few minutes.

Repetitions - happy to skip for now; I'll only add them if the noise floor turns out too high to get a stable threshold.

Trend tracking (the gist idea) - not too hacky as a zero-infra v0, but since this is the deferred Phase 3 I'd lean toward a small structured file in the repo (or the gh-pages route) over a gist when we get to it, just for durability and easy parsing. No rush - we can decide when we actually see drift.

Starting on the Phase 1 MVP. Will open a small PR against #144 once #117/#140 are in.

[prajwal-tech07]

@leifdenby - quick sequencing question to unblock the Phase 1 work here. #117 and #140 are both based off the same main commit and both add the full tests/benchmarks/ directory, and #140's scaling script (+184) looks like a superset of #117's (+91) - i.e. it already contains your scripts plus the JSON/memory additions. So the two would conflict on whichever merges second.

Since #140 includes everything #144 needs (--output-json), is the plan to merge #140 and close #117, or land #117 first and have #140 rebased down to just its delta? Happy to help with the rebase/testing once you've decided.

[yuvraajnarula]

Just saw this, @prajwal-tech07. Since #140 contains the baseline scaling scripts plus the JSON and memory tracking changes, I'm completely open to whatever @leifdenby prefers here.

If we want to land #140 first to unblock the CI work immediately, we can do that, or I can easily rebase my changes down to just the memory/JSON deltas if we want to merge #117 first. Standing by!

[leifdenby]

If we want to land #140 first to unblock the CI work immediately, we can do that, or I can easily rebase my changes down to just the memory/JSON deltas if we want to merge #117 first. Standing by!

@prajwal-tech07 and @yuvraajnarula: thanks for engaging on this! I would be completely happy with closing #117 and adding #140 instead since the latter I feel is simply a more complete implementation of the former. That is, if you feel @prajwal-tech07 that the implementation of how the output is written is adequate the your further work. What do you think @prajwal-tech07? That would mean that @yuvraajnarula takes over on adding baseline benchmarking to weather-model-graphs by working on #140 until it is ready to get merged, I will instead review #140 together with @prajwal-tech07, we will close #117, and #140 will replace #117 on the roadmap for release v0.4.0. Does that sound ok to you both?

Trend tracking (the gist idea) - not too hacky as a zero-infra v0, but since this is the deferred Phase 3 I'd lean toward a small structured file in the repo (or the gh-pages route) over a gist when we get to it, just for durability and easy parsing. No rush - we can decide when we actually see drift.

@prajwal-tech07 I agree on this. Rest of what you has written is great too, sounds like we're ready to go here once I've got the green light from you both that we will close #117 and have #140 replace it instead.