fix(embedding): download embeddinggemma external-data sibling (.onnx_data) - #1667
Open
JeanBaptisteRenard wants to merge 1 commit into
Open
Conversation
The q8 ONNX export (onnx-community/embeddinggemma-300m-ONNX) stores its weights in a sibling model_quantized.onnx_data file referenced by the .onnx via a relative path. _lazy_load() only fetched model_quantized.onnx, so on a clean cache the first embeddinggemma use crashes: onnxruntime ... FAIL: External data path validation failed ... External data path does not exist: .../model_quantized.onnx_data Fetch the .onnx_data sibling into the same snapshot dir so onnxruntime can resolve the relative path. Wrapped in try/except since inlined-weight exports have no sibling (non-fatal).
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates mempalace/embedding.py to download the external-data sibling file for the quantized ONNX model if it exists, preventing runtime failures when weights are stored externally. The review feedback recommends catching a specific EntryNotFoundError from huggingface_hub.utils instead of a broad Exception to avoid swallowing unrelated errors, and suggests pinning the repository revision to ensure consistency between downloads.
Comment on lines
+200
to
+208
| try: | ||
| hf_hub_download( | ||
| _EMBEDDINGGEMMA_REPO, | ||
| subfolder="onnx", | ||
| filename=_EMBEDDINGGEMMA_ONNX + "_data", | ||
| ) | ||
| except Exception: | ||
| # Some exports inline their weights — a missing sibling is non-fatal. | ||
| logger.debug("No external-data sibling for %s", _EMBEDDINGGEMMA_ONNX) |
Contributor
There was a problem hiding this comment.
Specific Exception Handling & Revision Pinning
- Avoid Broad Exception Catching: Catching
Exceptioncan swallow unrelated errors (such as network timeouts, DNS failures, permission issues, or disk full errors) and lead to confusing ONNX Runtime errors later. Instead, import and catchEntryNotFoundErrorfromhuggingface_hub.utilsto specifically handle the case where the sibling file does not exist on the hub. - Revision Pinning Recommendation: Currently, the downloads do not specify a
revision(defaulting to"main"). If a new commit is pushed to the repository between the download of the.onnxfile and the.onnx_datafile, they could be resolved to different commit hashes, leading to mismatched files or a missing sibling error. Pinning the revision to a specific commit hash (e.g., via a_EMBEDDINGGEMMA_REVISIONconstant) ensures consistency, prevents breaking changes from upstream updates, and allowshuggingface_hubto load cached files instantly without making remote HEAD requests.
Suggested change
| try: | |
| hf_hub_download( | |
| _EMBEDDINGGEMMA_REPO, | |
| subfolder="onnx", | |
| filename=_EMBEDDINGGEMMA_ONNX + "_data", | |
| ) | |
| except Exception: | |
| # Some exports inline their weights — a missing sibling is non-fatal. | |
| logger.debug("No external-data sibling for %s", _EMBEDDINGGEMMA_ONNX) | |
| try: | |
| from huggingface_hub.utils import EntryNotFoundError | |
| hf_hub_download( | |
| _EMBEDDINGGEMMA_REPO, | |
| subfolder="onnx", | |
| filename=_EMBEDDINGGEMMA_ONNX + "_data", | |
| ) | |
| except EntryNotFoundError: | |
| # Some exports inline their weights — a missing sibling is non-fatal. | |
| logger.debug("No external-data sibling for %s", _EMBEDDINGGEMMA_ONNX) |
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.
Problem
MEMPALACE_EMBEDDING_MODEL=embeddinggemmacrashes on first use on a clean cache:The q8 export (
onnx-community/embeddinggemma-300m-ONNX) keeps its weights in a siblingmodel_quantized.onnx_datafile that the.onnxreferences by relative path.EmbeddinggemmaONNX._lazy_load()onlyhf_hub_downloadsmodel_quantized.onnx, so the weights file is never fetched and onnxruntime can't resolve it.Fix
Also fetch the
.onnx_datasibling into the same snapshot dir (one extrahf_hub_download), wrapped in try/except so inlined-weight exports without a sibling stay non-fatal.Repro / verification
model_quantized.onnxpresent in the snapshotonnx/dir)..onnx_datalands next to the.onnx; embeddinggemma loads and embeds. Verified the model works once both files are co-located — cross-lingual FR/EN cosine 0.89 (parallel) vs 0.40 (unrelated) on a 6-pair probe, matching the model card's multilingual claim.No behavior change for
minilm. Single-file additive fetch; honors the local-first principle (still HF-hosted weights, same as before — just the missing half).