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: bedrock encoder merge #307

Merged
merged 8 commits into from
Jun 2, 2024
Merged

feat: bedrock encoder merge #307

merged 8 commits into from
Jun 2, 2024

Conversation

jamescalam
Copy link
Member

@jamescalam jamescalam commented Jun 2, 2024

PR Type

Bug fix, Enhancement, Tests, Documentation


Description

  • Fixed environment variable names for AWS credentials in BedrockEncoder.
  • Added retry logic for handling expired tokens in BedrockEncoder.
  • Refactored chunking logic into a separate method in BedrockEncoder.
  • Improved error handling and logging in BedrockEncoder.
  • Added comprehensive tests for BedrockEncoder, including retry logic, chunking functionality, and various initialization scenarios.
  • Updated dependencies to include botocore in pyproject.toml.

Changes walkthrough 📝

Relevant files
Bug fix
bedrock.py
Fix and enhance BedrockEncoder initialization and API calls

semantic_router/encoders/bedrock.py

  • Fixed environment variable names for AWS credentials.
  • Added retry logic for handling expired tokens.
  • Refactored chunking logic into a separate method.
  • Improved error handling and logging.
  • +112/-88
    Tests
    test_bedrock.py
    Add comprehensive tests for BedrockEncoder                             

    tests/unit/encoders/test_bedrock.py

  • Added tests for new retry logic and chunking functionality.
  • Added tests for various initialization scenarios.
  • Improved test coverage for error handling.
  • +151/-21
    Dependencies
    pyproject.toml
    Update dependencies to include botocore                                   

    pyproject.toml

  • Added botocore as an optional dependency.
  • Updated bedrock extra to include botocore.
  • +3/-2     

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

    @github-actions github-actions bot added documentation Improvements or additions to documentation enhancement Enhancement to existing features Bug fix Tests labels Jun 2, 2024
    Copy link

    github-actions bot commented Jun 2, 2024

    PR Description updated to latest commit (a93b348)

    Copy link

    github-actions bot commented Jun 2, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple complex changes including refactoring, error handling, and retry logic, which require careful review to ensure correctness and robustness.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Retry Logic Concern: The retry logic for handling expired tokens might not correctly reinitialize the client if the session token remains expired or invalid. This could lead to repeated failures without proper handling of the new token acquisition.

    Exception Handling: The broad exception handling in several methods could mask specific errors, making debugging more difficult. It's generally better to catch more specific exceptions where possible.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    Consider adding a backoff strategy for the retry logic to handle API rate limits or network issues more gracefully. This can prevent the system from being overwhelmed by repeated quick retries. [important]

    relevant linesleep(2**attempt)

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    It's recommended to handle specific exceptions from the Boto3 and Botocore libraries instead of a general exception, which can help in identifying the root cause of failures more accurately. [important]

    relevant lineexcept Exception as e:

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    To improve the security and robustness of token handling, consider implementing a method to refresh the session token automatically when it expires, instead of relying on the environment variable that might not update in real-time. [important]

    relevant lineself.session_token = os.getenv("AWS_SESSION_TOKEN")

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    Refactor the chunk_strings method to handle edge cases where the text might not need chunking or when the chunks are smaller than MAX_WORDS. This can prevent unnecessary processing and potential errors. [medium]

    relevant linedef chunk_strings(self, strings, MAX_WORDS=20):

    Copy link

    github-actions bot commented Jun 2, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use specific exceptions for better error handling

    Instead of catching a general Exception, specify the types of exceptions that are expected
    during client initialization. This will help in better error handling and debugging.

    semantic_router/encoders/bedrock.py [216]

    -except Exception as e:
    +except (SpecificExceptionType1, SpecificExceptionType2) as e:
     
    Suggestion importance[1-10]: 8

    Why: Using specific exceptions improves error handling and debugging, making the code more robust and easier to maintain. This is a best practice that enhances code quality significantly.

    8
    Enhancement
    Replace hardcoded retry delay with a configurable variable

    Replace the hardcoded retry delay with a variable that can be adjusted based on the
    environment or configuration settings. This will make the retry mechanism more flexible
    and maintainable.

    semantic_router/encoders/bedrock.py [220]

    -sleep(2**attempt)
    +sleep(retry_delay_seconds)
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the flexibility and maintainability of the retry mechanism by allowing the retry delay to be configurable. However, it is not a critical change and does not address a major bug or issue.

    7
    Make the MAX_WORDS parameter adjustable in the chunk_strings method

    The method chunk_strings uses a hardcoded MAX_WORDS value for chunking, which might not be
    optimal for all scenarios. Consider making MAX_WORDS a parameter of the method to allow
    flexibility.

    semantic_router/encoders/bedrock.py [229]

    -def chunk_strings(self, strings, MAX_WORDS=20):
    +def chunk_strings(self, strings, max_words=20):
     
    Suggestion importance[1-10]: 6

    Why: Making MAX_WORDS adjustable adds flexibility to the chunk_strings method, which can be beneficial for different use cases. However, it is a minor enhancement and does not address any critical issues.

    6
    Maintainability
    Refactor error handling in retry logic for clarity

    Refactor the error handling in the retry logic to separate concerns and improve
    readability. Specifically, handle the token expiration and other client errors in
    different blocks or methods.

    semantic_router/encoders/bedrock.py [204-217]

     if error.response["Error"]["Code"] == "ExpiredTokenException":
    -    logger.warning("Session token has expired. Retrying initialisation.")
    -    try:
    -        self.session_token = os.getenv("AWS_SESSION_TOKEN")
    -        self.client = self._initialize_client(
    -            self.access_key_id,
    -            self.secret_access_key,
    -            self.session_token,
    -            self.region,
    -        )
    -    except Exception as e:
    -        raise ValueError(f"Bedrock client failed to reinitialise. Error: {e}") from e
    +    handle_token_expiration()
    +else:
    +    handle_client_error(error)
     
    Suggestion importance[1-10]: 7

    Why: This refactoring improves code readability and maintainability by separating concerns in the error handling logic. While it enhances the structure of the code, it is not addressing a critical issue.

    7

    @jamescalam jamescalam merged commit 41a5874 into main Jun 2, 2024
    6 checks passed
    @jamescalam jamescalam deleted the james/bedrock-encoder-merge branch June 2, 2024 12:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix documentation Improvements or additions to documentation enhancement Enhancement to existing features Review effort [1-5]: 4 Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants