-
Notifications
You must be signed in to change notification settings - Fork 35
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
add JinaEncoder class to handle Jina Embeddings #59
add JinaEncoder class to handle Jina Embeddings #59
Conversation
Adding unit tests to class |
Done |
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.
@JoanFM thank you very much for your contribution!
Please see only one comment about missing a system (==integration) test. Other than that LGTM
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.
For encoders that use external APIs, on top of the highly mocked unit test we also have a system test that verifies integration with the actual embedding service. Please see tests/system/test_openai_encoder
for example.
Can you please add a similar system test for JinaEncoder?
These tests can assume that JINA_API_KEY
env var has a valid API key, and we will add a relevant key to the CI's secrets.
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.
Hey @igiloh-pinecone ,
System test added as request. Please check
The tests in this director will not be runnig in every PR's CI workflow, but in separate daily scheduled CI runs
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.
LGTM!
Head branch was pushed to by a user without write access
Adding JinaEncoder class to allow users to use Jina Embeddings using Jina Embedding API https://jina.ai/embeddings/
Type of Change
Test Plan
Unit tests have been added