-
Notifications
You must be signed in to change notification settings - Fork 126
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 Amazon Bedrock chat model support #333
Conversation
… new haystack-ai is released
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some small remarks, nothing impeding - I will still play around with both models and have another look at the tests
"""Extracts the responses from the Amazon Bedrock response.""" | ||
return self._extract_messages_from_response(response_body) | ||
|
||
def get_stream_responses(self, stream, stream_handler: Callable[[StreamingChunk], None]) -> List[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can add a type to stream
?
responses = ["".join(tokens).lstrip()] | ||
return responses | ||
|
||
def _update_params(self, target_dict: Dict[str, Any], updates_dict: Dict[str, Any]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the self
is never used here, seems like this can be a static method @statichmethod
...azon_bedrock/src/haystack_integrations/components/generators/amazon_bedrock/chat/adapters.py
Show resolved
Hide resolved
...azon_bedrock/src/haystack_integrations/components/generators/amazon_bedrock/chat/adapters.py
Outdated
Show resolved
Hide resolved
...edrock/src/haystack_integrations/components/generators/amazon_bedrock/chat/chat_generator.py
Show resolved
Hide resolved
Note that the AWS credentials are not required if the AWS environment is configured correctly. These are loaded | ||
automatically from the environment or the AWS configuration file and do not need to be provided explicitly via | ||
the constructor. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: from my experience, I suspect three minimum mandatory parameters need to be configured: aws_access_key_id, aws_session_token, aws_region_name
, maybe a sentence in the docstring mention that
) | ||
raise AmazonBedrockConfigurationError(msg) from exception | ||
|
||
model_adapter_cls = self.get_model_adapter(model=model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe first check if the given string is a valid AWS model before setting up a connection with AWS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what get_model_adapter
does. If not found we'll raise AmazonBedrockConfigurationError
...ns/amazon_bedrock/src/haystack_integrations/components/generators/amazon_bedrock/handlers.py
Show resolved
Hide resolved
I've noticed that the code format for some files has changed. I don't know if this is intended or if this is a misconfiguration on either the previous commit or this new commit - I am just raising that I've noticed it, but I don't know which formatting style is correct. |
after talking with @vblagoje, we will revert the code formatting in a separate PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Why:
Introduces significant upgrades to the Amazon Bedrock integration by adding functionality for chat model generation. The motivation behind these changes is to support conversational AI and chatbot applications that leverage Amazon Bedrock's machine learning capabilities. This enhancement expands the project's features, allowing users to interact with AI models in a chat-like manner and enable streaming of responses for real-time interaction.
What:
test_amazon_chat_bedrock.py
to ensure the chat functionality works as expected.__all__
directive in__init__.py
: Expanded the list of imports to include AmazonBedrockChatGenerator.How can it be used:
Chat Generation:
Streaming Responses:
How did you test it:
The testing process involves both unit and integration tests. The unit tests, found in
test_amazon_chat_bedrock.py
, check the functionality of different components such as model adapters and chat generators. These tests ensure that the classes behave as expected when provided with mock data and simulated AWS sessions. Integration tests would involve the actual AWS environment and interaction with the Bedrock API, ensuring end-to-end functionality.Notes for the reviewer: