test: phase 3 -- property-based, determinism, adversarial tests + concurrency docs - #50
Conversation
SnapIndex.search silently accepted k=0 and k<0, returning all results (via numpy slice with non-positive k, which no-ops). PQSnapIndex, ResidualSnapIndex, and IVFPQSnapIndex already validated k>=1; this aligns SnapIndex with the rest of the public API. Caught by the new adversarial test suite.
29 new tests across three files; full suite now at 180 tests. - test_properties.py (hypothesis-driven): len() matches add_batch count, search returns <= k hits in descending order, delete reduces len by 1, save/load preserves search output, filter_ids subset is honoured, PQ / IVF-PQ roundtrip under varied dim/n/seed. max_examples capped at 25 per test so the suite stays under 1s. - test_determinism.py: two fits with the same seed produce byte-identical index files for all four index types, plus search-result equality on a fixed query. Catches any accidental non-determinism (unstable sorts, thread-ordered reductions). - test_adversarial.py: empty index, n=1, k > n, zero-norm query, all-same-vector corpus, unknown / empty filter sets, delete-all, bits=2 on clustered data, k=0 validation (above fix), nprobe boundaries on IVF-PQ, residual rerank saturation. Pulls hypothesis>=6.100 into the [dev] optional deps; not exposed at runtime.
Adds docs/user-guide/concurrency.md covering what is safe (concurrent search, multi-process reads, single writer), what is not (concurrent add_batch/delete/fit, mutation-during-search), and a drop-in wrapper pattern for applications that need multi-writer semantics today. References the roadmap for native write locking and the delta-buffer incremental-update mode. Linked from the user-guide nav.
There was a problem hiding this comment.
Code Review
This pull request introduces a new concurrency guide to the documentation, adds input validation to the search method, and significantly expands the test suite with adversarial, determinism, and property-based tests using Hypothesis. The review feedback highlights an inconsistency in the thread-safety documentation example where the search method should be protected by a lock to prevent data races during mutations. Additionally, there are suggestions to expand the coverage of determinism and property-based tests to include all index types, specifically IVFPQSnapIndex and PQSnapIndex.
| def search(self, query, k=10, **kwargs): | ||
| return self._idx.search(query, k=k, **kwargs) # no lock |
There was a problem hiding this comment.
The SafeIndex example is currently unsafe for concurrent use when mutations are active. While the documentation states that search() only touches shared state, it also explicitly mentions in lines 21-23 that one thread mutating while another searches is not safe because internal arrays are resized. In the provided SafeIndex implementation, search() does not acquire the lock, meaning it can run concurrently with add_batch() or delete(), leading to potential crashes or undefined behavior. To be truly safe, search() should also be protected by the lock, or the documentation should clarify that this pattern only protects writers from each other and requires external coordination with readers.
| def search(self, query, k=10, **kwargs): | |
| return self._idx.search(query, k=k, **kwargs) # no lock | |
| def search(self, query, k=10, **kwargs): | |
| with self._lock: | |
| return self._idx.search(query, k=k, **kwargs) |
| @pytest.mark.parametrize( | ||
| ("index_cls", "ext", "build_kwargs"), | ||
| [ | ||
| (SnapIndex, ".snpv", {"bits": 4}), | ||
| (PQSnapIndex, ".snpq", {"M": 8, "K": 16}), | ||
| (ResidualSnapIndex, ".snpr", {"b1": 3, "b2": 3}), | ||
| ], |
There was a problem hiding this comment.
IVFPQSnapIndex is missing from the test_search_results_are_deterministic parametrization. Since it is included in the bitwise determinism tests (line 79), it should also be verified here to ensure that its search results are consistent across different builds with the same seed.
@pytest.mark.parametrize(
("index_cls", "ext", "build_kwargs"),
[
(SnapIndex, ".snpv", {"bits": 4}),
(PQSnapIndex, ".snpq", {"M": 8, "K": 16}),
(ResidualSnapIndex, ".snpr", {"b1": 3, "b2": 3}),
(IVFPQSnapIndex, ".snpi", {"nlist": 8, "M": 8, "K": 16}),
],
)| n=st.integers(min_value=2, max_value=50), | ||
| seed=st.integers(min_value=0, max_value=2**16), | ||
| ) | ||
| def test_snap_save_load_preserves_search( |
There was a problem hiding this comment.
The property-based tests for delete, save/load, and filter_ids are currently only implemented for SnapIndex. To ensure consistent behavior and catch regressions across all index types, these tests should be generalized or parametrized to include PQSnapIndex, IVFPQSnapIndex, and ResidualSnapIndex where applicable.
There was a problem hiding this comment.
Pull request overview
Adds phase-3 test coverage focused on invariants, determinism, and adversarial edge-cases; documents the library’s concurrency model; and aligns SnapIndex.search() with other index types by validating k >= 1.
Changes:
- Add 3 new test suites: property-based (Hypothesis), determinism (bitwise save), and adversarial edge cases.
- Fix
SnapIndex.search()to raiseValueErrorwhen called withk < 1. - Document the concurrency contract and link it into the MkDocs nav; add
hypothesisto dev dependencies.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_properties.py |
New Hypothesis-driven invariant tests across SnapIndex, PQSnapIndex, and IVFPQSnapIndex. |
tests/test_determinism.py |
New tests asserting bitwise-deterministic index files and deterministic search results. |
tests/test_adversarial.py |
New edge-case regression tests (empty index, k edge cases, filters, degenerate vectors, IVF-PQ nprobe bounds). |
snapvec/_index.py |
Enforce k >= 1 in SnapIndex.search() (matches other index classes’ validation). |
pyproject.toml |
Add hypothesis>=6.100 to dev dependencies. |
mkdocs.yml |
Add Concurrency page to documentation navigation. |
docs/user-guide/concurrency.md |
New concurrency contract documentation + usage guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def add_batch(self, ids, vectors): | ||
| with self._lock: | ||
| self._idx.add_batch(ids, vectors) | ||
|
|
||
| def delete(self, id_): | ||
| with self._lock: | ||
| return self._idx.delete(id_) | ||
|
|
||
| def search(self, query, k=10, **kwargs): | ||
| return self._idx.search(query, k=k, **kwargs) # no lock | ||
| ``` |
There was a problem hiding this comment.
The SafeIndex example only locks add_batch/delete, but leaves search() unlocked. That does not prevent reader/writer overlap on the same instance, which the page says is unsafe. Either (a) take the same lock in search() as well, (b) demonstrate a reader/writer lock, or (c) recommend a copy-on-write pattern (build a new index, then atomically swap the reference) so searches never overlap with mutations.
| - Multiple threads calling `search()` on the **same** index concurrently. | ||
| Search paths allocate their own scratch buffers and only read shared | ||
| state, so no external lock is required. | ||
| - Multiple processes opening **different** index files and querying |
There was a problem hiding this comment.
The doc claims concurrent search() calls are safe because search only reads shared state, but SnapIndex.search() can still mutate internal state (e.g., lazily materializing _cache on first query) unless the index is freeze()d / pre-warmed. This makes the “no external lock required” statement inaccurate for first-query concurrency. Consider updating the contract to explicitly require idx.freeze() (or an explicit warm-up query) before multi-threaded search, aligning with snapvec/_freezable.py and SnapIndex.freeze()’s cache pre-warm behavior.
- concurrency.md: the SafeIndex example was self-contradictory. The surrounding prose says 'mutation during search is not safe', but the example left search() outside the lock. Acquire the lock inside search() too, and document the build-then-serve scenario where the reader lock can be skipped. - test_determinism.py: parametrize IVFPQSnapIndex into the search determinism test, matching the bitwise test. - test_properties.py: extend save/load and delete invariants to PQSnapIndex, and save/load to IVFPQSnapIndex. Suite grows to 185 tests.
SnapIndex.search() lazily materialises a float16 centroid cache on the first query. Two threads hitting their first search concurrently race on that assignment. freeze() pre-warms the cache on purpose (see the docstring in _index.py:freeze), so the concurrency contract should require it -- or at least one serial warm-up search -- before fanning out. PQSnapIndex / IVFPQSnapIndex / ResidualSnapIndex inherit the same contract via FreezableIndex.
Bumps the version to 0.10.0 and documents the landed work. No library behaviour changes beyond the surgical SnapIndex.search(k<1) validation shipped in PR #50; everything else is CI, docs, tests, and benchmarks. Adds: - CHANGELOG entry for 0.10.0 covering PRs #43, #49, #50, #51, #52, and #53 (CI matrix + wheels, MkDocs site, 40+ new tests, threading curve, forward-compat errors, competitive Pareto bench). - ROADMAP.md with scoped plans for v0.11 (streaming ingest, OPQ, strict mypy), v0.12 (file format v2, delta buffer), and v1.0 (API freeze + deprecation policy). Explicit non-goals so nobody opens a GPU-backend PR. - CITATION.cff so downstream papers can cite snapvec with a machine-readable metadata file. References TurboQuant (arXiv:2504.19874) and Jegou et al. product quantization as underlying algorithms. - README roadmap link so ROADMAP.md is discoverable from the project root.
Summary
Phase 3 (first slice): trust and correctness. Adds 29 tests across
three new files and documents the concurrency contract. Fixes one
real bug caught while writing adversarial tests.
Commits
-- `SnapIndex.search(k=0)` was silently returning all results
because slicing with a non-positive k no-ops. PQSnapIndex,
ResidualSnapIndex, and IVFPQSnapIndex already validated k>=1; this
aligns SnapIndex with the rest of the public API. Caught by the
new adversarial suite.
-- 29 new tests, suite goes from 151 to 180. Pulls
`hypothesis>=6.100` into dev deps.
-- new `docs/user-guide/concurrency.md` page, linked from nav.
What each suite covers
test_properties.py (8 hypothesis-driven tests)
`max_examples=25` per test, full file runs in <1s.
test_determinism.py (7 tests)
same seed + same inputs produce byte-identical index files
(`sha256` comparison).
each class that doesn't require coarse training.
test_adversarial.py (14 tests)
Empty index; n=1; k > n; zero-norm query; all-same-vector corpus;
unknown / empty filter set; delete-all; bits=2 behaviour;
`nprobe=nlist` and `nprobe` out-of-range on IVF-PQ;
`ResidualSnapIndex` rerank saturation.
Test plan
Out of scope
Those move to phase 3b in a follow-up PR.