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

fix: Allow passing boto3 config to all AWS Bedrock classes #1166

Merged

Conversation

AnesBenmerzoug
Copy link
Contributor

Related PR

Proposed Changes:

  • Updated class init signatures to allow passing additional and optional boto3_config dictionary and use it when initializing boto3 client.
  • Updated tests accordingly

How did you test it?

  • Tested locally by running hatch test

Notes 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

@AnesBenmerzoug AnesBenmerzoug requested a review from a team as a code owner November 6, 2024 10:09
@AnesBenmerzoug AnesBenmerzoug requested review from davidsbatista and removed request for a team November 6, 2024 10:09
@CLAassistant
Copy link

CLAassistant commented Nov 6, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added integration:amazon-bedrock type:documentation Improvements or additions to documentation labels Nov 6, 2024
@AnesBenmerzoug
Copy link
Contributor Author

@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?

@davidsbatista
Copy link
Contributor

@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?

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.

@davidsbatista
Copy link
Contributor

davidsbatista commented Nov 12, 2024

@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.

@davidsbatista
Copy link
Contributor

@AnesBenmerzoug remove the parameterization of all the tests, and instead do a test such that:

  • Create and serialize a new instance without boto3config, then load/deserialize that object, and assert that new deserialized object is exactly the same as the initially created instance

@AnesBenmerzoug
Copy link
Contributor Author

@davidsbatista Sorry for the delay. I am not quite sure I understand your suggestion. I believe the tests with the parametrization already test the serialization and deserialization of the different bedrock instances with and without the boto3 config. Could you please explain what you meant more clearly?

@davidsbatista
Copy link
Contributor

sorry, this was a misunderstanding from my side - I think we could remove that parameterization but it doesn't hurt to be there.

I've approved your PR - you just need to sign the CLA

Copy link
Contributor

@davidsbatista davidsbatista left a comment

Choose a reason for hiding this comment

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

LGTM

@AnesBenmerzoug
Copy link
Contributor Author

@davidsbatista Thanks! I have already signed the CLA.
For some reason after I merged main into my branch, a commit made by the @HaystackBot found its way into my branch and it's the one account that didn't sign the CLA.
Is there a way to fix this?

@davidsbatista davidsbatista merged commit 798fb98 into deepset-ai:main Dec 3, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants