fix: ChromaUris could neither write nor read uris on a real chromadb (review of #5) - #6
Merged
Merged
Conversation
Post-merge review of #5, against chromadb 1.5.9. - Writes: upsert embeds only documents or images, so an upsert carrying only uris raises "Exactly one of documents, images must be provided", even with a data_loader. Only add embeds uris through the data loader. ChromaUris now adds new keys, and replaces existing ones (delete + add, keeping metadata as upsert would). If the add fails, it restores the old record. - Reads: a default collection.get leaves uris out, so ChromaUris[k] was always None. ChromaCollection gains a get_include seam; ChromaUris sets it to ("uris",). - The recording fake also records add and filters get by id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
round-3 review: no defect found. Checked on real chromadb 1.5.9: a failed overwrite of a doc-only record and of a doc+uri+metadata record restores embedding, document, uri and metadata exactly; a collection with no data loader restores after the add fails; a multi-key write with mixed existing and new keys keeps metadata; a missing-key read returns [] (the pre-existing behaviour, already noted). |
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.
Post-merge adversarial review of #5, run against a real
chromadb1.5.9 in a scratch venv.chromadbis unpinned, so this is what users install.Defects found (the fix in #5 did not make ChromaUris work)
With a real
PersistentClientcollection created withdata_loader=FileLoader():chromadb,upsertandupdateembed onlydocumentsorimages. Onlyaddembeds fromuristhrough the data loader (_validate_and_prepare_upsert_requestpassesembeddable_fields={"documents", "images"}, whileadduses the default set, which includesuris). So fix: ChromaUris was shadowed by a 'metadata' class, so every uris write raised #5's docstring ("needs a collection created with adata_loader") was not enough: the write fails either way. fix: ChromaUris was shadowed by a 'metadata' class, so every uris write raised #5's tests only exercised a recording fake that accepts any kwarg.collection.get(k)includes only documents and metadatas.uriscomes backNone, so the codec always returnedNone.Fix
ChromaCollection.get_includeis a new class attribute. It defaults toNone, which keeps chromadb's default, soChromaCollectionandChromaDocumentsreads are unchanged.ChromaUrissets it to("uris",).ChromaUris.__setitem__callsaddfor new keys. For existing keys it does delete + add, keeping the record's metadata the wayupsertdoes. If the add fails (for example when the loader can't read the uri), it restores the old record with its embeddings. It also turns chromadb's read-back{}metadata intoNone, since chromadb rejects{}on write.addtoo and filtersgetby id, so the existing kwarg assertions still check the same thing.Tests
These two fail on master and pass here:
test_chroma_uris_round_trips_against_real_chromadb: set, get, overwrite with and without metadata, and append, all on a real collection.test_chroma_uris_overwrite_that_fails_keeps_the_old_recordLocally: 13 passed (py3.12, chromadb 1.5.9). Ruff is clean.
Design concerns (not changed here)
ChromaCollection.__getitem__never raisesKeyError. A missing key returns an emptyGetResult(soChromaUrisgives[]andChromaDocumentsgives[]), and__contains__is therefore alwaysTrue.store.get(k, default)never returns the default. This predates fix: ChromaUris was shadowed by a 'metadata' class, so every uris write raised #5.ChromaMetadatascould write metadata for existing keys throughcollection.update(ids, metadatas=...), which needs no embeddable field. Only new keys are impossible. fix: ChromaUris was shadowed by a 'metadata' class, so every uris write raised #5 left it as read-mostly, and that is a defensible choice.Self-reviewed only (this reviewer was told not to spawn a sub-agent).
🤖 Generated with Claude Code