feat: add performance benchmark suite for search, propose, bundle, an…#116
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a complete pytest-based performance benchmark suite that measures search latency, proposal write throughput, bundle operations, and index rebuild time across knowledge bases of 1k, 10k, and 100k claims. The infrastructure includes shared KB fixture generation and four independent benchmark modules that are now executable via ChangesPerformance Benchmark Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/bench_search.py (1)
19-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick win100k search benchmark coverage is missing vs the stated acceptance criteria.
This file only benchmarks 1k/10k, but the objective requires search latency across 1k, 10k, and 100k claims. Add a
store_100kfixture and corresponding FTS5/substring tests so results are complete and comparable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/bench_search.py` around lines 19 - 47, Add a 100k fixture and corresponding benchmark tests: create a pytest.fixture named store_100k(scope="module") that returns _store(kb_100k), then add two tests mirroring the existing ones — test_search_fts5_100k(benchmark, store_100k) that calls benchmark(index_db.search, store_100k.kb_dir, "auth uses JWT", limit=10) and test_search_substring_100k(benchmark, store_100k) that calls benchmark(store_100k.search_substring, "auth", limit=10) so FTS5 and substring fallbacks are measured for 100k like the 1k/10k cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/bench_bundle.py`:
- Around line 37-60: The import benchmarks currently reuse a single dest_kb
(KBStore.init(tmp_path)) so iterations measure overwrite behavior; update
test_bundle_import_1k and test_bundle_import_10k to initialize a fresh KBStore
for each benchmark iteration by wrapping bundle.import_apply in a small callable
that calls KBStore.init(...) inside the benchmark invocation (so each run
imports into a fresh KB instead of overwriting), and add a new
test_bundle_import_100k that mirrors the 1k/10k tests but uses
exported_bundle_100k and the same per-iteration KBStore initialization pattern.
In `@benchmarks/bench_propose.py`:
- Around line 21-35: The benchmark test_propose_claim_latency mutates shared
state by inserting new claims into fresh_store on each iteration, making latency
results non-comparable; change it so each measured invocation sees an equivalent
store state: create a size-parameterized seed (e.g., sizes like 1k/10k/100k),
pre-seed a template store for each size, and in _propose use a fresh copy/clone
of that template store (or re-initialize fresh_store) so propose_claim always
runs against the same initial KB; keep source_id and proposed_by the same but
ensure benchmark(_propose) operates on an isolated per-iteration store so
metrics are stable and follow the required throughput sweep.
In `@benchmarks/conftest.py`:
- Around line 39-41: The subprocess.call currently uses a relative path
"benchmarks/fixtures/gen_kb.py" which makes KB generation cwd-dependent; change
it to compute an absolute path to the generator (e.g., build script_path =
Path(__file__).parent / "fixtures" / "gen_kb.py" and use
str(script_path.resolve())) and pass that absolute path into the
subprocess.check_call invocation (keep sys.executable, and the existing "--out"
and "--claims" args) so the call no longer depends on the current working
directory.
In `@CHANGELOG.md`:
- Line 10: Update the CHANGELOG entry for the "Performance benchmark suite in
`benchmarks/`" so it reflects coverage at 1k/10k/100k (not just 1k/10k) and
amend the example command `pytest benchmarks/ --benchmark-only` to optionally
show the documented JSON output flag (e.g., add
`--benchmark-json=benchmarks/results.json`) so the entry accurately reports
scope and how to produce machine-readable results.
---
Outside diff comments:
In `@benchmarks/bench_search.py`:
- Around line 19-47: Add a 100k fixture and corresponding benchmark tests:
create a pytest.fixture named store_100k(scope="module") that returns
_store(kb_100k), then add two tests mirroring the existing ones —
test_search_fts5_100k(benchmark, store_100k) that calls
benchmark(index_db.search, store_100k.kb_dir, "auth uses JWT", limit=10) and
test_search_substring_100k(benchmark, store_100k) that calls
benchmark(store_100k.search_substring, "auth", limit=10) so FTS5 and substring
fallbacks are measured for 100k like the 1k/10k cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb21cc8b-bd5b-49c3-bbe9-2a92fd5541cf
📒 Files selected for processing (6)
CHANGELOG.mdbenchmarks/bench_bundle.pybenchmarks/bench_index_rebuild.pybenchmarks/bench_propose.pybenchmarks/bench_search.pybenchmarks/conftest.py
| def test_bundle_import_1k(benchmark, exported_bundle_1k, tmp_path): | ||
| dest_kb = KBStore.init(tmp_path) | ||
| benchmark( | ||
| bundle.import_apply, | ||
| dest_kb.kb_dir, | ||
| exported_bundle_1k, | ||
| on_conflict="overwrite", | ||
| actor="bench", | ||
| ) | ||
|
|
||
|
|
||
| def test_bundle_export_check_1k(benchmark, exported_bundle_1k): | ||
| benchmark(bundle.export_check, exported_bundle_1k) | ||
|
|
||
|
|
||
| def test_bundle_import_10k(benchmark, exported_bundle_10k, tmp_path): | ||
| dest_kb = KBStore.init(tmp_path) | ||
| benchmark( | ||
| bundle.import_apply, | ||
| dest_kb.kb_dir, | ||
| exported_bundle_10k, | ||
| on_conflict="overwrite", | ||
| actor="bench", | ||
| ) |
There was a problem hiding this comment.
Bundle import benchmark currently measures overwrite-on-existing-KB behavior and misses 100k coverage.
dest_kb is initialized once, so repeated benchmark iterations mostly measure import+overwrite conflict handling rather than fresh import cost. Also, imports are only benchmarked for 1k/10k despite the stated 1k/10k/100k requirement.
Suggested fix sketch
def test_bundle_import_1k(benchmark, exported_bundle_1k, tmp_path):
- dest_kb = KBStore.init(tmp_path)
- benchmark(
- bundle.import_apply,
- dest_kb.kb_dir,
- exported_bundle_1k,
- on_conflict="overwrite",
- actor="bench",
- )
+ def _run():
+ dest_kb = KBStore.init(tmp_path / "run_1k")
+ return bundle.import_apply(dest_kb.kb_dir, exported_bundle_1k, on_conflict="overwrite", actor="bench")
+ benchmark(_run)Mirror this for 10k and add a 100k variant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/bench_bundle.py` around lines 37 - 60, The import benchmarks
currently reuse a single dest_kb (KBStore.init(tmp_path)) so iterations measure
overwrite behavior; update test_bundle_import_1k and test_bundle_import_10k to
initialize a fresh KBStore for each benchmark iteration by wrapping
bundle.import_apply in a small callable that calls KBStore.init(...) inside the
benchmark invocation (so each run imports into a fresh KB instead of
overwriting), and add a new test_bundle_import_100k that mirrors the 1k/10k
tests but uses exported_bundle_100k and the same per-iteration KBStore
initialization pattern.
| def test_propose_claim_latency(benchmark, fresh_store, source_id): | ||
| """propose_claim() write latency — sits in the agent hot loop.""" | ||
| counter = [0] | ||
|
|
||
| def _propose(): | ||
| counter[0] += 1 | ||
| return propose_claim( | ||
| fresh_store, | ||
| text=f"benchmark claim {counter[0]}", | ||
| evidence=[source_id], | ||
| proposed_by="bench-agent", | ||
| ) | ||
|
|
||
| result = benchmark(_propose) | ||
| assert result is not None |
There was a problem hiding this comment.
This benchmark mixes changing workload state, so latency numbers are not comparable.
Line 23+ keeps inserting new claims into the same store during one benchmark run, so later iterations are measuring a larger KB than earlier ones. That skews a single reported metric and weakens regression tracking. It also does not implement the 1k/10k/100k throughput sweep from the objective.
Suggested direction
- def test_propose_claim_latency(benchmark, fresh_store, source_id):
+ `@pytest.mark.parametrize`("kb_size", ["1k", "10k", "100k"])
+ def test_propose_claim_latency(benchmark, kb_size, ...):
...
- result = benchmark(_propose)
+ # Keep per-iteration state constant (fresh destination store each invocation/round)
+ result = benchmark(_propose)Use a size-parametrized seed input and ensure each measured invocation starts from equivalent state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/bench_propose.py` around lines 21 - 35, The benchmark
test_propose_claim_latency mutates shared state by inserting new claims into
fresh_store on each iteration, making latency results non-comparable; change it
so each measured invocation sees an equivalent store state: create a
size-parameterized seed (e.g., sizes like 1k/10k/100k), pre-seed a template
store for each size, and in _propose use a fresh copy/clone of that template
store (or re-initialize fresh_store) so propose_claim always runs against the
same initial KB; keep source_id and proposed_by the same but ensure
benchmark(_propose) operates on an isolated per-iteration store so metrics are
stable and follow the required throughput sweep.
| subprocess.check_call( | ||
| [sys.executable, "benchmarks/fixtures/gen_kb.py", | ||
| "--out", str(out), "--claims", str(claims)], |
There was a problem hiding this comment.
Use an absolute generator script path to avoid cwd-dependent failures.
subprocess.check_call currently depends on the process working directory. If invoked from another cwd, KB generation can fail at runtime.
Suggested fix
def _gen_kb(tmp_path_factory, *, claims: int) -> Path:
out = tmp_path_factory.mktemp(f"kb{claims}")
+ script = Path(__file__).resolve().parent / "fixtures" / "gen_kb.py"
subprocess.check_call(
- [sys.executable, "benchmarks/fixtures/gen_kb.py",
+ [sys.executable, str(script),
"--out", str(out), "--claims", str(claims)],
)
return out🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/conftest.py` around lines 39 - 41, The subprocess.call currently
uses a relative path "benchmarks/fixtures/gen_kb.py" which makes KB generation
cwd-dependent; change it to compute an absolute path to the generator (e.g.,
build script_path = Path(__file__).parent / "fixtures" / "gen_kb.py" and use
str(script_path.resolve())) and pass that absolute path into the
subprocess.check_call invocation (keep sys.executable, and the existing "--out"
and "--claims" args) so the call no longer depends on the current working
directory.
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - Performance benchmark suite in `benchmarks/` covering search latency, proposal write throughput, bundle export/import/verify round-trips, and index rebuild time at 1k/10k claim sizes. Run with `pytest benchmarks/ --benchmark-only`. |
There was a problem hiding this comment.
Align changelog benchmark scope and command with stated objectives.
This line says coverage is 1k/10k only, but the PR objective/acceptance criteria call out 1k/10k/100k in the single-command suite. Please update the wording (and optionally include the documented JSON output command) so release notes don’t under-report benchmark scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 10, Update the CHANGELOG entry for the "Performance
benchmark suite in `benchmarks/`" so it reflects coverage at 1k/10k/100k (not
just 1k/10k) and amend the example command `pytest benchmarks/ --benchmark-only`
to optionally show the documented JSON output flag (e.g., add
`--benchmark-json=benchmarks/results.json`) so the entry accurately reports
scope and how to produce machine-readable results.
|
@plind-junior , you may review this now |
|
@plind-junior , can you give this a check? |
|
All issues or conflicts have all been fixed |
|
Create the PR against test branch |
|
One thing worth a follow-up before this is the safety net it's meant to be: |
|
Functionality is great — could the diff be a bit slimmer? Most of the +422 is the same shape repeated across the three kinds. A small helper would cut it noticeably without hurting clarity. The biggest win is in Same idea on the test side: the orphan-claim / orphan-page / orphan-entity tests likely have near-identical setup, so a single Tiny extra: the three "if X not in claims → dangling finding" loops in |
|
@plind-junior , could you reopen this once again, then I would fix the conflicts right away |
|
PR should target the test branch, not main |
…d index benchmarks/ had a KB generator (gen_kb.py) and a detailed README but no timing harness. Add four benchmark modules covering: - bench_search.py: FTS5 and substring search latency at 1k/10k claims - bench_propose.py: propose_claim() write latency (agent hot loop) - bench_bundle.py: export, import, verify round-trips at 1k/10k claims - bench_index_rebuild.py: index rebuild time at 1k/10k claims Session-scoped fixtures in conftest.py generate synthetic KBs once per session via gen_kb.py to keep benchmark startup cost low. Run with: pytest benchmarks/ --benchmark-only --benchmark-json=bench.json Closes vouchdev#99
Tet-9
force-pushed
the
feat/99-performance-benchmarks
branch
from
56a512f to
8cffebe
Compare
Fixed |
ReviewSummary: Adds the missing pytest-benchmark harness that issue #99 called for — five modules covering search, propose, bundle, and index-rebuild. The structure is clean and the module-scoped fixtures are the right call for keeping wall-clock startup cost low. A few gaps vs the acceptance criteria and one path portability issue need addressing. What works
Suggestions
VerdictRequest changes — Two acceptance-criteria gaps (100k coverage and committed baseline) and one portability bug in the fixture path are blocking. The core harness design is solid; these are targeted fixes. |
2 participants
Summary
benchmarks/ had a KB generator (gen_kb.py) and a detailed README describing exactly what to measure, but no timing harness and no published numbers. This adds the missing harness.
Changes
conftest.py — session-scoped fixtures that generate synthetic KBs once per session via gen_kb.py at 1k, 10k, and 100k claim sizes, keeping benchmark startup cost low across all modules.
bench_search.py — FTS5 and substring fallback search latency at 1k and 10k claim sizes. Captures the performance curve as the KB grows, which is the number that matters most for search.
bench_propose.py — propose_claim() write latency. This sits directly in the agent hot loop. Any regression past ~50ms on a warm SSD is a signal that something needs attention.
bench_bundle.py — export, import, and verify round-trips at 1k and 10k claims. Bundle imports gate cross-team KB sharing so a 10k-claim bundle should land in seconds, not minutes.
bench_index_rebuild.py — vouch index rebuild time at 1k and 10k claims. Measures the full reset-and-reindex path.
Usage
Run with:
pytest benchmarks/ --benchmark-only --benchmark-json=bench.json
Benchmarks live outside tests/ so a regular pytest run never pulls them in. Requires pytest-benchmark.
Closes #99
Summary by CodeRabbit
Tests
pytest benchmarks/ --benchmark-only.Documentation