-
Notifications
You must be signed in to change notification settings - Fork 739
fix(embeddings): recover missing FastEmbed ONNX cache #1955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,12 +16,54 @@ | |||||||||||||||||
| import os | ||||||||||||||||||
|
|
||||||||||||||||||
| import pytest | ||||||||||||||||||
| from onnxruntime.capi.onnxruntime_pybind11_state import NoSuchFile | ||||||||||||||||||
|
|
||||||||||||||||||
| from nemoguardrails.embeddings.providers.fastembed import FastEmbedEmbeddingModel | ||||||||||||||||||
|
Comment on lines
18
to
21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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. |
||||||||||||||||||
|
|
||||||||||||||||||
| LIVE_TEST_MODE = os.environ.get("LIVE_TEST") | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| class _FakeEmbeddingVector: | ||||||||||||||||||
| def tolist(self): | ||||||||||||||||||
| return [0.1, 0.2] | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def test_recovers_from_missing_onnxruntime_model_cache(monkeypatch): | ||||||||||||||||||
| calls = [] | ||||||||||||||||||
|
|
||||||||||||||||||
| class FakeTextEmbedding: | ||||||||||||||||||
| def __init__(self, model_name, **kwargs): | ||||||||||||||||||
| calls.append((model_name, kwargs)) | ||||||||||||||||||
| if len(calls) == 1: | ||||||||||||||||||
| raise NoSuchFile( | ||||||||||||||||||
| "[ONNXRuntimeError] : 3 : NO_SUCHFILE : Load model from /tmp/model.onnx failed. File doesn't exist" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
Comment on lines
+37
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis 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! |
||||||||||||||||||
|
|
||||||||||||||||||
| def embed(self, documents): | ||||||||||||||||||
| return [_FakeEmbeddingVector()] | ||||||||||||||||||
|
|
||||||||||||||||||
| monkeypatch.setattr("fastembed.TextEmbedding", FakeTextEmbedding) | ||||||||||||||||||
|
|
||||||||||||||||||
| model = FastEmbedEmbeddingModel("all-MiniLM-L6-v2") | ||||||||||||||||||
|
|
||||||||||||||||||
| assert model.embedding_size == 2 | ||||||||||||||||||
| assert calls == [ | ||||||||||||||||||
| ("sentence-transformers/all-MiniLM-L6-v2", {}), | ||||||||||||||||||
| ("sentence-transformers/all-MiniLM-L6-v2", {"cache_dir": ".cache"}), | ||||||||||||||||||
| ] | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def test_reraises_unrelated_fastembed_errors(monkeypatch): | ||||||||||||||||||
| class FakeTextEmbedding: | ||||||||||||||||||
| def __init__(self, model_name, **kwargs): | ||||||||||||||||||
| raise ValueError("unrelated failure") | ||||||||||||||||||
|
|
||||||||||||||||||
| monkeypatch.setattr("fastembed.TextEmbedding", FakeTextEmbedding) | ||||||||||||||||||
|
|
||||||||||||||||||
| with pytest.raises(ValueError, match="unrelated failure"): | ||||||||||||||||||
| FastEmbedEmbeddingModel("all-MiniLM-L6-v2") | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| @pytest.mark.skipif(not LIVE_TEST_MODE, reason="Not in live mode.") | ||||||||||||||||||
| def test_sync_embeddings(): | ||||||||||||||||||
| model = FastEmbedEmbeddingModel("all-MiniLM-L6-v2") | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this an absolute path eg:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or alternatively, log a warning when overriding the value