-
Notifications
You must be signed in to change notification settings - Fork 159
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
Allow usage of different openai compatible clients in embedder and encoder #279
base: main
Are you sure you want to change the base?
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
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.
❌ Changes requested. Reviewed everything up to 0374108 in 1 minute and 21 seconds
More details
- Looked at
88
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. graphiti_core/cross_encoder/openai_reranker_client.py:44
- Draft comment:
The docstring mentions a 'cache' parameter, which is not present in the method signature. Please update or remove this documentation for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
2. graphiti_core/cross_encoder/openai_reranker_client.py:42
- Draft comment:
Consider using a more specific type than 'Any' for the 'client' parameter if possible, to improve type safety. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold80%
None
3. graphiti_core/embedder/openai.py:40
- Draft comment:
The updated constructor correctly adds an optional 'client' parameter. Ensure consistency with similar patterns in other modules. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold80%
None
4. graphiti_core/cross_encoder/openai_reranker_client.py:44
- Draft comment:
Docstring mentions a 'cache' parameter which is not present. Please update the docstring to reflect the current parameters. - Reason this comment was not posted:
Comment was on unchanged code.
5. graphiti_core/cross_encoder/openai_reranker_client.py:42
- Draft comment:
Consider using a more specific type hint or a protocol instead of 'Any' for the client parameter. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold80%
None
6. graphiti_core/embedder/openai.py:39
- Draft comment:
The constructor does not document the new 'client' parameter. Consider updating the class or constructor docstring to include it. - Reason this comment was not posted:
Confidence changes required:66%
<= threshold80%
None
Workflow ID: wflow_8nQVEGb5gwZwplS2
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
0e038a4
to
ad9fb53
Compare
This pull request addresses following issue: #254
Previously only llm client exposed an option to pass custom compatible client.
This resulted in
OpenAIError: The api_key client option must be set either by passing api_key to the client or by setting the OPENAI_API_KEY environment variable
when creating an embedder and cross encoder.This PR introduces option to pass your own instance of client into both
OpeanAIEmbedder
as well asOpenAIRerankerClient
following the convention fromOpenAIClient
Important
Allow custom client instances in
OpenAIEmbedder
andOpenAIRerankerClient
to fix API key error.OpenAIEmbedder
andOpenAIRerankerClient
constructors.OpenAIError
related to missing API key by allowing client injection.openai_reranker_client.py
: ModifyOpenAIRerankerClient.__init__()
to acceptclient
parameter.openai.py
: ModifyOpenAIEmbedder.__init__()
to acceptclient
parameter.__init__.py
: AddOpenAIRerankerClient
to__all__
for module exports.This description was created by
for 0374108. It will automatically update as commits are pushed.