Fix : short_term_memory with bedrock - using user defined model(when passed as attribute) rather than default

[rishi154]

Modified _configure_bedrock method to use user submitted model_name rather than default amazon.titan-embed-text-v1.

Sending model_name in short_term_memory (embedder_config/config) was not working.

Passing model_name to use model_name provide by user than using default. Added if/else for backward compatibility

[joaomdmoura]

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #1959

Overview

The proposed changes to the _configure_bedrock method in embedding_configurator.py introduce support for custom model names in Amazon Bedrock embeddings to enhance flexibility and user control over the embedding configurations.

Code Changes

This modification alters how the AmazonBedrockEmbeddingFunction is instantiated based on whether a model_name is provided. Below are the specific improvements and observations.

Key Findings and Recommendations

1. Code Style and Idiomatic Practices:

   • Comparison Operator:
     • Current Code:

       if model_name==None:

     • Suggested Improvement:

       if model_name is None:

     • Rationale: Using is for None comparisons is a best practice in Python.

2. Comment Quality:

   • Original Comment:

     # Passing model_name to use model_name provide by user than using default. Added if/else for backward compatibility

   • Revised Comment:

     # Allow custom model_name override with backwards compatibility

   • Rationale: The suggestion improves clarity and correctness, making it easier for others to understand the code's intent.

3. Code Structure:

   • Refactoring Redundant Code:
     • Current Implementation:

       if model_name==None:
           return AmazonBedrockEmbeddingFunction(
               session=config.get("session"),
           )
       else:
           return AmazonBedrockEmbeddingFunction(
               session=config.get("session"),
               model_name=model_name
           )

     • Recommended Refactor:

       kwargs = {"session": config.get("session")}
       if model_name is not None:
           kwargs["model_name"] = model_name
       return AmazonBedrockEmbeddingFunction(**kwargs)

   • Rationale: This changes simplify the code by reducing duplication and adhering to the DRY (Don't Repeat Yourself) principle.

4. Documentation Enhancements:

   • Docstring Update:
     • Suggested docstring adjustment to fully reflect the parameter behavior:

     def _configure_bedrock(config, model_name):
         """
         Configure Amazon Bedrock embedding function.
         
         Args:
             config (dict): Configuration dictionary containing session information.
             model_name (str, optional): Custom model name for embeddings, defaults to 'amazon.titan-embed-text-v1' if not specified.
         
         Returns:
             AmazonBedrockEmbeddingFunction: Configured embedding function instance
         """

   • Rationale: This update clarifies how the new parameter operates and enhances the understanding for future developers.

5. Type Hinting:

   • Adding type hints can provide better clarity and IDE support:

     def _configure_bedrock(config: dict, model_name: Optional[str] = None) -> AmazonBedrockEmbeddingFunction:

   • Rationale: Type hints facilitate easier code navigation and understanding, which benefits both current and future maintainers.

Additional Considerations

• Input Validation: It might be beneficial to validate model_name to ensure it corresponds to a valid Bedrock model identifier.
• Unit Testing: Tests should be added to cover the new functionality introduced by allowing model_name overrides.
• Deprecation Warnings: If default model usage is deprecated in the future, consider adding warnings to alert users to specify models explicitly.

Summary

While the functional enhancements are beneficial for supporting customized embedding configurations, incorporating the outlined suggestions would significantly improve code quality, maintainability, and clarity. Addressing the identified issues will position the code well for future development and user engagement.

[bhancockio]

Thank you for the fix!