-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ENH]: Client side retries #5419
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: 09-02-_enh_consolidate_retries
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ | |||||||||
from overrides import override | ||||||||||
from typing_extensions import Literal | ||||||||||
import platform | ||||||||||
from pydantic import BaseModel | ||||||||||
|
||||||||||
in_pydantic_v2 = False | ||||||||||
try: | ||||||||||
|
@@ -97,6 +98,14 @@ class APIVersion(str, Enum): | |||||||||
V2 = "/api/v2" | ||||||||||
|
||||||||||
|
||||||||||
class RetryConfig(BaseModel): | ||||||||||
factor: float = 2.0 | ||||||||||
min_delay: int = 1 | ||||||||||
max_delay: int = 5 | ||||||||||
Comment on lines
+103
to
+104
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. [BestPractice] For consistency with the Javascript client and to allow for more granular control over backoff timing, consider changing
Suggested change
⚡ Committable suggestion Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Context for Agents
|
||||||||||
max_attempts: int = 5 | ||||||||||
jitter: bool = True | ||||||||||
|
||||||||||
|
||||||||||
# NOTE(hammadb) 1/13/2024 - This has to be in config.py instead of being localized to the module | ||||||||||
# that uses it because of a circular import issue. This is a temporary solution until we can | ||||||||||
# refactor the code to remove the circular import. | ||||||||||
|
@@ -133,6 +142,8 @@ def empty_str_to_none(cls, v: str) -> Optional[str]: | |||||||||
return None | ||||||||||
return v | ||||||||||
|
||||||||||
retry_config: Optional[RetryConfig] = RetryConfig() | ||||||||||
|
||||||||||
chroma_server_nofile: Optional[int] = None | ||||||||||
# the number of maximum threads to handle synchronous tasks in the FastAPI server | ||||||||||
chroma_server_thread_pool_size: int = 40 | ||||||||||
|
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.
[BestPractice]
[CodeDuplication] The retry logic implemented in
_make_request
is nearly identical to the logic inchromadb/api/async_fastapi.py
's_make_request
method (lines 153-209). This introduces code duplication, making future maintenance harder.Consider refactoring the common retry configuration and execution logic into a shared helper function. This would centralize the retry strategy and reduce redundancy. For example, a helper could build the
Retrying
orAsyncRetrying
object based on a flag.Context for Agents