fix(embeddings): recover missing FastEmbed ONNX cache#1955
Conversation
Retry FastEmbed initialization with an explicit local cache when ONNXRuntime reports a missing model.onnx from a stale cache.
Greptile SummaryThis PR widens FastEmbed initialization error handling so that
|
| Filename | Overview |
|---|---|
| nemoguardrails/embeddings/providers/fastembed.py | Introduces _is_missing_onnx_model_error helper to detect stale ONNX cache errors by string matching; widens the caught exception type from ValueError to Exception (needed since NoSuchFile is not a ValueError); correctly uses raise instead of raise ex to preserve the original traceback; copies kwargs before mutating to avoid side effects. |
| tests/test_embeddings_fastembed.py | Adds two new unit tests covering the cache-recovery path and unrelated-error re-raise. The top-level from onnxruntime... import NoSuchFile creates a hard dependency that can break test-collection in environments without onnxruntime; using RuntimeError with the same message would be sufficient since the production helper only checks the string content. |
Sequence Diagram
sequenceDiagram
participant Caller
participant FastEmbedEmbeddingModel
participant Embedding as fastembed.TextEmbedding
participant Helper as _is_missing_onnx_model_error
Caller->>FastEmbedEmbeddingModel: "__init__(embedding_model, **kwargs)"
FastEmbedEmbeddingModel->>Embedding: "Embedding(embedding_model, **kwargs)"
alt Success
Embedding-->>FastEmbedEmbeddingModel: model instance
else Exception raised
Embedding-->>FastEmbedEmbeddingModel: raises Exception (e.g. NoSuchFile)
FastEmbedEmbeddingModel->>Helper: _is_missing_onnx_model_error(ex)
alt ONNX cache miss detected
Helper-->>FastEmbedEmbeddingModel: True
FastEmbedEmbeddingModel->>Embedding: "Embedding(embedding_model, cache_dir=".cache", **kwargs)"
Embedding-->>FastEmbedEmbeddingModel: model instance (downloads to .cache)
else Unrelated error
Helper-->>FastEmbedEmbeddingModel: False
FastEmbedEmbeddingModel-->>Caller: raise (original exception + traceback)
end
end
FastEmbedEmbeddingModel->>FastEmbedEmbeddingModel: "self.embedding_size = len(embed("test"))"
FastEmbedEmbeddingModel-->>Caller: model ready
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
tests/test_embeddings_fastembed.py:18-21
Top-level import of `onnxruntime.capi.onnxruntime_pybind11_state.NoSuchFile` makes the entire test module unimportable in any environment that lacks `onnxruntime`. Since `_is_missing_onnx_model_error` only inspects the string message (not the exception type), there's no need to use the real ONNXRuntime class — a plain `RuntimeError` with the same message text exercises exactly the same code path without the hard dependency.
```suggestion
import pytest
from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel
```
### Issue 2 of 2
tests/test_embeddings_fastembed.py:37-40
After removing the top-level `NoSuchFile` import, the test can use a plain `RuntimeError` with the identical message. The production helper only string-matches on the message, so this change preserves full test coverage while eliminating the `onnxruntime` hard dependency.
```suggestion
if len(calls) == 1:
raise RuntimeError(
"[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist"
)
```
Reviews (1): Last reviewed commit: "fix(embeddings): recover missing FastEmb..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import pytest | ||
| from onnxruntime.capi.onnxruntime_pybind11_state import NoSuchFile | ||
|
|
||
| from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel |
There was a problem hiding this comment.
onnxruntime.capi.onnxruntime_pybind11_state.NoSuchFile makes the entire test module unimportable in any environment that lacks onnxruntime. Since _is_missing_onnx_model_error only inspects the string message (not the exception type), there's no need to use the real ONNXRuntime class — a plain RuntimeError with the same message text exercises exactly the same code path without the hard dependency.
| import pytest | |
| from onnxruntime.capi.onnxruntime_pybind11_state import NoSuchFile | |
| from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel | |
| import pytest | |
| from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_embeddings_fastembed.py
Line: 18-21
Comment:
Top-level import of `onnxruntime.capi.onnxruntime_pybind11_state.NoSuchFile` makes the entire test module unimportable in any environment that lacks `onnxruntime`. Since `_is_missing_onnx_model_error` only inspects the string message (not the exception type), there's no need to use the real ONNXRuntime class — a plain `RuntimeError` with the same message text exercises exactly the same code path without the hard dependency.
```suggestion
import pytest
from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel
```
How can I resolve this? If you propose a fix, please make it concise.| if len(calls) == 1: | ||
| raise NoSuchFile( | ||
| "[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist" | ||
| ) |
There was a problem hiding this comment.
NoSuchFile import, the test can use a plain RuntimeError with the identical message. The production helper only string-matches on the message, so this change preserves full test coverage while eliminating the onnxruntime hard dependency.
| if len(calls) == 1: | |
| raise NoSuchFile( | |
| "[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist" | |
| ) | |
| if len(calls) == 1: | |
| raise RuntimeError( | |
| "[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist" | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_embeddings_fastembed.py
Line: 37-40
Comment:
After removing the top-level `NoSuchFile` import, the test can use a plain `RuntimeError` with the identical message. The production helper only string-matches on the message, so this change preserves full test coverage while eliminating the `onnxruntime` hard dependency.
```suggestion
if len(calls) == 1:
raise RuntimeError(
"[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist"
)
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR adds automatic fallback recovery when FastEmbedEmbeddingModel encounters a missing ONNX model cache during initialization. A new helper function detects the specific error pattern, triggers a retry with an explicit cache directory, and allows unrelated errors to propagate. Two unit tests verify both the recovery path and correct error re-raising behavior. ChangesONNX Model Cache Recovery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_embeddings_fastembed.py (1)
19-19: ⚡ Quick winUnconditional module imports are covered by default deps (CI), but still make tests require onnxruntime/fastembed
tests/test_embeddings_fastembed.pyimportsNoSuchFileandFastEmbedEmbeddingModelat module import time, butpyproject.tomldeclares bothonnxruntimeandfastembedunder[tool.poetry.dependencies](non-optional), and CI installs them withpoetry install --with devbefore running pytest—so collection should not fail in the standard test environment. If the goal is to allow running these tests without the embedding stack, make the imports lazy (inside tests) or usepytest.importorskip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_embeddings_fastembed.py` at line 19, The test imports (NoSuchFile and FastEmbedEmbeddingModel) at module scope causing test collection to fail when onnxruntime/fastembed aren't present; make these imports conditional by moving them into the test functions or using pytest.importorskip at the top of the test file to skip the module when dependencies are missing (e.g., call pytest.importorskip("onnxruntime") and pytest.importorskip("fastembed") or wrap imports of NoSuchFile and FastEmbedEmbeddingModel inside the specific tests that use them), ensuring tests/test_embeddings_fastembed.py can be collected without the embedding stack.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_embeddings_fastembed.py`:
- Line 19: The test imports (NoSuchFile and FastEmbedEmbeddingModel) at module
scope causing test collection to fail when onnxruntime/fastembed aren't present;
make these imports conditional by moving them into the test functions or using
pytest.importorskip at the top of the test file to skip the module when
dependencies are missing (e.g., call pytest.importorskip("onnxruntime") and
pytest.importorskip("fastembed") or wrap imports of NoSuchFile and
FastEmbedEmbeddingModel inside the specific tests that use them), ensuring
tests/test_embeddings_fastembed.py can be collected without the embedding stack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 91a656f4-5503-44a3-95dd-76c7833a0f0f
📒 Files selected for processing (2)
nemoguardrails/embeddings/providers/fastembed.pytests/test_embeddings_fastembed.py
github-actions
Bot
added
status: needs triage
| self.model = Embedding(embedding_model, cache_dir=".cache", **kwargs) | ||
| if _is_missing_onnx_model_error(ex): | ||
| fallback_kwargs = dict(kwargs) | ||
| fallback_kwargs["cache_dir"] = ".cache" |
There was a problem hiding this comment.
can we make this an absolute path eg:
fallback_kwargs["cache_dir"] = str(Path.home() / ".cache" / "fastembed")
my concern is that is we use a relative path, it will fail in docker containers since cwd is usually / or read only
There was a problem hiding this comment.
or alternatively, log a warning when overriding the value
2 participants
Description
Retry FastEmbed initialization with an explicit local cache when ONNXRuntime reports a missing model.onnx from a stale cache.
Summary by CodeRabbit