Skip to content

fix(bench): the regression harness builds again - #400

Merged
FBumann merged 1 commit into
mainfrom
fix/regression-harness
Jul 31, 2026
Merged

fix(bench): the regression harness builds again#400
FBumann merged 1 commit into
mainfrom
fix/regression-harness

Conversation

@FBumann

@FBumann FBumann commented Jul 31, 2026

Copy link
Copy Markdown
Owner

build_and_hand_over passes memory_limit='1GB' to lps.build, which has had no such parameter since #189 retired the engine that took one. Half the regression workloads have been raising TypeError — invisibly, because bench/regressions sits outside testpaths and nothing in CI runs it.

Why this matters beyond the one line

This is the harness for "did this change make it worse?" — one lane compared to itself, memray rather than RSS because the bias cancels within a lane and the number is attributable to a call stack. It is what any performance work leans on, and half of it was dead.

It is also already engine-agnostic, which is the part worth writing down: nothing in it names an engine, so it goes through lps.build and LPSPEC_ENGINE selects one. The same two commands answer the same question for either engine — no per-engine tooling, no second harness.

uv sync --group bench
uv run pytest bench/regressions --benchmark-memory                 # record
uv run pytest bench/regressions --benchmark-memory-compare=<id>    # compare

The docstring now says that, with the caveat that matters: a stored baseline does not record which engine produced it, so comparing across engines measures the engine rather than the change.

Verified on dispatch-s, both workloads running:

wall (mean) memray peak
polars 17.3 ms 19.7 MB
duckdb 45.7 ms 118.5 MB

Scope

One line of behaviour plus the docstring. No new tooling — comparing two runs by hand with --benchmark-memory-compare is what the harness already does and it is enough.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation

    • Clarified that LPSPEC_ENGINE controls the engine used during builds.
    • Added guidance to avoid comparing performance baselines across different engines.
  • Bug Fixes

    • Removed the fixed 1 GB memory limit from the build handoff process.

`build_and_hand_over` passed `memory_limit='1GB'` to `lps.build`, which has had
no such parameter since #189 retired the engine that took one. Half the
regression workloads have been raising `TypeError` ever since — invisibly,
because `bench/regressions` is outside `testpaths` and nothing in CI runs it.

The docstring now also says what the harness measures: *whatever `lps.build`
builds with*. Nothing here names an engine, so `LPSPEC_ENGINE` selects one and
the same two commands answer the same question for either — with the caveat
that matters, that a stored baseline does not record which engine produced it,
so comparing across engines measures the engine rather than the change.

Verified on `dispatch-s`: both workloads run, and the memray peak separates the
engines cleanly (19.7 MB against 118.5 MB), which is the signal perf work needs.
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f4950652-fa20-4bdd-bcf7-1e3c0735fee4

📥 Commits

Reviewing files that changed from the base of the PR and between 7cff562 and fb405c6.

📒 Files selected for processing (1)
  • bench/regressions/test_build.py

📝 Walkthrough

Walkthrough

The benchmark documentation now defines engine selection through LPSPEC_ENGINE. The build helper no longer passes a fixed memory_limit value to lps.build.

Changes

Benchmark build behavior

Layer / File(s) Summary
Engine guidance and build configuration
bench/regressions/test_build.py
The documentation describes LPSPEC_ENGINE and prohibits cross-engine baseline comparisons. build_and_hand_over no longer passes memory_limit='1GB' to lps.build.

Estimated code review effort: 2 (Simple) | ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the benchmark regression harness fix and matches the main change in the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/regression-harness

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@FBumann
FBumann merged commit 291f714 into main Jul 31, 2026
5 checks passed
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.

1 participant