test(backends): cover the _embed_texts fallback and empty-batch guard - #2191
Merged
igorls merged 1 commit intoAug 11, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 tasks
mbeacom
force-pushed
the
mbeacom-fix-numpy2-embedding-floats
branch
from
August 8, 2026 13:41
141c902 to
4826529
Compare
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
force-pushed
the
mbeacom-fix-numpy2-embedding-floats
branch
from
August 8, 2026 14:00
1a06310 to
656e0a4
Compare
igorls
approved these changes
Aug 11, 2026
igorls
left a comment
Member
There was a problem hiding this comment.
Wave 3 minimal for 3.7.0: LGTM. Docs/tests/tiny fix only.
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.
Refs #2190. Follow-up to #2187 — tests only, no production code changes.
What does this PR do?
#2187 fixed the default-backend ingest failure and covered the numpy path well — an ndarray-returning EF plus a real
EmbeddingCollection.upsertround-trip. Two branches of the fixed function are still unexercised:1. The
float(x)fallback arm._embed_textsbranches onhasattr(v, "tolist"):Only the numpy side runs under test. The fallback is the arm serving embedders that return plain sequences — custom/BYO EFs, and rows that arrive as tuples. I confirmed the gap by mutation: replacing that arm with
list(v)leaves the suite fully green. A regression there would surface only in the field, on a non-default embedder, as the same productionValueError#2190 describes.2. The
if not texts: return []guard. Also untested. Callers pass empty batches (a drawer set fully filtered by dedup), and constructing the EF is the expensive part — on the ONNX default it spins up a native session. Deleting the early return is likewise green today.Both tests are written so they assert the behaviour, not just the shape:
Decimal, so== [[0.5, 0.25], ...]plustype(x) is floatproves a real conversion happened rather than values passing through unchanged;get_embedding_functionraise, so it verifies the EF is never constructed, not merely that[]came back.Neither test depends on the NumPy version. They live in
tests/test_embedding.pyfor exactly the reason #2187 documented: conftest's autouse_stable_embedding_function_for_testsreplaces_embed_textsoutright for every module outside_REAL_EMBEDDING_TEST_MODULES, so tests placed elsewhere exercise the stub and pass against broken code.How to test
Mutation-verified — each new test was proven to fail against a deliberately broken tree, then the tree was restored:
list(v)test_embed_texts_handles_plain_sequence_embeddersfails (type(x) is float→ False)if not texts: return []test_embed_texts_short_circuits_on_empty_inputfails (get_embedding_function must not be called for an empty batch)developThe load-bearing point: deleting either branch of the shipped function leaves the suite entirely green today. That is the argument for these tests existing.
git diff develop -- mempalace/is empty — the change is confined totests/test_embedding.py(+49).Correcting the root-cause framing (this was mine, and it was wrong)
An earlier revision of this PR described #2190 as a "NumPy 2.x" failure, and I repeated that on the issue. That attribution is incorrect and I want it on the record rather than quietly edited away.
np.float32has never been afloatsubclass in any NumPy major. Verified directly across the full supported range (pyproject.tomlpinsnumpy>=1.24):isinstance(np.float32(0.5), float)isinstance(np.float64(0.5), float)FalseTrueFalseTrueFalseTruefloat32mro on 1.26.4:['float32', 'floating', 'inexact', 'number', 'generic', 'object']— nofloat.So
normalize_embeddingswould have rejectedlist(arr)output on NumPy 1.x just the same; there was no version boundary. The accurate framing is the one #2187's own docstring used, which pointedly never claimed version-specificity:A corollary worth stating explicitly: a
numpy<2upper bound would not have prevented this, and shouldn't be added on this basis.None of this affects the change here — both tests target version-independent branches and remain mutation-proven.
Independent confirmation that #2190 is resolved on
develop_embed_texts(['hello world'])→<class 'numpy.float32'>, andnormalize_embeddingsraises.develop: →<class 'float'>,normalize_embeddingsOK, dim 384.--dry-run) of 3 Claude transcripts into a throwaway palace: pre-fix 0 drawers filed, aborting in_validate_and_prepare_upsert_request; ondevelop86 drawers filed, andmempalace searchreturns the drawer verbatim.Checklist
python -m pytest tests/ -v) — 3854 passed, 31 skippedruff check .) — plusruff format --check .clean