test(integration): raise per-query HNSW ef so small-graph recall is reliable#1512
Closed
dawsontoth wants to merge 4 commits into
Closed
test(integration): raise per-query HNSW ef so small-graph recall is reliable#1512dawsontoth wants to merge 4 commits into
dawsontoth wants to merge 4 commits into
Conversation
The vector-index-integrity suite is a major Windows CI hotspot (~29 min for the suite). Most of the cost is ~100–200 sequential, individually-committed HTTP round-trips per test — tiny data (DIMS=8, N≤50), so it's round-trip + per-write fsync overhead, not vector math. Apply correctness-neutral reductions: - reindex backfill: bulk-insert the N pre-index seed records in one request instead of N sequential POSTs. There is no HNSW index during insert, so per-record ordering is irrelevant; the backfill builds the index afterward. - update-churn & reindex: run the independent read-back verification searches concurrently (Promise.all) instead of serializing N round-trips. Left untouched (semantically order-sensitive): the entry-point insert/delete sequence and the churn update sequence. Note: validated on CI, not locally — restart_service is flaky in a local multi-loopback setup (readiness probes time out at 60s), which both fails and dominates local timings; the churn test (the one that runs cleanly locally) still passes with the parallelized read-back. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Each test in this suite deployed its own component and restarted the HTTP workers; the restart is the dominant per-test cost on slow runners (a test doing 3 inserts still took ~265s on Windows — almost entirely restart/setup). Deploy all four tables in one component with a single restart in `before`, and drop the per-test add_component/set_component_file/restart. Restarts: ~5 -> 2 (the shared one, plus the single restart `reindex` needs for its mid-test index-add); add_component: 4 -> 1. Tables keep distinct names/databases so the tests stay isolated despite sharing one component. ReindexTable is deployed without an index initially; the reindex test redeploys the full schema with it indexed (a no-op re-assert for the other three, which have already run — reindex is last) to trigger backfill. CI-validated (local restart_service is flaky in a multi-loopback setup). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eliable The reindex/delete-entry/churn recall assertions intermittently failed: an exact-match self-vector search occasionally missed records that ARE correctly indexed. Root cause — Harper's search ef auto-scales with √N (tuned for 5K–30K-node graphs); on these 40–50-node test graphs it under-explores, so HNSW approximate recall drops below the tolerance. A per-query ef overrides the auto-scale (search() resolves per-query ef first), so the vector-search helpers now pass ef=64 (above the working set) for reliable recall on the small graphs. Tolerances are left unchanged: a genuinely unindexed/disconnected record (a real backfill bug) is still missed regardless of ef, so the data-integrity guards stand — this only removes the small-graph approximate-recall noise. Stacked on #1505. CI-validated (local restart_service is flaky). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a default SEARCH_EF constant of 64 in the vector index integrity integration tests to prevent flaky recall assertions on small test graphs. It updates vectorSearch and vectorThresholdSearch to apply this default ef value. The reviewer suggests allowing opts.ef to be explicitly set to null so that tests can still omit the ef parameter and verify the system's default auto-scaling behavior.
Contributor
|
Reviewed; no blockers found. |
Address review feedback (#1512): vectorSearch now accepts `ef: null` to omit the ef param entirely, so a future test can exercise Harper's default auto-scaling behavior. `ef` undefined still defaults to SEARCH_EF (the small-graph recall fix); an explicit number still overrides. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Status: Draft — focused fix for the HNSW recall flake in shard 5/6. Stacked on #1505 (same file).
Flake
The
reindex backfill(and occasionallydelete-entry-point/update-churn) recall assertions intermittently failed — e.g. on PR #1504's run,reindex backfillfailed on v24+v26 with too many misses, even though the test is unmodified there. An exact-match self-vector search would miss records that are correctly indexed.Root cause
Harper's HNSW search
efauto-scales with √N, deliberately tuned for 5K–30K-node graphs (resources/indexes/HierarchicalNavigableSmallWorld.ts). On these tiny 40–50-node test graphs the auto-scaled ef under-explores, so approximate recall drops below the tolerance — nondeterministically, depending on the graph the backfill happened to build.Fix
A per-query
efwins over the auto-scale (search()resolves per-query ef first). The vector-search helpers now passef = 64(above the working set) so the search explores essentially the whole small graph → reliable recall.Tolerances are left unchanged. A genuinely unindexed/disconnected record (a real backfill bug — what these tests guard) is still missed regardless of ef, so the data-integrity assertions stand. This only removes the small-graph approximate-recall noise, not the guard.
Validation
CI-only (local
restart_serviceis flaky). If CI shows theeffield doesn't thread through the sort/condition query path, the fallback is an explicitefConstructionSearchon the index schema — but note that path currently stores the value as a string (the@indexedarg parser uses a string cast, not the numeric coercer), which is a separate latent bug worth its own fix.🤖 Generated with Claude Code