haystack-core-integrations
haystack-core-integrations copied to clipboard
deepset-ai
feat: bedrock converse
Related Issues
Proposed Changes:
Added a new converse generator, some other utility classes have been added to facilitate its use such as ConverseMessage or ToolConfig
How did you test it?
unit test and manual testing mostly
Checklist
- I have read the contributors guidelines and the code of conduct
- I have updated the related issue with new insights and changes
- I added unit tests and updated the docstrings
- I've used one of the conventional commit types for my PR title:
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.
@silvanocerza It seems like the AWS region is not assigned in the CI. I had the same issues in my previous PR.
@FloRul PR from forks can't have access to repository secrets. Those tests should be skipped if the necessary secrets are not found, see this as an example.
Why do we need a new converse generator? Isn't it akin to a chat generator that we already have? Shouldn't we update that?
It seems like all the abstractions added are extremely similar to existing ones for chat generators. 🤔
Why do we need a new converse generator? Isn't it akin to a chat generator that we already have? Shouldn't we update that?
It seems like all the abstractions added are extremely similar to existing ones for chat generators. 🤔
That is a fair point, the thing with the current bedrock chat is that it relies on adpaters, would it be more logic to consider converse its own adapter ? Keeping in mind that aws seems to aim at converse being the main point of entry for bedrock inference api.
That is a fair point, the thing with the current bedrock chat is that it relies on adpaters, would it be more logic to consider converse its own adapter ? Keeping in mind that aws seems to aim at converse being the main point of entry for bedrock inference api.
I would consider converse its own adapter. If AWS goes all in with this kind of common API we can ditch completely the adapter concepts too. That's less code to maintain. :)
Maybe the best approach would be to ditch the adapters for those models that support the new Converse API, that should work right?
The idea behind adding a new generator was because of the standardization, which results in a richer output from the generator as well as facilitating the multimodal integration. I think trying to put the implementation in the former ChatGenerator would either not be retrocompatible or making it lose all the benefits this implementation tries to provide: easier tool calling, multimodal integration and richer output.
We already have a standard and that is the ChatMessage. Adding all the classes from utils.py goes against that.
This Component will be extremely hard to use in any existing Pipelines in place of other generators also. The ConverseMessage is a completely new type that no other Component returns as output nor expects as input. It would be completely isolated.
That's why I'm advocating to use the existing interfaces and types.
