refactor: refactor test local inference#952
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
📝 WalkthroughWalkthroughThe test suite in A new pytest fixture named Constants ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/embed_tests/test_local_inference.py (5)
33-37: Consider refactoring star-arg usage after a named argument.Static analysis flags the pattern
TextEmbedding(model_name=model_name, *args, **kwargs)as B026. While it works, some style guides discourage mixing named arguments and star-args in this manner.🧰 Tools
🪛 Ruff (0.8.2)
36-36: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
39-42: Consider refactoring star-arg usage after a named argument.Identical reasoning as above for
_cached_sparse_text_embedding.🧰 Tools
🪛 Ruff (0.8.2)
41-41: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
44-47: Consider refactoring star-arg usage after a named argument.Identical reasoning as above for
_cached_late_interaction_text_embedding.🧰 Tools
🪛 Ruff (0.8.2)
46-46: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
49-52: Consider refactoring star-arg usage after a named argument.Identical reasoning as above for
_cached_image_embedding.🧰 Tools
🪛 Ruff (0.8.2)
51-51: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
379-379: Remove unused variable.
retrieved_point_id_0is set but never referenced, triggering a static analysis warning. Consider removing it:- retrieved_point_id_0 = local_client.retrieve(COLLECTION_NAME, ids=[0], with_vectors=True)[0] + local_client.retrieve(COLLECTION_NAME, ids=[0], with_vectors=True)🧰 Tools
🪛 Ruff (0.8.2)
379-379: Local variable
retrieved_point_id_0is assigned to but never usedRemove assignment to unused variable
retrieved_point_id_0(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/embed_tests/test_local_inference.py(27 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/embed_tests/test_local_inference.py
36-36: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
41-41: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
46-46: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
51-51: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
379-379: Local variable retrieved_point_id_0 is assigned to but never used
Remove assignment to unused variable retrieved_point_id_0
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
🔇 Additional comments (34)
tests/embed_tests/test_local_inference.py (34)
3-5: No concerns.These import statements appear valid and straightforward.
10-10: No issues found.Importing the embedder is necessary for the mock patches and embedding logic.
14-19: No issues found.These imports from
fastembed_commonappear correct and in line with test objectives.
54-82: Fixture implementation looks good.Leveraging
lru_cachefor embedding objects and patching them in a fixture is a neat way to speed up tests and reduce repetitive object creation.
133-133: No issues found.The definition of
test_upsert(cached_embeddings)appears valid and aligned with the test flow.
240-259: Confirm necessity ofremote_clientusage.According to the PR objectives, redundant references to remote vs local may be unnecessary. Please verify if these lines and assertions truly need
remote_client, or if everything can be tested locally.
288-293: No issues found.
303-303: No issues found.
305-307: No issues found.
311-315: No issues found.
322-326: No issues found.
333-337: No issues found.
380-380: No issues found.
412-412: No issues found.
495-507: No issues found.
517-518: No issues found.
584-588: No issues found.
607-611: No issues found.
639-644: No issues found.
646-650: No issues found.
662-663: No issues found.
669-669: No issues found.
684-688: No issues found.
759-759: No issues found.
767-767: No issues found.
771-782: No issues found.
783-784: No issues found.
826-827: No issues found.
1000-1003: No issues found.
1095-1100: No issues found.
1118-1122: Confirm necessity ofremote_client.This test still relies on
remote_client. Please confirm whether remote inference is intentionally preserved or could be simplified.
1140-1140: No issues found.
1337-1340: No issues found.
1437-1439: No issues found.
2 participants
this refactoring reduces the time required to run these tests on local machine from 3.5 minutes to ~40 seconds
After reviewing the tests, I figured out, that congruence tests are not required here, cuz here we're testing the code which is the same for both local and remote modes, also it does not make sense running it with both http and grpc for the same reason.
Also @hh-space-invader tried caching embedding generation itself, however, after a couple of benchmarks, I've got the results which show that caching embedding generation won't bring as much benefits