Skip to content

fix: ChromaUris was shadowed by a 'metadata' class, so every uris write raised - #5

Merged
thorwhalen merged 1 commit into
masterfrom
fix/chroma-uris-shadowed
Sep 22, 2026
Merged

thorwhalen merged 1 commit into
masterfrom
fix/chroma-uris-shadowed

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Post-merge refute review of #3.

Defect

chromadol/base.py defines class ChromaUris twice. #3 reordered both decorator stacks but left them as they were otherwise. The second definition wins and is a single_nested_value("metadata") codec, and metadata is not a chromadb kwarg. So on master:

>>> ChromaUris(col)["k"] = "file:///a"
TypeError: Collection.upsert() got an unexpected keyword argument 'metadata'. Did you mean 'metadatas'?
>>> ChromaUris(col).append("file:///b")   # same TypeError

#3's test_append_writes_what_setitem_writes[ChromaUris] passes only because _RecordingCollection.upsert accepts any kwarg, so it compares two equally wrong calls.

Fix

  • ChromaUris is now the one uris store again. Its docstring says a collection with a data_loader is needed, because chromadb refuses uris otherwise.
  • The shadowing class is now ChromaMetadatas on the real metadatas field. It has no append: chromadb refuses an upsert with neither documents nor images ("Exactly one of documents, images must be provided"), so an auto-keyed metadata-only append can never succeed. It works as a read view.
  • fleet_dependents.json lists no importers of chromadol.

Tests

  • test_single_field_stores_write_their_own_chromadb_field (documents/uris/metadatas) asserts the exact kwarg each store sends. On master ChromaUris sends metadata.
  • test_chroma_metadatas_reads_the_metadatas_field runs against a real PersistentClient.
  • Local: 11 passed.

Self-reviewed only (the worker was told not to spawn a sub-agent reviewer). This needs a post-merge review.

🤖 Generated with Claude Code

base.py defined `ChromaUris` twice; the second (codec field "metadata",
which is not a chromadb kwarg) replaced the first, so every ChromaUris
write/append raised TypeError. #3's test covered "ChromaUris" only through
a fake that accepts any kwarg, so it passed. The second class is now
`ChromaMetadatas` on the real "metadatas" field, with no `append` since
chromadb refuses an upsert that has metadata but no documents.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit fab2bc5 into master Sep 22, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the fix/chroma-uris-shadowed branch September 22, 2026 14:11
thorwhalen added a commit that referenced this pull request Sep 22, 2026
)

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>
@thorwhalen

Copy link
Copy Markdown
Member Author

Post-merge review against a real chromadb 1.5.9: ChromaUris still could not write or read. upsert embeds only documents or images, so a uris-only upsert raises even with a data_loader. And a default get leaves uris out, so reads were always None. Fixed in #6: add for new keys, delete + add (restored on failure) for existing ones, and a get_include seam. Design concerns, not changed: ChromaCollection.__getitem__ never raises KeyError, so in is always True and .get(k, default) never returns the default. ChromaMetadatas could write existing keys through collection.update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant