From e64aa92f0d6144bc149eb07055b853206ba5f49c Mon Sep 17 00:00:00 2001 From: Joshua Fontany Date: Wed, 5 Aug 2026 10:27:05 -0700 Subject: [PATCH] test(chroma): assert the HNSW defaults rather than a threshold arithmetic test_hnsw_write_defaults_bound_index_rewrites divided a hypothetical corpus size by the configured sync_threshold and read the quotient as a count of full on-disk index rewrites. It measured no writes, no compactions, no persistence calls and no rewritten bytes, so it asserted arithmetic on a constant rather than a property of the implementation. The metadata tests already cover the defaults, and a write-amplification regression wants I/O measurement under the benchmark suite. Two neighbouring docstrings carried the same reasoning in prose and now state what holds: the values are chromadb's own documented defaults, so a collection this backend creates indexes on the same terms as one chromadb creates itself, and sync_threshold=2 reads as a value chromadb's own parameter validation rejects. The threshold relationship now reads batch_size <= sync_threshold, which is what chromadb permits. _hnsw_creation_metadata(options) reads hnsw_space directly, so get_collection's local copy of that resolution had no reader; removing it leaves the option's path to a created collection unchanged. tests/test_backends.py: 91 passed. Backend/palace selection: 593 passed, 16 skipped. --- mempalace/backends/chroma.py | 3 --- tests/test_backends.py | 44 ++++++------------------------------ 2 files changed, 7 insertions(+), 40 deletions(-) diff --git a/mempalace/backends/chroma.py b/mempalace/backends/chroma.py index 9e2db52..2c7bce4 100644 --- a/mempalace/backends/chroma.py +++ b/mempalace/backends/chroma.py @@ -2410,9 +2410,6 @@ class ChromaBackend(BaseBackend): pass client = self._client(palace_path) - hnsw_space = "cosine" - if options and isinstance(options, dict): - hnsw_space = options.get("hnsw_space", hnsw_space) ef = self._resolve_embedding_function() ef_kwargs = {"embedding_function": ef} if ef is not None else {} diff --git a/tests/test_backends.py b/tests/test_backends.py index 56a8633..6de1524 100644 --- a/tests/test_backends.py +++ b/tests/test_backends.py @@ -526,9 +526,9 @@ def test_chroma_backend_creates_collection_with_cosine_distance(tmp_path): def test_chroma_backend_sets_hnsw_write_defaults_on_creation(tmp_path): """HNSW batch/sync thresholds must land on freshly-created collection metadata. - That both keys land covers #1161 (config silently dropped). The VALUES guard - #1308: sync_threshold sets a mine's write amplification, and batch_size must - stay strictly below it (#2526). + That both keys land covers #1161, where the configuration was silently dropped. + The values are chromadb's own documented defaults, so a collection this backend + creates indexes on the same terms as one chromadb creates itself. """ palace_path = tmp_path / "palace" @@ -544,7 +544,7 @@ def test_chroma_backend_sets_hnsw_write_defaults_on_creation(tmp_path): sync = col.metadata.get("hnsw:sync_threshold") assert batch == 100 assert sync == 1000 - assert batch < sync, "chroma requires batch_size < sync_threshold (#2526)" + assert batch <= sync, "chromadb permits batch_size <= sync_threshold" def test_chroma_backend_create_collection_sets_hnsw_write_defaults(tmp_path): @@ -568,8 +568,9 @@ def test_sub_threshold_mine_survives_reopen_and_escapes_quarantine(tmp_path): leaves the segment alone. Asserting the mechanism instead (index_metadata.pickle present after 3 - records) only holds at sync_threshold=2, which buys #1308. Under chromadb's - own thresholds the Rust writer keeps the tail durable WITHOUT a pickle, so + records) only holds at sync_threshold=2, a value chromadb's own parameter + validation rejects. Under chromadb's documented thresholds the Rust writer + keeps the tail durable WITHOUT a pickle, so the pickle is rightly absent here and asserting on it would fail a healthy palace. """ @@ -607,37 +608,6 @@ def test_sub_threshold_mine_survives_reopen_and_escapes_quarantine(tmp_path): reopened.close() -def test_hnsw_write_defaults_bound_index_rewrites(tmp_path): - """Regression for #1308: the thresholds must bound write amplification. - - chromadb rewrites the ENTIRE on-disk index once sync_threshold records - accumulate, so a mine of N drawers costs N/sync_threshold full rewrites of a - multi-megabyte segment. At 2, a 26k-drawer mine fires ~13,000 — enough to - wedge the compactor and to tear a persist mid-pickle. - - Arithmetic, not a live mine: a realistic corpus must cost rewrites in the - tens, never the thousands. - """ - palace_path = tmp_path / "palace" - ChromaBackend().get_collection( - str(palace_path), collection_name="mempalace_drawers", create=True - ) - client = chromadb.PersistentClient(path=str(palace_path)) - col = client.get_collection("mempalace_drawers") - - sync = col.metadata.get("hnsw:sync_threshold") - batch = col.metadata.get("hnsw:batch_size") - - assert batch < sync, "chroma requires batch_size < sync_threshold (#2526)" - - realistic_corpus = 26_000 - rewrites = realistic_corpus / sync - assert rewrites <= 100, ( - f"sync_threshold={sync} costs ~{rewrites:.0f} full index rewrites over a " - f"{realistic_corpus}-drawer mine — that is the #1308 corruption path" - ) - - def test_single_record_upsert_not_quarantined(tmp_path): """A single-record upsert must not trigger quarantine.