Skip to content

Add automated benchmark regression check in CI (#144) - #147

Open
prajwal-tech07 wants to merge 1 commit into
mllam:mainfrom
prajwal-tech07:feat/ci-benchmark-regression
Open

Add automated benchmark regression check in CI (#144)#147
prajwal-tech07 wants to merge 1 commit into
mllam:mainfrom
prajwal-tech07:feat/ci-benchmark-regression

Conversation

@prajwal-tech07

Copy link
Copy Markdown
Collaborator

Describe your changes

Adds an automated benchmark regression check in CI, implementing Phase 1 of #144.

On a pull request, the scaling benchmark is run on the PR and on its base branch back-to-back on the same runner, and a sticky PR comment reports the relative runtime change per grid size. Because both runs execute on the same VM and we compare relative (%) change rather than absolute seconds, the per-runner noise (which can be 20–50% across different GitHub-hosted runners) largely cancels.

How the harness is held constant: the benchmark script always comes from the PR checkout, and only the library under test is swapped between the two runs. The package uses a src/ layout, so the working tree never shadows the installed library - installing the base-branch build over the PR build cleanly swaps the code being measured while the ruler (the benchmark harness) stays fixed.

Changes:

  • tests/benchmarks/compare.py - standard-library-only script that parses two --output-json files (schema introduced in feat: add memory profiling and JSON output to scaling benchmark #140: {grid_points, runtime_s, peak_memory_mb}), computes per-grid-size % deltas, renders the Markdown comparison table, and flags any grid size above a configurable threshold. Informational by default (--fail-on-regression available for later).
  • tests/benchmarks/test_compare.py - 17 unit tests covering the comparison logic (regression/improvement/threshold boundary, null memory, mismatched grid sizes, zero-baseline guard, malformed input, exit codes, output file).
  • .github/workflows/benchmark-regression.yml - the same-runner A/B workflow. Uses uv for env setup, swaps only the library via uv pip install --reinstall-package, runs compare.py, and posts/updates the sticky comment via first-party actions/github-script (with a job-summary fallback so fork PRs still see results).

Design notes / deliberate choices (per the discussion in #144):

  • The threshold starts very low (0.1%) on purpose - it's meant to be ratcheted up in follow-up runs once we've observed the runner's real noise floor, rather than guessing a large number upfront.
  • The check is non-blocking, so a noisy perf signal doesn't train people to ignore CI. It can be promoted to a hard gate later once the threshold is proven.
  • Fork PRs get a read-only token, so on forks the sticky comment is skipped and results go to the job summary instead. This is intentional - commenting on forks would require pull_request_target, which would run untrusted PR code with a write token.
  • Memory-delta reporting and historical trend tracking are left for later phases.

Dependencies: no new runtime dependencies for the library. The workflow installs the package with its visualisation extra (matplotlib, already an optional dependency) to run the benchmark. compare.py is standard-library-only.

Issue Link

Addresses #144 (Phase 1). Builds on the benchmark scripts from #117 and the JSON output from #140.

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the documentation to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging)

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug

Run the scaling benchmark on the PR and its base branch back-to-back on
the same runner, swapping only the library under test (the src/ layout
keeps the PR's benchmark harness fixed), and post a sticky pull-request
comment with the relative runtime change per grid size. Compares relative
(%) change rather than absolute seconds so per-runner noise largely
cancels. Informational and non-blocking for now, with a low starting
threshold to be calibrated against the runner's noise floor.

- tests/benchmarks/compare.py: parse two --output-json files (schema from
  mllam#140), compute per-grid-size % deltas, render the markdown table, flag
  above a configurable threshold
- tests/benchmarks/test_compare.py: unit tests for the comparison logic
- .github/workflows/benchmark-regression.yml: same-runner A/B workflow,
  sticky comment via actions/github-script (job summary fallback for forks)
@prajwal-tech07

Copy link
Copy Markdown
Collaborator Author

Self-reviewed and ready for review ✅

This implements Phase 1 of #144 (automated benchmark regression check).

  • tests/benchmarks/compare.py — parses two --output-json runs (schema from feat: add memory profiling and JSON output to scaling benchmark #140), computes per-grid-size % deltas, renders the sticky Markdown table, flags above a configurable threshold. Standard-library only.
  • tests/benchmarks/test_compare.py — 17 unit tests (all green).
  • .github/workflows/benchmark-regression.yml — same-runner A/B: benchmark the PR build, swap in the base-branch library (the src/ layout keeps the PR's harness fixed), benchmark again, compare.py, post a sticky comment. Informational / non-blocking.

Notes:

  • The benchmark check ran green on this PR itself, exercising the full A/B flow end-to-end.
  • This is a fork PR, so the sticky-comment step falls back to the job summary (fork tokens are read-only) — by design; a same-repo PR posts the real comment.
  • Threshold intentionally starts at 0.1% so it can be calibrated up against the runner's real noise floor over the first few runs.

No rush given the timeline — happy for this to wait for review. cc @yuvraajnarula (builds on your JSON output).

# summary above. Never fail the job over the comment (informational).
if: github.event_name == 'pull_request'
continue-on-error: true
uses: actions/github-script@v7

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants