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

feat: bedrock converse #1061

Closed
wants to merge 40 commits into from
Closed

Conversation

FloRul
Copy link
Contributor

@FloRul FloRul commented Sep 7, 2024

Related Issues

#977

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

@FloRul FloRul requested a review from a team as a code owner September 7, 2024 03:16
@FloRul FloRul requested review from silvanocerza and removed request for a team September 7, 2024 03:16
@FloRul FloRul changed the title Feat bedrock converse feat: bedrock converse Sep 7, 2024
@github-actions github-actions bot added the type:documentation Improvements or additions to documentation label Sep 8, 2024
@lambda-science
Copy link
Contributor

Following this, it looks great ! Thank you

@FloRul
Copy link
Contributor Author

FloRul commented Sep 9, 2024

@silvanocerza It seems like the AWS region is not assigned in the CI. I had the same issues in my previous PR.

@silvanocerza
Copy link
Contributor

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

@silvanocerza
Copy link
Contributor

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

@vblagoje vblagoje self-requested a review September 12, 2024 12:51
@FloRul
Copy link
Contributor Author

FloRul commented Sep 13, 2024

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.

@silvanocerza
Copy link
Contributor

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?

@FloRul
Copy link
Contributor Author

FloRul commented Sep 17, 2024

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.

@silvanocerza
Copy link
Contributor

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.

@FloRul
Copy link
Contributor Author

FloRul commented Oct 3, 2024

Ok @silvanocerza I'll look into that.

@anakin87
Copy link
Member

Superseded by #1219

@anakin87 anakin87 closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:amazon-bedrock type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants