fix(backends): convert embedding vectors to Python floats before upsert - #2187
Merged
Conversation
Chroma declares `requires_explicit_embeddings`, so every write on the
default backend routes through `EmbeddingCollection`. `_embed_texts`
built its rows with `list(v)`, and `v` is a float32 `np.ndarray` — that
unpacks into `np.float32` *scalars*, which chromadb's
`normalize_embeddings` rejects:
ValueError: Expected embeddings to be a list of floats or ints, a
list of lists, a numpy array, or a list of numpy arrays
`mine` aborted on the first drawer, as did every other write against a
default palace. Convert with `.tolist()` (C-speed), keeping a
`float(x)` branch for embedders that already return plain sequences.
The suite could not see this. conftest's autouse
`_stable_embedding_function_for_tests` monkeypatches
`embedding_wrapper._embed_texts` itself for every module outside
`_REAL_EMBEDDING_TEST_MODULES`, so the defective function was never
executed under test. The regression tests therefore go in
`test_embedding.py`, which is exempt from that stub: one asserts the
returned elements are builtin floats, one drives a real Chroma
collection through `EmbeddingCollection.upsert` and reads the document
back. Both fail against the previous line with the production
ValueError.
Verified end to end outside the suite: mining a project and searching
it back returns the drawer verbatim, on the host and in the container
image built from this tree.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a regression in the explicit-embedding wrapper used by backends (including default Chroma on develop) where numpy-backed embeddings were being converted into np.float32 scalars that ChromaDB rejects during normalization, causing writes (e.g., mempalace mine) to fail immediately.
Changes:
- Convert embedding vectors to plain Python floats using
v.tolist()(fast path) with afloat(x)fallback for already-sequence-like embedders. - Add regression + end-to-end tests to ensure wrapper output is accepted by a real Chroma collection and stays compatible with ChromaDB’s embedding shape/type expectations.
- Document the regression and test-suite blind spot in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mempalace/backends/embedding_wrapper.py |
Converts embedded vectors to plain Python floats before delegating to explicit-embedding backends. |
tests/test_embedding.py |
Adds targeted regression and end-to-end tests covering the real _embed_texts path and Chroma upsert/read-back. |
CHANGELOG.md |
Records the bug, impact, and why existing CI didn’t catch it. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mbeacom
added a commit
to mbeacom/mempalace
that referenced
this pull request
Aug 8, 2026
MemPalace#2187 fixed the NumPy 2.x ingest failure (MemPalace#2190) and covered the numpy path: an ndarray-returning EF plus a real EmbeddingCollection.upsert. Two branches of the fixed function are still unexercised. `_embed_texts` branches on `hasattr(v, "tolist")`. The `float(x)` arm is the one that serves embedders returning plain sequences — custom/BYO EFs, and rows arriving as tuples. Nothing ran it, so dropping the `float()` call there stays green and reaches users as the same production ValueError, just on a non-default embedder. The `if not texts: return []` guard is likewise untested. Callers pass empty batches (a drawer set fully filtered by dedup), and loading the EF is the expensive part — on the ONNX default it spins up a native session. Both tests are mutation-verified against this tree: - replacing the fallback with `list(v)` fails test_embed_texts_handles_plain_sequence_embedders - deleting the early return fails test_embed_texts_short_circuits_on_empty_input The plain-sequence EF yields `Decimal`, so the assertion proves a real conversion rather than values passing through unchanged, and the empty-batch test asserts by making `get_embedding_function` raise, so it verifies the EF is never constructed rather than only checking the return value. Tests only — no production code changes. They live in `test_embedding.py` for the reason MemPalace#2187 documented: conftest's autouse `_stable_embedding_function_for_tests` replaces `_embed_texts` outright for every module outside `_REAL_EMBEDDING_TEST_MODULES`. Refs MemPalace#2190 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 8, 2026
mbeacom
added a commit
to mbeacom/mempalace
that referenced
this pull request
Aug 8, 2026
MemPalace#2187 fixed the default-backend ingest failure (MemPalace#2190) and covered the numpy path: an ndarray-returning EF plus a real EmbeddingCollection.upsert. Two branches of the fixed function are still unexercised. (The defect was not NumPy-2.x-specific. `np.float32` has never been a `float` subclass in any NumPy major -- verified on 1.24.4, 1.26.4 and 2.4.4 -- so `list(arr)` output was always rejected. It was latent until 0e79797 added `requires_explicit_embeddings` and routed the default Chroma backend through `_embed_texts` for the first time.) `_embed_texts` branches on `hasattr(v, "tolist")`. The `float(x)` arm is the one that serves embedders returning plain sequences — custom/BYO EFs, and rows arriving as tuples. Nothing ran it, so dropping the `float()` call there stays green and reaches users as the same production ValueError, just on a non-default embedder. The `if not texts: return []` guard is likewise untested. Callers pass empty batches (a drawer set fully filtered by dedup), and loading the EF is the expensive part — on the ONNX default it spins up a native session. Both tests are mutation-verified against this tree: - replacing the fallback with `list(v)` fails test_embed_texts_handles_plain_sequence_embedders - deleting the early return fails test_embed_texts_short_circuits_on_empty_input The plain-sequence EF yields `Decimal`, so the assertion proves a real conversion rather than values passing through unchanged, and the empty-batch test asserts by making `get_embedding_function` raise, so it verifies the EF is never constructed rather than only checking the return value. Tests only — no production code changes. They live in `test_embedding.py` for the reason MemPalace#2187 documented: conftest's autouse `_stable_embedding_function_for_tests` replaces `_embed_texts` outright for every module outside `_REAL_EMBEDDING_TEST_MODULES`. Refs MemPalace#2190 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mbeacom
added a commit
to mbeacom/mempalace
that referenced
this pull request
Aug 8, 2026
MemPalace#2187 fixed the default-backend ingest failure (MemPalace#2190) and covered the numpy path: an ndarray-returning EF plus a real EmbeddingCollection.upsert. Two branches of the fixed function are still unexercised. (The defect was not NumPy-2.x-specific. `np.float32` has never been a `float` subclass in any NumPy major -- verified on 1.24.4, 1.26.4 and 2.4.4 -- so `list(arr)` output was always rejected. It was latent until 0e79797 added `requires_explicit_embeddings` and routed the default Chroma backend through `_embed_texts` for the first time.) `_embed_texts` branches on `hasattr(v, "tolist")`. The `float(x)` arm is the one that serves embedders returning plain sequences — custom/BYO EFs, and rows arriving as tuples. Nothing ran it, so dropping the `float()` call there stays green and reaches users as the same production ValueError, just on a non-default embedder. The `if not texts: return []` guard is likewise untested. Callers pass empty batches (a drawer set fully filtered by dedup), and loading the EF is the expensive part — on the ONNX default it spins up a native session. Both tests are mutation-verified against this tree: - replacing the fallback with `list(v)` fails test_embed_texts_handles_plain_sequence_embedders - deleting the early return fails test_embed_texts_short_circuits_on_empty_input The plain-sequence EF yields `Decimal`, so the assertion proves a real conversion rather than values passing through unchanged, and the empty-batch test asserts by making `get_embedding_function` raise, so it verifies the EF is never constructed rather than only checking the return value. Tests only — no production code changes. They live in `test_embedding.py` for the reason MemPalace#2187 documented: conftest's autouse `_stable_embedding_function_for_tests` replaces `_embed_texts` outright for every module outside `_REAL_EMBEDDING_TEST_MODULES`. Refs MemPalace#2190 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
pull Bot
pushed a commit
to FaZios/mempalace
that referenced
this pull request
Aug 8, 2026
The Docker workflow built both images and never started a container, and never parsed a Compose file. A green run therefore only meant the Dockerfile compiled. Two defects that break the very first documented command shipped past it: `docker-compose.yml` carried a bare `environment:` key that made Compose reject the file outright (MemPalace#2188), and `_embed_texts` handed chromadb `np.float32` scalars so `mine` aborted on the first drawer (MemPalace#2187). Add `scripts/docker-smoke.sh`, which exercises what the README tells users to run: 1. `compose config` on docker-compose.yml and the server compose file 2. entrypoint dispatch for both `cli ...` and bare passthrough 3. `mine` a mounted directory, asserting a drawer is filed 4. `search` from a *separate* container, asserting the stored text comes back verbatim — this is the assertion that matters, since storing user words exactly is the promise the palace makes 5. a real MCP stdio JSON-RPC handshake: initialize, tools/list, and a mempalace_search call whose result must contain the drawer It asserts on returned content, not just exit codes, and lives in a script rather than inline YAML so it runs identically on a laptop: `scripts/docker-smoke.sh <image>`. The new `smoke` job builds amd64 natively with `load: true` (buildx cannot load a multi-arch manifest) and reads the publish job's cache while writing its own scope, so an amd64-only export never lands on top of the multi-arch one. `build` now needs it, so a failing smoke test blocks publication rather than being noticed afterwards. Verified by reintroducing each defect against a real build: the compose regression fails at step 1, the embedding regression at step 3, and the current tree passes all five. Failure output is clipped to 500 columns because a rejected embedding batch otherwise prints a whole 384-dim vector on one line and buries the message.
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.
The bug
mempalace mineaborts on the first drawer against a default (Chroma) palace:_embed_textsbuilt its rows withlist(v).vis a float32np.ndarray, solist(v)unpacks it intonp.float32scalars — a shape chromadb'snormalize_embeddingsrefuses. Every write through the wrapper dies, not just mining.This became reachable when Chroma started declaring
requires_explicit_embeddings(0e79797, "stabilize Chroma embeddings on Windows"). Before that the wrapper only served the opt-in backends; after it, the default backend routes through it too.Scope:
developonly —main/ PyPI 3.6.0 do not carry the capability flag. Butdevelopis this repo's default branch, so anyone cloning and building the Docker image per the README hits it. Related to the Docker complaint in #2177.The fix
.tolist()(C-speed) with afloat(x)fallback for embedders that already hand back plain sequences.Why CI was green
tests/conftest.pyautouse fixture:It replaces the exact function that crashes, for every module outside
_REAL_EMBEDDING_TEST_MODULES. 3852 passing tests, zero execution of the real path.So the regression tests live in
test_embedding.py, which is exempt from that stub:test_embed_texts_returns_plain_python_floats— feeds a numpy-returning EF, asserts every element is a builtinfloat.test_embedding_collection_upsert_accepts_numpy_backed_vectors— drives a real Chroma collection throughEmbeddingCollection.upsertand reads the document back, so a future chromadb tightening is caught too.Verification
ValueError, and pass with the fix. The tests were checked against the bug, not just against the patch.ruff check+ruff format --checkclean.