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: Adding BedrockEncoder #277

Merged
merged 8 commits into from
May 14, 2024
Merged

feat: Adding BedrockEncoder #277

merged 8 commits into from
May 14, 2024

Conversation

theanupllm
Copy link
Contributor

@theanupllm theanupllm commented May 14, 2024

User description

#173 An implementation for Bedrock embedding models.

Now Semantic router will support generating embeddings using AWS Bedrock. It integrates the BedrockEncoder into the encoder modules and adds it to the AutoEncoder class. Additionally, it sets up unit tests for BedrockEncoder functionalities, error handling, and creates a Jupyter notebook documentation for demonstrating its usage. Boto3 has been added as a dependency for AWS operations related to Bedrock.


PR Type

enhancement, documentation


Description

  • Added BedrockEncoder class for embedding generation using AWS Bedrock.
  • Integrated BedrockEncoder into the encoder modules and added it to the AutoEncoder class.
  • Set up unit tests for BedrockEncoder functionalities and error handling.
  • Created a Jupyter notebook documentation for demonstrating the usage of BedrockEncoder.
  • Added boto3 as a dependency for AWS operations related to Bedrock.

Changes walkthrough 📝

Relevant files
Enhancement
__init__.py
Integrate BedrockEncoder into Encoder Modules                       

semantic_router/encoders/init.py

  • Imported BedrockEncoder in the module.
  • Added BedrockEncoder to the list of available encoders.
  • Added conditional initialization for BedrockEncoder in AutoEncoder
    class.
  • +4/-0     
    bedrock.py
    Implement BedrockEncoder with AWS Integration                       

    semantic_router/encoders/bedrock.py

  • Implemented BedrockEncoder class with methods for initialization,
    client setup, and embedding generation.
  • Added error handling and environment variable management for AWS
    credentials and configuration.
  • +250/-0 
    schema.py
    Add BEDROCK Encoder Type                                                                 

    semantic_router/schema.py

    • Added BEDROCK as a new encoder type in the EncoderType enum.
    +1/-0     
    defaults.py
    Set Default Bedrock Embedding Model                                           

    semantic_router/utils/defaults.py

    • Added default embedding model for Bedrock encoder.
    +5/-0     
    Tests
    test_bedrock.py
    Add Unit Tests for BedrockEncoder                                               

    tests/unit/encoders/test_bedrock.py

  • Added unit tests for BedrockEncoder covering initialization, embedding
    generation, and error handling.
  • +116/-0 
    Documentation
    bedrock.ipynb
    Documentation for BedrockEncoder Usage                                     

    docs/encoders/bedrock.ipynb

  • Created a comprehensive Jupyter notebook demonstrating the usage of
    BedrockEncoder.
  • +1323/-0
    Configuration changes
    pyproject.toml
    Add Boto3 Dependency for Bedrock Support                                 

    pyproject.toml

    • Added boto3 as an optional dependency under the 'bedrock' extra.
    +2/-0     

    💡 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 labels May 14, 2024
    Copy link

    PR Description updated to latest commit (73cf6b8)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR introduces a new encoder class with a significant amount of new functionality, including AWS integration and error handling. The complexity of the AWS client initialization and the embedding generation logic requires careful review to ensure security and functionality.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The chunk_strings function in bedrock.py uses a hardcoded MAX_WORDS value which might not be suitable for all types of documents. This could lead to incomplete or incorrect embeddings for longer texts.

    Error Handling: The error handling in the __call__ method of BedrockEncoder might not catch all potential exceptions from the AWS SDK, leading to unhandled exceptions in some cases.

    🔒 Security concerns

    Sensitive information exposure: The code handles AWS credentials and session tokens, which are sensitive. While the PR retrieves these from environment variables or parameters, ensuring these are not logged or exposed through exceptions is crucial.

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

    Consider parameterizing MAX_WORDS in the chunk_strings function or adjusting it based on the document length to handle longer texts more effectively. This change would make the function more flexible and capable of dealing with a variety of document sizes without risking data loss or inefficiency. [important]

    relevant lineMAX_WORDS=20

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    Implement more comprehensive exception handling around the AWS SDK calls in the __call__ method to catch specific exceptions like network issues or API limits. This would improve the robustness of the encoder and provide clearer error messages for troubleshooting. [important]

    relevant lineresponse = self.client.invoke_model(

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    Add a validation step to check the input_type in the __init__ method of BedrockEncoder. This ensures that only supported input types are used, which can prevent runtime errors and make the code safer and more predictable. [medium]

    relevant lineself.input_type = input_type

    relevant filesemantic_router/encoders/bedrock.py
    suggestion      

    Consider using a more secure method to handle AWS credentials, such as AWS IAM roles for EC2 or other AWS services, which can avoid hardcoding or passing credentials directly and reduce the risk of credential leakage. [important]

    relevant lineaccess_key_id: Optional[str] = None

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for JSON parsing in the __call__ method

    The call method should handle potential JSON parsing errors when calling json.loads to
    prevent unhandled exceptions if the response is not in the expected format.

    semantic_router/encoders/bedrock.py [206-207]

    -response_body = json.loads(response.get("body").read())
    +try:
    +    response_body = json.loads(response.get("body").read())
    +except json.JSONDecodeError:
    +    raise ValueError("Failed to parse JSON response from Bedrock service.")
     
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential bug by adding error handling for JSON parsing, which is crucial for preventing runtime errors due to unexpected response formats. This improvement significantly enhances the robustness of the method.

    8
    Best practice
    Use more specific exception handling for boto3 errors

    The exception handling in the _initialize_client method could be more specific by catching
    boto3 related exceptions instead of a general Exception. This would make the error
    handling more precise and informative.

    semantic_router/encoders/bedrock.py [137-140]

    -except Exception as err:
    +except boto3.exceptions.BotoCoreError as err:
         raise ValueError(
    -        f"The Bedrock client failed to initialize. Error: {err}"
    +        f"The Bedrock client failed to initialize due to a BotoCoreError. Error: {err}"
         ) from err
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves error handling by specifying the type of exceptions related to boto3, which enhances the clarity and maintainability of the error handling process.

    7
    Enhancement
    Allow customization of the MAX_WORDS parameter in the chunk_strings method

    The method chunk_strings uses a hardcoded MAX_WORDS value which limits flexibility.
    Consider making MAX_WORDS a parameter of the BedrockEncoder class or method to allow
    customization based on different use cases.

    semantic_router/encoders/bedrock.py [163]

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

    Why: Making MAX_WORDS customizable is a valid enhancement that increases the flexibility of the chunk_strings method. This change allows the method to be more adaptable to various text sizes and use cases.

    6
    Maintainability
    Reorder imports alphabetically to improve code readability

    It's recommended to maintain imports in alphabetical order for better readability and
    maintainability. The import of BedrockEncoder should be placed in the correct order.

    semantic_router/encoders/init.py [4]

     from semantic_router.encoders.bedrock import BedrockEncoder
    +from semantic_router.encoders.bm25 import BM25Encoder
    +from semantic_router.encoders.clip import CLIPEncoder
    +from semantic_router.encoders.cohere import CohereEncoder
     
    Suggestion importance[1-10]: 5

    Why: The suggestion correctly identifies a minor maintainability improvement by reordering imports alphabetically. However, the existing and improved code snippets are identical, indicating a mistake in the suggestion's presentation.

    5

    @theanupllm theanupllm changed the title feat/bedrock encoder feat: Adding BedrockEncoder May 14, 2024
    Copy link

    codecov bot commented May 14, 2024

    Codecov Report

    Attention: Patch coverage is 83.33333% with 15 lines in your changes are missing coverage. Please review.

    Project coverage is 80.98%. Comparing base (8cf0234) to head (73cf6b8).

    ❗ Current head 73cf6b8 differs from pull request most recent head a4b13a6. Consider uploading reports for the commit a4b13a6 to get more accurate results

    Files Patch % Lines
    semantic_router/encoders/bedrock.py 84.70% 13 Missing ⚠️
    semantic_router/encoders/__init__.py 33.33% 2 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #277      +/-   ##
    ==========================================
    + Coverage   80.90%   80.98%   +0.08%     
    ==========================================
      Files          44       45       +1     
      Lines        2582     2672      +90     
    ==========================================
    + Hits         2089     2164      +75     
    - Misses        493      508      +15     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    Copy link
    Member

    @jamescalam jamescalam left a comment

    Choose a reason for hiding this comment

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

    lgtm! Just rerunning tests

    @jamescalam jamescalam merged commit 423e1a3 into main May 14, 2024
    7 of 8 checks passed
    @jamescalam jamescalam deleted the feat/bedrock_encoder branch May 14, 2024 09:47
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    documentation Improvements or additions to documentation enhancement Enhancement to existing features Review effort [1-5]: 4
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants