-
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
Implement retry for LLMClient #44
Conversation
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 52f5f10 in 13 seconds
More details
- Looked at
69
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. graphiti_core/llm_client/client.py:65
- Draft comment:
The_is_server_error
method should be used to check an instance of an exception, not the exception class itself. Pass the exception instance to_is_server_error
. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_QmpODbYjon2Lxyrs
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.
Skipped PR review on ced3fab because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away. Generated with ❤️ by ellipsis.dev |
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.
👍 Looks good to me! Incremental review on b32d46f in 25 seconds
More details
- Looked at
21
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. graphiti_core/llm_client/client.py:91
- Draft comment:
Consider expanding the retry logic to handle other transient errors, such as network issues, not just server errors. This can improve the robustness of the retry mechanism. - Reason this comment was not posted:
Confidence changes required:50%
The retry logic is implemented using the tenacity library, which is a good choice for handling retries. However, the retry logic is only applied to server errors (HTTP 5xx). The code should also handle other types of transient errors, such as network issues, which are not currently covered by the is_server_error function.
2. graphiti_core/llm_client/client.py:91
- Draft comment:
Consider adding comments to explain the retry logic in_generate_response_with_retry
for better clarity. - Reason this comment was not posted:
Confidence changes required:50%
The code is mostly well-structured, but there are a few areas that could be improved for clarity and maintainability.
Workflow ID: wflow_AcCVCLAmye20HY1I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This pull request implements retry functionality for the
LLMClient
class in theclient.py
file. The_generate_response_with_retry
method has been added, which retries the generation of a response in case of a server error. Thetenacity
library is used for the retry logic. This enhancement improves the reliability of the client by handling temporary server errors gracefully.Summary:
This PR adds retry functionality to
LLMClient
usingtenacity
for handling server errors and updates dependencies.Key points:
LLMClient
ingraphiti_core/llm_client/client.py
.tenacity
for retry logic with exponential backoff._generate_response_with_retry
method for handling server errors.generate_response
to use retry logic.pyproject.toml
to includetenacity
.Generated with ❤️ by ellipsis.dev