Skip to content

fix(embedding): add embed_query/embed_documents to EmbeddinggemmaONNX for ChromaDB 1.5.x - #1631

Merged
igorls merged 2 commits into
developfrom
unknown repository
Jun 6, 2026
Merged

fix(embedding): add embed_query/embed_documents to EmbeddinggemmaONNX for ChromaDB 1.5.x#1631
igorls merged 2 commits into
developfrom
unknown repository

Conversation

@ghost

@ghost ghost commented May 27, 2026

Copy link
Copy Markdown

Bug

ChromaDB 1.5.x calls embedding_function.embed_query(input=...) via keyword argument during collection.query(). EmbeddinggemmaONNX lacked both embed_query and embed_documents methods, causing:

TypeError: embed_query() got an unexpected keyword argument input

whenever semantic search was triggered.

Fix

This PR adds the two methods required by the ChromaDB EF protocol, using input (the ChromaDB kwarg name, noqa A002) so that palace search works correctly with the embeddinggemma model.

Also downloads the companion _data ONNX file alongside the main model to prevent runtime InferenceSession failures.

Related

Fixes silent search failures when embedding_model is set to "embeddinggemma".

… for ChromaDB 1.5.x compatibility

ChromaDB 1.5.x calls embedding_function.embed_query(input=...) via
keyword argument during collection.query(). EmbeddinggemmaONNX lacked
both embed_query and embed_documents methods, causing:

  TypeError: embed_query() got an unexpected keyword argument 'input'

whenever semantic search was triggered.

This patch adds the two methods required by the ChromaDB EF protocol,
using  (the ChromaDB kwarg name, noqa A002) so that palace
search works correctly with the embeddinggemma model.

Also downloads the companion  ONNX file alongside the main model
to prevent runtime InferenceSession failures.

Fixes silent search failures when  is set to
embeddinggemma.
@ghost
ghost requested a review from milla-jovovich as a code owner May 27, 2026 03:32

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates mempalace/embedding.py to download the ONNX model data file during lazy loading and adds embed_query and embed_documents methods to support the ChromaDB embedding function protocol. A critical issue was identified in embed_query: if it is called with a single string (as is common in integrations like LangChain), it will iterate over individual characters instead of treating the string as a single input. A suggestion was provided to handle both string and list inputs correctly.

Comment thread mempalace/embedding.py
Comment on lines +229 to +231
def embed_query(self, input: list[str]) -> list[list[float]]: # noqa: A002 — ChromaDB EF protocol
"""Embed query documents (ChromaDB EF protocol)."""
return self(input)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If embed_query is called with a single string (which is the standard signature for embed_query in LangChain and other common integrations), input will be a str instead of a list[str].

Since EmbeddinggemmaONNX.__call__ expects an iterable of strings, passing a single string will cause Python to iterate over its individual characters.

Suggested change
def embed_query(self, input: list[str]) -> list[list[float]]: # noqa: A002 — ChromaDB EF protocol
"""Embed query documents (ChromaDB EF protocol)."""
return self(input)
def embed_query(self, input: str | list[str]) -> list[float] | list[list[float]]: # noqa: A002 — ChromaDB EF protocol
"""Embed query documents (ChromaDB EF protocol)."""
if isinstance(input, str):
return self([input])[0]
return self(input)

…eights + tokenizer)

The EmbeddinggemmaONNX lazy-load now fetches the ONNX external-weights file
(model.onnx_data) in addition to the model graph and tokenizer, so a single
warm-up issues 3 downloads, not 2. The lazy-load-once invariant is unchanged
(InferenceSession and Tokenizer.from_file are still each built exactly once).
@igorls
igorls self-requested a review as a code owner May 30, 2026 05:57
@igorls

igorls commented May 30, 2026

Copy link
Copy Markdown
Member

Maintainer push: added one commit (48f4eb6) updating test_lazy_load_runs_once to expect 3 hf_hub_download calls instead of 2. Your fix correctly adds the model.onnx_data external-weights download (the model can't load weights without it), so a single warm-up now legitimately issues 3 downloads. The lazy-load-once invariant still holds — InferenceSession and Tokenizer.from_file are each still built exactly once. Full test_embeddinggemma.py is green locally (10 passed). The embed_query/embed_documents EF-protocol additions look correct for ChromaDB 1.5.x. Thanks!

@mvalentsev

Copy link
Copy Markdown
Contributor

#1667 independently adds the same model_quantized.onnx_data sibling fetch (sidecar only). Flagging the overlap for dedup. This PR also bumps test_lazy_load_runs_once for the extra hf_hub_download and adds the embed_query/embed_documents methods, which #1667 doesn't.

@igorls
igorls merged commit 5289bdf into MemPalace:develop Jun 6, 2026
6 checks passed
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.

2 participants