Add CI benchmark suite: latency + recall regression tracking - #446
Add CI benchmark suite: latency + recall regression tracking#446Sravan1011 wants to merge 24 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…emoss#435) Adds an automated benchmark harness under benchmarks/ci/ that runs on every push/PR to main, records p50/p95/p99 latency and recall@5/@10 against a fixed 1K-doc corpus, and fails the build when results regress past configurable thresholds versus a checked-in baseline. - test_bench_ci_moss.py: pytest suite (latency, recall, regression guard, JSON results writer) - conftest.py: CLI flags for output path, baseline file, and thresholds - generate_ground_truth.py + ground_truth.json: pre-computed expected results per query (top_k=50, moss-minilm) - baseline.json: placeholder baseline; latency guard skips until a real baseline is captured from CI hardware - .github/workflows/benchmark.yml: CI job with artifact upload and a manual update_baseline dispatch input Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 12 files
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Share QUERIES/index config between tests and generator via bench_queries.py so the sets can never drift - Check index existence via list_indexes() instead of a broad except around get_index, so auth/network errors surface rather than silently triggering index creation - Add --recreate flag to generate_ground_truth.py to rebuild the index after corpus/model changes before capturing ground truth - Run baseline-update workflow dispatches without the regression comparison so an intentional perf change can't fail its own baseline refresh; clarify that persisting requires committing the artifact - Activate the recall guard: baseline now carries measured recall (hardware-independent); latency stays placeholder until a CI- runner baseline is committed - Pin benchmark dependencies exactly to keep results attributable to repo changes - Docs: fix non-runnable shell example, qualify CI coverage (fork PRs skip), document ground truth as a ranking-stability reference Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a CI-oriented benchmark harness under benchmarks/ci/ to measure Moss query latency (p50/p95/p99) and retrieval stability (recall@5/@10) per commit, persist results to JSON, and optionally gate PRs/commits on regressions vs a checked-in baseline. This fits the repo’s broader goal of keeping the on-device runtime fast and stable across SDK/native changes.
Changes:
- Introduces a pytest-based benchmark suite that builds/loads a fixed CI index, measures latency + recall, compares to
baseline.json, and writesbenchmark_results.json. - Adds a dedicated GitHub Actions workflow to run the suite on push/PR and upload per-commit benchmark artifacts (with a manual baseline-refresh mode).
- Documents the benchmark suite and links it from existing benchmark/agent guidance docs; updates ignores for generated outputs.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
benchmarks/README.md |
Links the main benchmark docs to the new CI benchmark suite and describes behavior at a high level. |
benchmarks/ci/test_bench_ci_moss.py |
Core pytest suite: index setup, latency/recall measurement, regression guards, and JSON writer. |
benchmarks/ci/requirements.txt |
Minimal pinned deps for running the benchmark suite from repo root. |
benchmarks/ci/README.md |
Usage and operational documentation for running benchmarks, guards, and baseline updates. |
benchmarks/ci/ground_truth.json |
Committed ground-truth doc IDs per query for recall computation. |
benchmarks/ci/generate_ground_truth.py |
Script to (re)generate ground_truth.json when corpus/model changes. |
benchmarks/ci/conftest.py |
Adds pytest CLI flags for output paths and regression thresholds. |
benchmarks/ci/bench_queries.py |
Shared benchmark constants (queries, model id, index name, doc count). |
benchmarks/ci/baseline.json |
Initial placeholder baseline for regression comparison (recall active; latency intentionally zero). |
benchmarks/.gitignore |
Ignores CI benchmark __pycache__ and generated benchmark_results.json within benchmarks/. |
AGENTS.md |
Documents the new benchmark workflow in agent guidance. |
.gitignore |
Broadens .venv ignore pattern and ignores benchmark_results.json at repo root. |
.github/workflows/benchmark.yml |
New workflow that runs the benchmark suite and uploads results artifacts; supports manual baseline refresh behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for q in QUERIES: | ||
| expected_ids = ground_truth.get(q, []) | ||
| if not expected_ids: | ||
| continue |
| output_path = request.config.getoption("--benchmark-output") | ||
| with open(output_path, "w") as f: | ||
| json.dump(benchmark_results, f, indent=2) |
| help="Max allowed absolute decrease in recall@k vs baseline " | ||
| "(default: 0.05 = 5 percentage points)", |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Restore indentation of the lazy moss import inside the moss_client fixture (auto-applied suggestion dedented it, breaking collection) - Fail fast when a benchmark query has no ground-truth entry instead of silently shrinking the evaluated set and inflating recall - Create parent directories for --benchmark-output before writing - Correct --recall-threshold help text (guard compares recall@5) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex reviewNo issues found. |
…index Addresses two blocking review findings: 1. Missing credentials previously skipped unconditionally, so a missing or misconfigured secret on push/same-repo PRs turned the workflow into a green no-op. The workflow now exports ALLOW_BENCHMARK_SKIP=1 only for fork PRs (which cannot read repository secrets); in any other CI run, absent credentials pytest.fail() with an actionable message. Local runs (no CI env) still skip. The credential gate also moved above the moss import so fork PRs skip cleanly even where native bindings are unavailable. 2. The fixed benchmark-ci index name could silently reuse remote state built from a different corpus, DOC_COUNT, or model, benchmarking stale data. The index name is now derived from a content signature (model id + DOC_COUNT + the exact corpus slice, sha256-truncated): changed inputs produce a different name and the index is rebuilt. generate_ground_truth.py derives the same name and embeds the signature in ground_truth.json; the recall fixture fails with a regenerate hint when the stored signature does not match the current inputs. The signature and index name are also recorded in the results artifact for traceability. MOSS_INDEX_NAME remains an explicit override. Verified all three credential paths locally: local no-creds skips, CI=true no-creds fails with the configured message, and ALLOW_BENCHMARK_SKIP=1 skips. ground_truth.json restamped with the computed signature (73d5e83176f7). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ne compatibility Addresses three review findings: - BLOCKING: missing required inputs (corpus file, ground_truth.json) used the same unconditional pytest.skip as optional paths, so trusted CI could go green with the whole suite skipped. A shared _missing_required_input() helper now applies the credentials policy everywhere: skip on fork PRs (ALLOW_BENCHMARK_SKIP=1) and local runs, fail in trusted CI. An explicitly provided --baseline-file that does not exist now fails outright in every environment: asking for a regression comparison and silently not getting one is a config error. - CONSIDER (concurrency): all benchmark jobs share one Moss project and the deterministic benchmark-ci-<hash> index, so parallel PR/push runs could race on first index creation and contaminate each other's P95. The workflow now uses a global concurrency group with cancel-in-progress: false so runs queue instead of overlapping. - CONSIDER (baseline/query-set compatibility): the signature only covered corpus/model inputs. bench_queries now exposes query_set_hash(); ground_truth.json embeds it (recall fails with a regenerate hint when QUERIES drifted), benchmark results record it in config, and both regression guards fail via _assert_baseline_compatible() unless the baseline's config matches the current run on signature, query-set hash, doc count, rounds, and top_k. baseline.json and ground_truth.json restamped accordingly; index_name is deliberately excluded from the comparison since MOSS_INDEX_NAME may override it without changing what is measured. Verified locally: local no-creds skips, CI=true no-creds fails, fork-PR skips, and a provided-but-missing baseline file fails both guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Hosted runners occasionally drop the connection mid-wheel-download (urllib3 IncompleteRead / ProtocolError), and pip does not resume or retry a broken stream. Retry the requirements install up to 3 times with a short backoff before failing the job. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… zero latency baseline Addresses two review findings: - BLOCKING: --recreate deleted whatever MOSS_INDEX_NAME pointed at, so a developer with a shared or production Moss project in their env could destroy a non-benchmark index. Deletion is now guarded three ways: the derived benchmark-ci-<signature> name deletes without confirmation, an overridden name inside the benchmark-ci-* namespace additionally requires the new --force flag, and names outside that namespace are refused even with --force. - CONSIDER: the checked-in zero latency baseline made test_no_latency_regression skip on every run, leaving the latency guard silently inactive until someone manually armed it. A zero baseline now FAILS comparison runs with the arming procedure in the message. update_baseline dispatch runs are unaffected (they do not pass --baseline-file), and fork PRs skip earlier at the credentials gate, so the failure lands exactly on trusted runs that should be enforcing the guard. baseline.json note and README updated; the first trusted CI run's artifact is the natural baseline to commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous fail-on-zero behavior made every trusted run red until a CI-captured baseline was committed — and this PR cannot ship one, since it runs from a fork without secrets. Per review, the comparison is now gated on the baseline file itself: - baseline.json declares latency_guard: unarmed — the latency test skips with a loud arming message while the recall guard stays active (dropping --baseline-file entirely would have disabled recall too) - committing a CI-captured benchmark-results-<sha> artifact as baseline.json arms the guard automatically: artifacts carry no latency_guard flag - a zero p95 WITHOUT the unarmed marker still fails as a misconfigured baseline, and a non-zero p95 WITH the marker fails as inconsistent, so the guard can never be silently inactive by accident Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssing The guards previously skipped on absent latency/recall data, so a run with --baseline-file could go green without ever measuring anything (e.g. a random-ordering plugin collecting a guard before the measurement tests, or measurement crashing in a way that left results empty). Both guards now route missing measurement data through the shared skip-or-fail policy: skip on fork PRs and local runs where measurement legitimately cannot happen, FAIL in trusted CI where a silently passing regression gate is a green no-op. The measure-guard-write ordering comment updated to match: random ordering now fails loudly in trusted CI by design. Verified: local+baseline skips, CI=true without measurements fails both guards, fork PR skips. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite computes and stores recall_at_10 in both results and the baseline, but the guard only compared recall_at_5 — a change that preserves the top 5 while dropping documents ranked 6-10 would pass CI with recall@10 silently regressed. The guard now loops over both metrics with the same threshold, reports each in the log, and lists every regressed metric in the failure message. conftest help text and README updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Sravan1011 address the comments |
…in sessionfinish Index staleness (blocking finding): the index name now embeds a build fingerprint (installed SDK/bindings versions + Python SDK source tree hash) alongside the corpus/model signature. A PR that changes create_index, document serialization, or the index/model build path gets a new index name, forcing a rebuild that exercises that path — while ground truth stays keyed to data inputs only, so an SDK change that shifts rankings surfaces as a recall regression instead of passing against stale embeddings. generate_ground_truth.py gains --prune to clean up superseded benchmark-namespace indexes. Results artifact: serialization moved from a TestWriteResults test to pytest_sessionfinish in conftest.py, so the artifact is written even under -x/--maxfail, ordering plugins, or a failing regression guard, and a stub documents sessions where nothing was measured. Also satisfies the extended ruff rule set on latest ruff (import sorting, no blocking open() in async code, narrow exception in _git_sha, executable bit on the generator script). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dering Build fingerprint (blocking finding): now hashes the on-disk content of every file in the installed moss and inferedge-moss-core distributions (Python sources, data, and native .so/.pyd bindings) instead of just their version strings. A native binding rebuilt without a version bump — e.g. a locally compiled wheel, or a binding change shipped under an unchanged pin — previously kept the old fingerprint and could reuse a stale index; it now changes the file bytes and forces a rebuild. MossClient construction moved inside the moss_client fixture's _setup() coroutine, after _run() has created and installed the shared event loop, so any async initialization the client performs binds to the loop it will actually run on. Made python-dotenv optional in test_bench_ci_moss.py (try/except ModuleNotFoundError) so a repo-wide pytest run that collects this module doesn't hard-fail on an unrelated missing dependency before the credential-based skips even run. Extended ALLOW_BENCHMARK_SKIP to also cover same-repo Dependabot PRs, which GitHub denies normal secrets to just like fork PRs, but which the previous fork-only check would have made fail as if secrets were misconfigured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@HarshaNalluru now good to go for the merge |
Closes #435
What this adds
An automated benchmark harness (
benchmarks/ci/) that runs on every push/PR tomain, records p50/p95/p99 latency and recall@5/@10 per commit, and fails the build when results regress past configurable thresholds versus a checked-in baseline.How it works
TestBenchmarkLatencymoss-minilm,top_k=5TestBenchmarkRecallground_truth.json, generated withtop_k=50); reports recall@5 and recall@10TestRegressionGuardbaseline.json(both thresholds configurable via CLI flags)TestWriteResultsThe GitHub Actions workflow (
.github/workflows/benchmark.yml) uploadsbenchmark_results.jsonas an artifact (benchmark-results-<sha>, 90-day retention) on every run — including failed ones — so numbers accumulate per commit and can be published/graphed later.Files
benchmarks/ci/test_bench_ci_moss.py— the suite (latency, recall, guard, writer)benchmarks/ci/conftest.py— pytest CLI flags (--benchmark-output,--baseline-file,--latency-threshold,--recall-threshold)benchmarks/ci/generate_ground_truth.py— regeneratesground_truth.jsonwhen the corpus/model changesbenchmarks/ci/ground_truth.json— committed ground truth for the 15 benchmark queriesbenchmarks/ci/baseline.json— performance baseline (placeholder for now, see below).github/workflows/benchmark.yml— CI job withworkflow_dispatch+update_baselineinputbenchmarks/ci/README.md— full usage docs; top-levelbenchmarks/README.mdandAGENTS.mdlink to itVerification
Tested end-to-end in a
python:3.12-slim-trixiecontainer (same Python as the workflow), invoked exactly as CI invokes it from the repo root:pip install -r benchmarks/ci/requirements.txtfrom repo root installs the local SDK + deps4 passed, 1 skipped in 43s(skip = latency guard, expected with placeholder baseline)P95 latency regressed by +181%,Recall@5 dropped by 0.0600 (threshold 0.0500))if: always()) has dataNotes for reviewers
baseline.jsonis intentionally a placeholder. Latency numbers are hardware-dependent, so the first real baseline should come from CI: trigger the workflow once after merge, download thebenchmark-results-<sha>artifact, and commit it asbenchmarks/ci/baseline.json. Until then the latency guard skips (recall guard is active but trivially passes against a zero baseline).update_baselinedispatch input does not auto-commit — the workflow hascontents: read, so it copies the results overbaseline.jsonon the runner and prints them; persisting is a documented manual commit. Happy to add an auto-commit step (needscontents: write) if maintainers prefer.MOSS_PROJECT_ID/MOSS_PROJECT_KEYsecrets the suitepytest.skips rather than erroring, so external contributors' PRs won't break — at the cost of not benchmarking them.moss-minilmmodel-download timeout was observed across many local runs; if it shows up in CI,pytest-rerunfailureswould be a cheap hardening follow-up.