fix: Allow passing boto3 config to all AWS Bedrock classes #1166
Conversation
AnesBenmerzoug
requested review from
davidsbatista
and removed request for
a team
|
|
github-actions
bot
added
integration:amazon-bedrock
type:documentation
|
@davidsbatista I could not completely sign the CLA because I accidentaly some of the commits without configuring my email address. Should I leave it like this or is it okay to update the author of those commits and force-push the change? |
| @@ -71,12 +72,16 @@ def test_from_dict(mock_boto3_session): | |||
| "generation_kwargs": {"temperature": 0.7}, | |||
| "streaming_callback": "haystack.components.generators.utils.print_streaming_chunk", | |||
| "truncate": True, | |||
| "boto3_config": { | |||
There was a problem hiding this comment.
did you test that the from_dict can handle a serialized object without the newly added boto3_config? we need to be sure this change is still compatible with older versions
There was a problem hiding this comment.
Done. I parametrized the tests for the boto3_cofig value.
| @@ -117,6 +121,7 @@ def test_from_dict(self, mock_boto3_session): | |||
| assert embedder.progress_bar | |||
| assert embedder.meta_fields_to_embed == [] | |||
| assert embedder.embedding_separator == "\n" | |||
| assert embedder.boto3_config == {"read_timeout": 1000} | |||
There was a problem hiding this comment.
same thing here as above
There was a problem hiding this comment.
Done. I parametrized the tests for the boto3_cofig value.
| @@ -76,13 +77,17 @@ def test_from_dict(self, mock_boto3_session): | |||
| "aws_profile_name": {"type": "env_var", "env_vars": ["AWS_PROFILE"], "strict": False}, | |||
| "model": "cohere.embed-english-v3", | |||
| "input_type": "search_query", | |||
| "boto3_config": { | |||
There was a problem hiding this comment.
same thing as before
There was a problem hiding this comment.
Done. I parametrized the tests for the boto3_cofig value.
You need to sign the CLA in order for the PR to be merged, so it seems you will need to update the author of these commits. |
|
@AnesBenmerzoug thanks for this contribution! I've left some comments regarding backward compatibility. I've also asked my colleague @vblagoje to do a second review. |
…eepset-ai#1165) * fix chroma breaking changes * improve warning * better warning
AnesBenmerzoug
force-pushed
the
fix/bedrock_boto3_config
branch
from
b59e51a
to
f08beea
Compare
|
@AnesBenmerzoug remove the parameterization of all the tests, and instead do a test such that:
|
Related PR
Proposed Changes:
boto3_configdictionary and use it when initializing boto3 client.How did you test it?
hatch testNotes for the reviewer
This is a follow up to PR !1135 that simply applies the same change to all other aws bedrock classes (
AmazonBedrockDocumentEmbedder,AmazonBedrockTextEmbedder,AmazonBedrockChatGenerator).Checklist
I have updated the related issue with new insights and changesfix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.