fix(backends): return Python floats from _embed_texts for NumPy 2.x - #2
Closed
mbeacom wants to merge 1 commit into
Closed
fix(backends): return Python floats from _embed_texts for NumPy 2.x#2mbeacom wants to merge 1 commit into
mbeacom wants to merge 1 commit into
Conversation
Every write to the default Chroma backend failed with:
ValueError: Expected embeddings to be a list of floats or ints, a list
of lists, a numpy array, or a list of numpy arrays, got
[[np.float32(-0.08368583), ...]]
Two independently-correct changes combined into a regression:
1. ChromaBackend.capabilities began declaring "requires_explicit_embeddings",
routing the DEFAULT backend through EmbeddingCollection for the first time.
2. embedding_wrapper._embed_texts converted rows with list(...). Iterating a
NumPy row yields np.float32 *scalars*, so it returned list[list[np.float32]]
rather than the annotated list[list[float]].
ChromaDB's normalize_embeddings gates on isinstance(target[0][0], (int, float)).
Under NumPy 2.x np.float32 is no longer a float subclass (np.float64 still is),
so the batch fell through to the ValueError.
This was latent before: only pgvector/sqlite_exact/milvus/qdrant used
_embed_texts, and they accept np.float32. pyproject pins numpy>=1.24 with no
upper bound, so fresh installs resolve NumPy 2.x and hit this by default.
Rows now convert with .tolist(), which returns genuine Python floats. The
hasattr guard keeps embedding functions that already return plain lists working.
The regression tests live in tests/test_embedding.py deliberately: conftest's
autouse fixture monkeypatches _embed_texts away for every module outside
_REAL_EMBEDDING_TEST_MODULES, so tests placed anywhere else would exercise the
stub and pass against the buggy code. That stub is why CI stayed green while
ingest was broken in the field.
Fixes MemPalace#2190
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Superseded by MemPalace#2191 — this fix belongs upstream against |
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.
Fixes MemPalace#2190
What does this PR do?
Fixes a total ingest failure on the default Chroma backend. Every write raised:
Root cause — two independently-correct changes that combined into a regression
mempalace/backends/chroma.py—ChromaBackend.capabilitiesbegan declaringrequires_explicit_embeddings(commit0e79797, "fix: stabilize Chroma embeddings on Windows"). This routed the default backend throughEmbeddingCollectionfor the first time.mempalace/backends/embedding_wrapper.py::_embed_texts— converted rows withlist(...):Iterating a NumPy row yields
np.float32scalars, so this returnedlist[list[np.float32]], not the annotatedlist[list[float]].ChromaDB's
normalize_embeddings(chromadb/api/types.py) gates onisinstance(target[0][0], (int, float)). Under NumPy 2.x,np.float32is not afloatsubclass — so the batch falls straight through toraise ValueError. Confirmed locally on numpy 2.4.4 / chromadb 1.5.7:Neither change is wrong on its own. #2 was latent: only pgvector / sqlite_exact / milvus / qdrant used
_embed_texts, and they all acceptnp.float32. Becausepyproject.tomlpinsnumpy>=1.24with no upper bound, fresh installs resolve NumPy 2.x and hit this by default.The fix
Convert rows with
.tolist()instead oflist(...)— it returns genuine Python floats, which every backend accepts. Thehasattrguard keeps embedding functions that already return plain Python lists working unchanged.Why CI didn't catch this
tests/conftest.pyhas an autouse fixture that monkeypatches_embed_textsaway for every test module except the_REAL_EMBEDDING_TEST_MODULESallowlist ({"test_embedding", "test_embeddinggemma"}). Every existing test exercised the stub, not the real function — so the suite stayed green while ingest was broken in the field.The regression tests are therefore placed in
tests/test_embedding.pydeliberately. I verified the failure mode: tests placed intests/test_backends.pysilently hit the conftest stub and pass even against the buggy code.How to test
1. Reproduce on
develop(pre-fix) — fails:2. Same snippet with this branch — passes:
3. The new tests genuinely catch the regression. I reverted the one-line fix back to
return [list(v) for v in vectors]and re-ran:The fix was then restored — the revert is not in this branch.
4. Real end-to-end mine (not a dry-run — this actually exercises the Chroma write path) over 3 real Claude transcripts into a throwaway palace:
develop(unpatched)ValueErrorin_validate_and_prepare_upsert_requestmempalace searchagainst the resulting palace returns correct verbatim content with sensiblecosine_sim/bm25scores, so the read path is healthy too.5. Full suite / lint
Test-count accounting:
developcollects 3881, this branch collects 3883 (exactly +2), and 3852 passed + 31 skipped = 3883. ✅Checklist
python -m pytest tests/ -v) — 3852 passed, 31 skippedruff check .) — plusruff format --check .cleanNotes
_embed_textsnow records why.tolist()is required, so thelist(...)form isn't reintroduced.EmbeddingCollection→ real-backend seam is effectively untested outside two allowlisted modules. That blind spot is what let this reach users.