Skip to content
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.

Sign up for GitHub

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

Better LLM retry behavior #6557

Merged
merged 9 commits into from
Feb 17, 2025
Merged

Better LLM retry behavior #6557

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc0bf44
stop retrying on all exceptions
rbren Jan 30, 2025
4019cfe
fix retry behavior
rbren Jan 30, 2025
c46665c
fix tests
rbren Jan 30, 2025
ee06747
fix test
rbren Jan 31, 2025
6a829b7
Merge branch 'main' into rb/no-retry-llm
rbren Jan 31, 2025
e94c57d
Merge branch 'main' into rb/no-retry-llm
enyst Feb 3, 2025
e81b312
Update openhands/llm/llm.py
enyst Feb 3, 2025
11937d0
Merge branch 'main' into rb/no-retry-llm
rbren Feb 13, 2025
185288b
[rbren no-retries] add user-friendly messages (#6576)
enyst Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions openhands/llm/llm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@
from litellm import completion as litellm_completion
from litellm import completion_cost as litellm_completion_cost
from litellm.exceptions import (
APIConnectionError,
APIError,
InternalServerError,
RateLimitError,
ServiceUnavailableError,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

503 is a transitory error, we could probably keep it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It's transitory but also unexpected...

I'm open to it but I lean towards telling the user their LLM is flaking out rather than OpenHands looking like it's slow

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda agree with you actually. We always had a problem in understanding our retry settings, because it's a bit weird to figure out a sensible default for "unexpected stuff happened".

And now we do allow the user to continue normally after reporting the error.

eval is the exception, I'd love to hear from Xingyao on that.

)
from litellm.types.utils import CostPerToken, ModelResponse, Usage
from litellm.utils import create_pretrained_tokenizer
Expand All @@ -42,15 +39,7 @@
__all__ = ['LLM']

# tuple of exceptions to retry on
LLM_RETRY_EXCEPTIONS: tuple[type[Exception], ...] = (
APIConnectionError,
# FIXME: APIError is useful on 502 from a proxy for example,
# but it also retries on other errors that are permanent
APIError,
InternalServerError,
RateLimitError,
ServiceUnavailableError,
)
LLM_RETRY_EXCEPTIONS: tuple[type[Exception], ...] = (RateLimitError,)

# cache prompt supporting models
# remove this when we gemini and deepseek are supported
Expand Down
Loading