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: add max_retries to OpenAI and Azure encoders #389

Merged
merged 10 commits into from
Aug 19, 2024

Conversation

ashraq1455
Copy link
Contributor

@ashraq1455 ashraq1455 commented Aug 18, 2024

User description

This update introduces a max_retries option for the OpenAIEncoder and AzureOpenAIEncoder classes. This option allows users to specify the maximum number of retry attempts for API calls in case of errors. By default, it is set to 3 retries. If no retries are needed, you can set max_retries to 0.

Example:

from semantic_router.encoders import OpenAIEncoder

encoder = OpenAIEncoder(name="text-embedding-3-large", max_retries=2)

PR Type

enhancement


Description

  • Introduced a max_retries option for both OpenAIEncoder and AzureOpenAIEncoder classes, allowing users to specify the maximum number of retry attempts for API calls.
  • Implemented exponential backoff retry logic for handling API call failures in both synchronous and asynchronous methods.
  • Enhanced error logging to provide more detailed information on exceptions.
  • Removed redundant error message handling to streamline code.

Changes walkthrough 📝

Relevant files
Enhancement
openai.py
Add retry logic and error handling to OpenAIEncoder           

semantic_router/encoders/openai.py

  • Added max_retries attribute to OpenAIEncoder class with a default
    value of 3.
  • Implemented retry logic using exponential backoff for API calls.
  • Improved error logging and handling for API call failures.
  • Removed redundant error message variable.
  • +26/-15 
    zure.py
    Add retry logic and error handling to AzureOpenAIEncoder 

    semantic_router/encoders/zure.py

  • Added max_retries attribute to AzureOpenAIEncoder class with a default
    value of 3.
  • Implemented retry logic using exponential backoff for API calls.
  • Improved error logging and handling for API call failures.
  • Removed redundant error message variable.
  • +25/-22 

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @ashraq1455 ashraq1455 changed the title Ashraq/openai max retry feat: add max_retries option for OpenAI and AzureOpenAI encoders Aug 18, 2024
    @github-actions github-actions bot added the enhancement Enhancement to existing features label Aug 18, 2024
    @ashraq1455 ashraq1455 changed the title feat: add max_retries option for OpenAI and AzureOpenAI encoders feat: add max_retries to OpenAI and Azure encoders Aug 18, 2024
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Exception Handling
    The exception handling in the retry logic raises a generic ValueError with the message "OpenAI API call failed." This could be improved by providing more specific error types or more detailed error messages to help with debugging and error resolution.

    Hardcoded Test Exception
    There is a hardcoded raise OpenAIError("Test") within the retry logic. This seems to be for testing purposes and should be removed or commented out for production code.

    Exception Handling
    Similar to openai.py, the exception handling could be more specific than just raising a ValueError with a generic message. More specific exceptions or messages could aid in better error handling and debugging.

    Hardcoded Test Exception
    The retry logic includes a raise OpenAIError("Test") which appears to be for testing. This should be removed or properly handled before moving to production.

    Copy link

    github-actions bot commented Aug 18, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Remove or replace the test exception raise inside the retry loop

    Replace the hardcoded exception raise inside the retry loop with a proper error
    handling mechanism or remove it if it was for testing purposes.

    semantic_router/encoders/openai.py [117]

    -raise OpenAIError("Test")
    +# Removed test exception raise
     
    Suggestion importance[1-10]: 9

    Why: The hardcoded exception raise is likely for testing and should be removed or replaced with proper error handling to avoid unintended disruptions in production code.

    9
    Enhancement
    Skip retry loop when max_retries is zero

    Implement a mechanism to handle the case when max_retries is zero, to skip the retry
    loop entirely, optimizing performance when no retries are intended.

    semantic_router/encoders/openai.py [115]

    -for j in range(self.max_retries + 1):
    +if self.max_retries > 0:
    +    for j in range(self.max_retries + 1):
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves performance by avoiding unnecessary iterations when no retries are intended, making the code more efficient and logical.

    8
    Cap the maximum sleep time during retries to avoid excessive delays

    Ensure that exponential backoff sleep time increases with each retry attempt by
    adjusting the sleep time calculation.

    semantic_router/encoders/openai.py [128]

    -sleep(2**j)
    +sleep(min(2**j, 120))  # Caps the sleep time to a maximum of 120 seconds
     
    Suggestion importance[1-10]: 8

    Why: Capping the sleep time during retries prevents excessive delays, which is beneficial for maintaining reasonable response times and system performance.

    8
    Possible issue
    Validate max_retries to be a non-negative integer

    The max_retries attribute should be validated to ensure it is a non-negative
    integer. This prevents potential runtime errors or logical bugs due to negative
    retry attempts.

    semantic_router/encoders/openai.py [45]

    -max_retries: int = 3
    +max_retries: int = max(0, int(value))  # Ensures non-negative integer
     
    Suggestion importance[1-10]: 7

    Why: Ensuring max_retries is a non-negative integer is a good practice to prevent logical errors and runtime exceptions. However, the suggested code does not show how value is defined or used, which could lead to confusion.

    7

    @ashraq1455 ashraq1455 requested a review from jamescalam August 18, 2024 11:33
    @jamescalam jamescalam merged commit 592fd3b into main Aug 19, 2024
    3 of 6 checks passed
    @jamescalam jamescalam deleted the ashraq/openai-max-retry branch August 19, 2024 14:38
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    enhancement Enhancement to existing features Review effort [1-5]: 3
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants