test: register --run-perf and let latency budgets govern (#1160) - #1197
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesThe test suite registers the Performance benchmark execution
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
robotrocketscience
added
the
attn:review
Reviewer's GuideRegisters the Sequence diagram for running perf tests with --run-perf and enforced timeoutssequenceDiagram
participant CLI as cli_pytest
participant Pytest as pytest_runner
participant Conftest as tests_conftest_py
participant PerfTest as perf_test_function
CLI->>Pytest: invoke_pytest_with_args(--run-perf)
Pytest->>Conftest: load_conftest_hooks()
Conftest->>Pytest: pytest_addoption_register_run_perf()
Pytest->>PerfTest: execute_perf_test()
PerfTest->>Pytest: config_getoption("--run-perf") via _has_run_perf
alt [run-perf enabled]
PerfTest->>PerfTest: apply_pytest_mark_timeout(120)
PerfTest->>PerfTest: run_latency_measurement_and_assert_budget()
else [run-perf disabled]
PerfTest->>PerfTest: skip_test_due_to_perf_guard()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Setr:2026-07-30T16:49:24Z] |
Four modules gate a latency assertion on a `_has_run_perf` helper that reads `--run-perf`, but nothing registered the option. `test_bm25_index.py` carried a `pytest_addoption` stub, and pytest only invokes that hook from `conftest.py` or an installed plugin — never from a plain test module. So `getoption` always raised, every helper fell back to False, and `pytest --run-perf` exited 4 with `unrecognized arguments`: the five perf tests could not be opted into at all, only reached by editing the guard. Registered in tests/conftest.py, which is where the hook is actually called, and once rather than per module so there is one authority for the flag. Defaults to False, so the suite skips them exactly as before. Refs #1160.
With the flag registered, the perf tests become reachable — and one of them could not pass. `pyproject.toml` sets a global `timeout = 5` sized for unit tests, while every perf test asserts its own larger wall-clock budget, so pytest-timeout decided the outcome ahead of the assertion: test_eigsolve_under_budget_n10k asserts elapsed < 30s, measures 6.76s, was killed at 5.0s inside the ARPACK solve — a guaranteed failure, and the test's own comment already said "allow 30s for noisy hosts". Two more were latent rather than broken: hrr's test_build_latency_at_n_10k asserts `elapsed <= 5.0`, exactly the global cap, so it could never pass at its own boundary; bm25's test_score_latency_under_5ms_n50k spends 3.07s building a 50k store before measuring, leaving under 2s of headroom on a slower host for an assertion about per-query milliseconds. Each now carries `@pytest.mark.timeout(_PERF_TIMEOUT_S)` at 120s — the per-test override convention pyproject.toml:125-127 documents. The bound is deliberately generous because it is only a hang guard; the budget is the assertion. Also drops the no-op `pytest_addoption` stub and the docstrings describing the flag as unregistered. All five now pass with --run-perf (14.87s total) and still skip without it. Refs #1160.
Guards both ways the latency benchmarks were unreachable. The
registration check is behavioural rather than source-level —
`config.getoption("--run-perf")` raises if the option is absent, so
reverting to the no-op stub fails it. The budget check walks the AST for
every test calling `_has_run_perf`, resolves its
`@pytest.mark.timeout` override against the `timeout` value read from
pyproject.toml, and fails if the harness could still kill the test
before its own assertion runs.
Scan vacuity is pinned too, since finding no perf gates would satisfy
the budget assertion for free.
Mutation-verified: un-registering the flag fails the first test,
dropping the eigsolve timeout override fails the second, breaking the
scan fails the third.
Refs #1160.
9c0a5de to
b7858dd
Compare
|
Approve — with one correction to the diagnosis. The fix is right either way, but the PR understates the defect. "Always fails" is host-dependent, and that makes it worse, not smallerReproduced here with the eigsolve override removed: 4.81s against the 5.0s global cap — it passes, with 0.19s of margin. The table says 6.76s and "always fails." Both measurements are honest; the pre-fix outcome was simply decided by hardware and runner load rather than by the 30s budget the test asserts. That is a worse failure mode than the one described. A test that always fails gets noticed and fixed. A test that flips at ±0.2s under load becomes a flake, and this repo already has a documented history of exactly that with latency assertions. The justification for the override is stronger stated as the harness cap was deciding the outcome instead of the assertion — which holds on either side of 5s — than as the test could never pass, which holds only on some hosts. Verified independently
Rebased and pushed#1200 merged while this was under review; both add to Now at Notes, none blocking
On the deferred CI jobAgree, and I'd go further than "happy to wire it if you want." Don't. The local PR gate here already false-fails a median-latency assertion on loaded hardware; adding five more wall-clock assertions to a shared runner buys noise, not signal. Making them runnable on demand is the unambiguous improvement. A nightly that cries wolf would be a regression dressed as coverage. The note about |
robotrocketscience
added
ready-to-merge
|
[release:review:Setr:2026-07-30T16:58:08Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged b7858dd → |
1 participant
Third acceptance criterion of #1160 — "Register the perf flag or delete the perf tests." Registers it, and fixes the second defect that only becomes visible once the flag works. The umbrella stays open.
Defect 1 — the opt-in did not exist
Five perf tests across four modules gate a wall-clock assertion on a
_has_run_perfhelper that reads--run-perf. Nothing registered that option.test_bm25_index.pycarried apytest_addoptionstub, but pytest only invokes that hook fromconftest.pyor an installed plugin — never from a plain test module. Sogetoptionalways raised, every helper fell back toFalse, and the flag was not merely ignored:The tests were reachable only by hand-editing the guard. No measurement of retrieval latency at realistic store size has run in a long time.
Defect 2 — registering the flag exposed a test that could not pass
pyproject.tomlsets a globaltimeout = 5, sized for unit tests. Every perf test asserts its own, larger budget, so pytest-timeout decided the outcome ahead of the assertion:timeout = 5test_eigsolve_under_budget_n10kelapsed < 30.0test_build_latency_at_n_10k(hrr)elapsed <= 5.0test_score_latency_under_5ms_n50ktest_probe_latency_at_n_50k(hrr)test_heat_kernel_latency_at_n_50k_under_10msThe eigsolve test's own comment already read "allow 30s for noisy hosts" — it was arguing with the harness and losing. Registering the flag without this fix would have shipped a guaranteed-failing opt-in, which is why both land together.
Each perf test now carries
@pytest.mark.timeout(_PERF_TIMEOUT_S)at 120s — the per-test override conventionpyproject.toml:125-127already documents. The bound is deliberately generous because it is only a hang guard; the budget is the assertion.Verification
--run-perf; full suite6237 passed, 69 skipped(6234 baseline + 3 new).The guard
tests/test_perf_gate_wiring.pypins both failure modes. The registration check is behavioural, not source-level —config.getoption("--run-perf")raises if the option is absent, so a revert to the no-op stub fails it rather than passing a grep. The budget check walks the AST for every test calling_has_run_perf, resolves its@pytest.mark.timeoutoverride, and compares it against thetimeoutvalue read frompyproject.toml, so it tracks that value if it changes instead of hard-coding 5.New module rather than an addition to an existing one, so this can merge in any order relative to #1194 and #1195.
Deliberately not included: a CI job
No workflow runs
--run-perf. I have not added one, and that is a judgement call worth stating rather than burying: these are wall-clock assertions on shared runners, and this repo has a documented history of latency tests false-failing under load. A nightly job is the natural home if that noise is acceptable — but making a flaky perf gate visible is a different decision from making the tests runnable, and only the second is unambiguously an improvement. Happy to wire the nightly if you want it.Note
test_eigsolve_under_budget_n10kis the same failure family as this umbrella's parked finding "the 5 s global timeout equals SQLite'sbusy_timeout, making the lock-contention path structurally untestable" — a global cap chosen for unit tests silently governing tests whose own budgets exceed it. Worth considering together when that one is dispositioned.Summary by Sourcery
Register the pytest --run-perf option and ensure latency benchmarks are governed by their own budgets rather than the global timeout.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
--run-perfoption.