Bedrock embeddings #392
Bedrock embeddings #392
Conversation
| - Added support for Amazon Bedrock embeddings via `BedrockEmbedding` class. | ||
| - Users can now leverage Bedrock-hosted Embedding models for vector generation. | ||
| - Added unit test and conducted Unit test | ||
|
|
There was a problem hiding this comment.
We usually add one entry for each added feature, so it is better to group the first two in one item. The last one can be skipped.
| Embedder implementation using Amazon Bedrock's Titan Text Embedding model. | ||
|
|
||
| This class integrates with AWS Bedrock via `boto3` and uses the Titan Embedding | ||
| model (`amazon.titan-embed-text-v2:0`) to generate 1536-dimensional vector |
There was a problem hiding this comment.
Can you please mention here that amazon.titan-embed-text-v2:0 is the default model used as other models could be accepted as well.
| body=json.dumps({"inputText": text}) | ||
| ) | ||
| body = json.loads(response['body'].read()) | ||
| time.sleep(0.05) # To prevent throttling |
There was a problem hiding this comment.
I'd rather remove this from here as we have a RateLimitHandler interface that could be handling these kind of issues (for now it is only applied on LLM calls for answer generation, but we have a plan to apply it to the embedders as well. The user may also want to customise the behaviour so fixing the behaviour here won't help.
There was a problem hiding this comment.
sure. I will remove the
time.sleep(0.05)
Thanks
| self, | ||
| model_id: str = 'amazon.titan-embed-text-v2:0', | ||
| region: str = 'us-east-1' | ||
| ): |
There was a problem hiding this comment.
it would be also good to allow other optional parameters in form of **kwargs (same as in the other embedder classes). same applies to the embed_query function below
| Initialize the BedrockEmbeddings instance. | ||
|
|
||
| Args: | ||
| model_id (str): Identifier for the Bedrock Titan embedding model. |
There was a problem hiding this comment.
maybe for the wrapper we can name it model instead of model_id (for consistency with the other embedding wrappers?)
|
|
||
| from abc import ABC, abstractmethod | ||
| from typing import List | ||
| import boto3 |
There was a problem hiding this comment.
this further involves 1) adding this package in the list of optional dependencies in the .toml file, 2) wrapping the import in a try-except block to avoid crashing the whole application if the module is not installed (please check how similar issue is handled in the other embedders e.g., here)
Description
Type of Change
Complexity
Complexity: Medium
How Has This Been Tested?
Checklist
The following requirements should have been met (depending on the changes in the branch):