-
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: AmazonBedrockChatGenerator - migrate Anthropic chat models to use messaging API #545
Conversation
@anakin87 not a priority yet, let's sync internally first |
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.
In general, I feel that our Bedrock Generators can be simplified somewhat...
It is not always easy to understand what is going on.
About this PR, I have an idea:
can we start adding integration tests, as we do for other integrations?
That way, we can be sure that our integration is stable and working...
@anakin87 in 8683af0 I've added model parameter tracking. I thought it lead to a better UX. If user uses unknown parameter rather than getting exception from AWS she'll get a warning. Instead of:
user will see:
This comes with cost of tracking these parameters. LMK if you think it is worth it. We can rollback the last commit if you disagree. |
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.
In general, I would like to simplify this integration a bit in the future.
I still see some legacy of the Prompt Node 😃
Anyway, your PR looks good and I appreciated the extra effort of adding integration tests.
I think they will help us in the future...
Fixes deepset-ai/haystack#7324