fix(metrics): stop publishing precision under upstream's mean_precisions key - #249
Merged
Merged
Conversation
Our results JSON emitted `mean_precisions` carrying hits/|results returned|. Upstream qdrant/vector-db-benchmark emits a key of the SAME name defined as len(ids & expected[:top])/top — recall@top. An engine returning 5 of 10 requested results, all correct, reported 0.50 upstream and 1.00 here, so overlaying the two tools' files on one chart silently compared precision against recall. Renamed rather than dual-emitted. Keeping `mean_precisions` while adding a second field would leave the colliding name in place — the one option the issue rules out — and an upstream-compatible hits/top aggregate cannot be added without a fifth per-query vector threaded through all 15 engine harnesses (elasticsearch.rs / opensearch.rs are frozen behind PRs #246/#248). Instead the upstream formula gets a canonical implementation (QueryMetrics::recall_at_top, test-pinned), and every result file now says whether our `mean_recall` IS upstream's number for that dataset. Emitted key changes (schema version 2): mean_precisions -> mean_precision_at_returned precisions -> precisions_at_returned (--dump-raw-latencies only) precision_dist -> precision_at_returned_dist + top-level metrics_schema block: formulas, ground-truth width profile, and comparable_to_upstream_mean_precisions Calibrator: it binary-searches on mean_precision_at_returned, whose denominator is results returned, so a 0.95 target is unreachable by construction when the ground truth is narrower than `top`. On the shipped h-and-m-2048-angular tests.jsonl all 10 000 rows are narrower than top=100 (931 have fewer than 25 neighbours), capping that metric at 0.233 — the sweep used to converge on ef=1000 and print it as a success. It now warns up front with the arithmetic, warns again on termination, and records params.calibration.reached_target in the result file. Also: plot.rs reads both the new and legacy key so existing summaries still chart; scripts/v0_check.sh now compares Python `mean_precisions` against Rust `mean_recall` (it was comparing recall with precision); the dead PyO3 accelerators carry a note about the same mismatch. Closes #217 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ths, and stop paying for the profile on --skip-search Review follow-ups on #217. Verified, not assumed: the only two shipped configs that calibrate (cohere-calibration.json, dbpedia-calibration.json, both 0.95 at top=100) target datasets whose ground truth is FULL width, so no published cohere-cal-*/dbpedia-cal-* number is a fake calibration: cohere-768-1M 10000 x 100, all rows full, ceiling 1.000 dbpedia-openai-1M-{512,1024,1536, 2048,3072}-angular 1000 x 100, all rows full, ceiling 1.000 The suspicion that the base DBpedia HDF5 lacked 100 neighbours (because a separate *-100neighbors entry exists) is disproved — that entry is a different packaging, not a repair. h-and-m is a latent trap no shipped config steps in: its ceiling is 0.2334 at top=100 and 0.9337 at its natural top=25, so a 0.95 target is unreachable there at either top (reachable only at top=10, 0.9592). README: both H&M-2048 rows claimed 100 neighbours and 2000 test queries. Measured: with-filters 10000 queries of width 1..25 (mean 23.4), no-filters 10000 queries of width 10. Added a footnote explaining that this column bounds what recall can mean and how the two tools' denominators differ. --skip-search no longer loads the ground-truth profile nothing will consume (~22 s warm / ~137 s cold per config on the compound h-and-m dataset). Also: summaries now carry metrics_schema_version, metrics_definitions and uncalibrated_configs, and plot.rs warns when a summary predates the marker — our pre-v2 files and upstream's files are otherwise indistinguishable and would be charted on one axis. Unreached calibration targets are repeated at the end of the run, not only before a sweep that scrolls them away. Cosmetic review notes: unreachable_target_note now excludes only NaN (+inf is unreachable and must be flagged), and the "exactly when" claim on recall_matches_upstream is softened to name the two pathological shapes (interleaved sentinels, a page wider than top) that can still separate the numbers at full width. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker Build ValidationPlatforms tested:
Tests performed:
|
1 similar comment
Docker Build ValidationPlatforms tested:
Tests performed:
|
This was referenced Aug 8, 2026
fcostaoliveira
added a commit
that referenced
this pull request
Aug 9, 2026
…nce Feb 2026 (#297) Closes #291. `src/vectorsets/` was a PyO3 extension that has not compiled since February 2026. `ebd4a5c` (2026-02-27) moved it out of a crate root that declared it and, in the same commit, added the real `engine/vectorsets.rs`; the new `src/lib.rs` never carried the declaration over. Appending `pub mod vectorsets;` today gives **190 errors**, the first being `error[E0433]: cannot find module or crate 'pyo3'` — and `pyo3` is in neither `Cargo.toml` nor `Cargo.lock`. The directory is unbuildable, not merely unreferenced. It still contained `VADD idx` / `VSIM idx`, the hardcoded key #236 spent a PR removing everywhere else, and it kept attracting edits: two repo-wide refactors touched it after it died, the most recent (`1282eda`, #249) **162 days** later. Deadness established six ways and independently re-derived: nothing declares it (`src/vectorsets/mod.rs` exists, so a declaration *would* pick it up — hence the compile proof); no reference to any of its three public types anywhere, including `tests/`, `v0/` and the `fuzz/` crate, which has its own `[workspace]` and path-depends on the root lib so it can only reach what `src/lib.rs` exports; no packaging or CI path picks it up. The one literal `src/vectorsets` hit outside the module is `Dockerfile:26`, a `mkdir -p` fabricating a stub tree for dependency caching, before `rm -rf src` at :39 and the real `COPY src` at :43 — confirmed by an actual `docker build`, which passes. Also removes the module's own entry from `memory-bank/standards/coding-standards.md`'s tree map, which would otherwise describe a directory that no longer exists. `src/redisearch/`'s block stays — that directory still exists and goes with #296. Delta zero on every gate, which is the correct signature for deleting never-compiled files: 919 unit tests before and after, `overhead_invariants` 9/9 both sides, `integration_redis` 41 both sides, fmt and clippy clean. Files under `src/` go 62 → 58 against the guard's `> 20` floor; backslash-continuations are unchanged at 307 because the deleted files contain none. **One thing here is not delta-zero.** #264 carries `const UNCOMPILED = &["src/redisearch/", "src/vectorsets/"]` with a bidirectional assertion added after this PR was opened, so once this lands #264 must delete the `"src/vectorsets/"` row before it can go green. That is one string, and it is recorded on both PRs. #275 remains open as the umbrella: #291 and #296 are the two halves of its item 1, and its items 2-4 — `RedisConfig::from_env`, `WEAVIATE_USE_GRAPHQL`, a stale comment — are untouched. `from_env`'s remaining ten non-test call sites are all inside `src/redisearch/`, so its item 2 unblocks with #296, not here. Reviewed by three adversarial agents. 5 files, 1,426 deletions, **zero additions**.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #217.
The bug
Our results JSON emitted
"mean_precisions"carryingresults.mean_precision=hits / |results returned|. Upstreamqdrant/vector-db-benchmark(engine/base_client/search.py:49-52) emits a key of the same name defined aslen(ids & expected[:top]) / top— recall@top, which is ourmean_recall.An engine returning 5 of 10 requested results, all correct, reported
mean_precisions = 0.50upstream and1.00here. The shared key name actively invites overlaying the two tools' files on one chart, which silently compared precision against recall.Rename, not dual-emit — and why
The issue offers "rename" and/or "additionally emit an upstream-compatible
recall_at_top". This PR renames, and deliberately does not keepmean_precisionsalive:mean_precisions— with either definition — leaves a key that means two things across the two tools. Re-pointing it at recall@top would be worse: our own historical files would then hold two different quantities under one name.mean_recall_at_topaggregate cannot be added without breaking the query→ground-truth pairing. There appears to be a free derivation:recall_at_top_i = recall_i · t_i / k, reusing the per-queryresults.recallswe already keep and the per-row truth widthst_ithatGroundTruthProfilealready loads. It does not work, and the reason is structural rather than cosmetic: every engine's parallel harness pulls query indices off a shared atomic counter and merges per-thread sample buffers in thread-join order (e.g.redis.rs:2416-2424).recalls[j]therefore has no correspondence to ground-truth rowj— the pairing is destroyed before aggregation. Recovering it means either re-keying every engine's sample buffers by query index or threading a fifth per-query vector through all 15 harnesses, two of which (elasticsearch.rs,opensearch.rs) are frozen behind PRs fix(opensearch): force-merge to one SEARCHABLE segment, matching elasticsearch (#210) #246 and fix(elasticsearch): disable periodic refresh during bulk upload, matching OpenSearch and upstream (Closes #240) #248.So instead: the upstream formula gets one canonical, test-pinned implementation (
QueryMetrics::recall_at_topinsrc/metrics.rs), and every result file names the field that IS upstream's number, when one is.comparable_to_upstream_mean_precisions: "mean_recall"is not a hint that the two are similar — it asserts that ourmean_recalland upstream'smean_precisionsare the same quantity computed the same way for this dataset andtop, and may be overlaid directly. It isnullexactly when no field in the file can be.Emitted key changes (schema version 2)
results.mean_precisionsresults.mean_precision_at_returnedhits / |deduped results kept|results.precisions(only under--dump-raw-latencies)results.precisions_at_returnedresults.precision_distresults.precision_at_returned_distmetrics_schema(new, top-level)comparable_to_upstream_mean_precisionsparams.calibration(new, when calibrating)reached_target, ceiling, notemean_recall,recall_p10,recall_dist,mrr/ndcgand every timing field are unchanged. Summary files rename the same key insidesearch_results[]and additionally carrymetrics_schema_version: 2,metrics_definitions, anduncalibrated_configs.Which published numbers are affected, and what stops being comparable
mean_precision_at_returnedis bit-for-bit the number that used to be calledmean_precisions; the filter-only-1.0sentinel is unchanged. Nothing about how any engine is measured moved.mean_precisionsvalue was precision, not recall@top. Any chart, table or dashboard that put ourmean_precisionson the same axis as upstream's (or Python v0's) was comparing precision to recall. Where the ground truth is full width and the engine returned a full page, the two coincide numerically, so most published dense-benchmark curves are unaffected in value. Where they diverge — short/filtered ground truth, or engines returning fewer thantop— the old overlays were wrong and re-reading them againstmean_recallis the fix.mean_precisionsis today'smean_precision_at_returned.--charthandles this itself (plot.rsreads the new key, falling back to the legacy one, and warns when a summary lacksmetrics_schema_versionbecause our pre-v2 files and upstream's files are then indistinguishable). Any external SQL/notebook selectingresults.mean_precisionsfrom new files gets NULL and must be pointed atmean_precision_at_returned— or atmean_recall, if what it wanted was the upstream-comparable quantity.v0/tools/upload_results_postgres.shloads.results.mean_precisionsinto amean_precisionscolumn; it now falls back to.results.mean_recallfor Rust files, which is the quantity that column has always held for Python v0 rows. Rows previously loaded from Rust files into that column held precision and are not comparable with the Python-loaded rows.scripts/v0_check.shcompared Pythonmean_precisionsagainst Rustmean_precisions— i.e. recall against precision. It now maps to Rustmean_recall. It passed only because its datasets have full-width ground truth.Calibrator (second half of the issue) — and no, the shipped calibrations are not fiction
calibrate()binary-searches onmean_precision_at_returned, whose denominator is results returned. On a dataset whose ground truth is narrower thantop, the metric is hard-capped and the target is unreachable no matter how highefgoes — the sweep walkedefto the 1000 bound and printedCalibrated ef=1000 → precision=0.24as if it had succeeded.Are any shipped/published calibration numbers actually affected? No — verified, not assumed. Only two shipped configs set
calibration_precision(both 0.95 at"top": 100):cohere-calibration.jsonanddbpedia-calibration.json. Both were profiled directly from the real dataset files:cohere-768-1M(full 2.9 GB download, decompressed)dbpedia-openai-1M-{512,1024,1536,2048,3072}-angular(HDF5neighborsread over HTTP range requests, ~3 MB each)So a 0.95 target is fully reachable on both, and no published
cohere-cal-*/dbpedia-cal-*number is a fake calibration. The suspicion that the base DBpedia HDF5 lacked 100 neighbours (raised because a separatedbpedia-openai-1M-1536-angular-100neighborsentry exists) is disproved: the base file has 1000×100 full-width ground truth; the-100neighborsentry is a different packaging (tar), not a repair.The h-and-m case is therefore a latent trap that no shipped config currently steps in, not evidence of a fake published number. It is still worse than hypothetical, in two ways:
h-and-m-2048-angular/hnm/tests.jsonl, all 10,000 rows are narrower than 100 (931 narrower than 25; min 1, max 25), so attop: 100the ceiling is 0.2334 — and no shipped config pairs h-and-m with calibration only because none happens to.top: 25(every other shipped config leavestopunset, so it is derived from the ground-truth row width) the ceiling is 0.9337 — still below 0.95. Any 0.95 calibration on h-and-m is unreachable at eithertop. It only becomes reachable attop: 10, where the ceiling is 0.9592.What the code now does:
ground_truth.rs) and the target is bounded against it before the first search, printing the full arithmetic when it is unreachable;params.calibration.reached_targetper search file anduncalibrated_configsin the summary — so a CI sweep that "calibrated" against an unreachable target no longer exits 0 with a silent artifact.The ceiling is treated as a warning, not a hard error: a filtered query that legitimately returns fewer than
topresults can score above it, so failing the run would break valid filtered-dataset calibrations. A--fail-on-uncalibratedgate (mirroring--fail-on-dropped-queries) is a reasonable follow-up but is not in this PR.Documentation correctness
README.mdclaimed both H&M-2048 rows have 100 neighbours per query and 2,000 test queries. Measured on the shipped files:h-and-m-2048-angular-filtersh-and-m-2048-angular-no-filtersNeither is 100, and neither is 2,000 queries. That false line is exactly the belief that produced this bug, so it is corrected here — with a footnote explaining that the column bounds what recall can mean — rather than left to outlive the fix.
Performance
GroundTruthProfile::loadis skipped when nothing consumes it (--skip-search, filter-only runs). Otherwise an upload-only run would pay a full query-file parse per config — on the compound h-and-m dataset that parses every 2048-dim query vector only to drop it (~22 s warm / ~137 s cold per config; a 12-config sweep pays it twelve times). It is not inside any timed window, but it was a real--skip-searchregression.Tests
src/metrics.rs: the canonical 5-of-10 case (precision 1.00 / recall 0.50 / upstream 0.50), a short-ground-truth row (top=100, one true neighbour: 0.01 / 1.00 / 0.01 — a 100x split on identical data), the full-width case where all three agree, and sentinel padding shrinking only our denominator.src/bin/vector_db_benchmark/ground_truth.rs: width profiling, ceiling arithmetic, dedup/sentinel handling, unreachable-target detection (NaN excluded,+infstill flagged).src/bin/vector_db_benchmark/experiment.rs: the emitted document must not containmean_precisions,precisionsorprecision_dist; must contain the new names,metrics_schemaand the calibration block; calibration outcomes reported both ways.src/bin/vector_db_benchmark/summary.rs: summaries carrymetrics_schema_version: 2, no upstream key name, and list only genuinely unreached calibrations.plot.rs: legacymean_precisionssummaries still chart, and the new key wins when both are present.cargo test --lib --bins(694 + 98 + 2 pass),cargo clippy --all-targets -- -D warningsandcargo fmt --checkare clean.elasticsearch.rsandopensearch.rsare untouched.🤖 Generated with Claude Code