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.

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

(feat) Improve retry_decorator with blacklist exceptions #4484

Open
tobitege opened this issue Oct 18, 2024 · 5 comments · May be fixed by #5333
Open

(feat) Improve retry_decorator with blacklist exceptions #4484

tobitege opened this issue Oct 18, 2024 · 5 comments · May be fixed by #5333
Labels
enhancement New feature or request fix-me Attempt to fix this issue with OpenHands

Comments

@tobitege
Copy link
Collaborator

What problem or use case are you trying to solve?

In #4482 a specific exception (for Cloudflare) needed to be treated as non-retryable even though its base exception was APIError.
The latter is part of the list of retriable exceptions.

cc @neubig

Do you have thoughts on the technical implementation?

I asked llama-3.1-nemotron-70b-instruct to come up with an improvement (related files: retry_mixin.py, llm.py):

Q: How could the retry_decorator be enhanced to specify a list of exception types, that should NOT be retried, even if the base exception is part of one in the retry_exceptions list.
Example: see LLM_RETRY_EXCEPTIONS, where APIError is a generic exception that could contain text in the error message that would qualify to NOT be retried?

Answer:

Enhancing retry_decorator to Exclude Specific Exception Types

To achieve this, we'll modify the retry_decorator to accept an additional parameter, exclude_exceptions, which will contain a list of exception types that should not be retried, even if their base exception is in retry_exceptions.

Updated Code

class RetryMixin:
   ...

    def retry_decorator(self, **kwargs):
       ...
        retry_exceptions = kwargs.get('retry_exceptions')
        exclude_exceptions = kwargs.get('exclude_exceptions', [])  # New parameter
       ...

        def _should_retry(exception):
            """Check if an exception should be retried."""
            return (
                isinstance(exception, tuple(retry_exceptions))
                and not isinstance(exception, tuple(exclude_exceptions))
            )

        return retry(
           ...
            retry=(retry_if_exception_type(_should_retry)),  # Update retry condition
           ...
        )

Explanation

  1. We added a new parameter exclude_exceptions to retry_decorator, defaulting to an empty list.
  2. We defined a new inner function _should_retry that takes an exception as input.
  3. _should_retry checks if the exception is an instance of any type in retry_exceptions but not an instance of any type in exclude_exceptions.
  4. We updated the retry decorator to use _should_retry as the condition for retrying exceptions.

Example Usage

LLM_RETRY_EXCEPTIONS = (
    APIConnectionError,
    APIError,  # Generic exception that might contain non-retryable errors
   ...
)

LLM_EXCLUDE_EXCEPTIONS = (  # New tuple for exceptions to exclude
    APIErrorIfContainsText,  # Custom exception that checks for specific text in APIError
)

...

@self.retry_decorator(
    num_retries=self.config.num_retries,
    retry_exceptions=LLM_RETRY_EXCEPTIONS,
    exclude_exceptions=LLM_EXCLUDE_EXCEPTIONS,  # Pass the exclude exceptions
   ...
)
def wrapper(*args, **kwargs):
   ...

Custom Exception Example (APIErrorIfContainsText)

class APIErrorIfContainsText(APIError):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.text_to_check = "specific error text that should not be retried"

    def __instancecheck__(self, instance):
        return (
            super().__instancecheck__(instance)
            and self.text_to_check in instance.response.text
        )

In this example, APIErrorIfContainsText is a custom exception that checks if the APIError response text contains a specific string. If it does, the exception will be excluded from retries.

@tobitege tobitege added the enhancement New feature or request label Oct 18, 2024
@tobitege
Copy link
Collaborator Author

This new exception class APIErrorIfContainsText could then be extended with the exceptions to the rule (pun intended) text occurences for a centralized handler.

Copy link
Contributor

This issue is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale Inactive for 30 days label Nov 19, 2024
@enyst
Copy link
Collaborator

enyst commented Nov 19, 2024

There is an issue over at litellm on this: BerriAI/litellm#6550

IMHO this solution is better than what we currently have, in the meantime. The code with the __instancecheck__ will need added on the metaclass, though. Source: bug in python

@github-actions github-actions bot removed the Stale Inactive for 30 days label Nov 20, 2024
@enyst enyst added the fix-me Attempt to fix this issue with OpenHands label Nov 30, 2024
Copy link
Contributor

OpenHands started fixing the issue! You can monitor the progress here.

Copy link
Contributor

A potential fix has been generated and a draft PR #5333 has been created. Please review the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix-me Attempt to fix this issue with OpenHands
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants