spring-ai icon indicating copy to clipboard operation
spring-ai copied to clipboard

Use Amazon Bedrock Converse API to re-implementing Bedrock AI Models.

Open maxjiang153 opened this issue 1 year ago • 13 comments

see: https://github.com/spring-projects/spring-ai/issues/809

Provide a new BedrockConverseApi for AI model chat usage. Re-implementing current chat models from AbstractBedrockApi to the Bedrock Converse API to reduce complexity. Re-implementing Bedrock chat model usage information: https://github.com/spring-projects/spring-ai/pull/605. Support build-in exponential backoff: https://github.com/spring-projects/spring-ai/pull/759.

things todo:

  • [x] BedrockConverseApi
  • [x] re-implement the Bedrock chat model.
  • [x] chat model exponential backoff.
  • [x] chat model options document.
  • [x] re-implement the Bedrock chat model usage information. https://github.com/spring-projects/spring-ai/pull/605
  • [x] Tools support. https://github.com/spring-projects/spring-ai/issues/802 https://github.com/spring-projects/spring-ai/pull/810
  • [x] remove deprecated bedrock chat api.
  • [x] Support custom BedrockRuntimeClient and BedrockRuntimeAsyncClient.
  • [x] embedding model exponential backoff. https://github.com/spring-projects/spring-ai/pull/759
  • [x] Amazon Bedrock Mistral Chat model support. https://github.com/spring-projects/spring-ai/pull/808
  • [x] Amazon Bedrock Cohere Command R model support. https://github.com/spring-projects/spring-ai/pull/667
  • [x] Amazon Bedrock Anthropic Claude 3.5 Sonnet model support. https://github.com/spring-projects/spring-ai/issues/914

maxjiang153 avatar Jun 02 '24 23:06 maxjiang153

@tzolov Hi Christian, I'm basically finished re-implementing Bedrock AI models with the Amazon Bedrock Converse API. But there are some thing's I'd like to discuss.

  1. Because of the Amazon Bedrock Converse API, bedrock chat models don't need such things as Anthropic3ChatBedrockApi anymore. Should I remove the old Apis?
  2. Bedrock Mistral and Cohere Command R model which are not supported yet. Should I includes these models in this PR?

maxjiang153 avatar Jun 04 '24 14:06 maxjiang153

Is it realistic to expect this PR to be merged before 1.0.0? That would be great to see.

Hakenadu avatar Jun 13 '24 13:06 Hakenadu

Is it realistic to expect this PR to be merged before 1.0.0? That would be great to see.

I hope that, I'm looking forward to have this PR merged as soon as possible too...

DEG-7 avatar Jun 14 '24 06:06 DEG-7

I hope so, This is a huge PR, Maybe we can review it by ourselves first to reduce the workload for the team members

maxjiang153 avatar Jun 14 '24 11:06 maxjiang153

Hello @tzolov Any advice on this PR?

maxjiang153 avatar Jun 18 '24 14:06 maxjiang153

Hi @maxjiang153, it's a pity that there is no news about this PR, maybe it's too big, I don't know.... @tzolov @markpollack any thoughts on how to move forward with this?

Thanks all for the huge effort !!

DEG-7 avatar Jun 27 '24 06:06 DEG-7

Hi @maxjiang153 ,

Thank you very much for the great (and big ;) contribution.

Sorry for not reaching out earlier. We've been quite busy and lacking the manpower to review and address all PRs.

The #813 has a hight priority for me and I hope to be able to jump on it soon.

tzolov avatar Jun 27 '24 13:06 tzolov

@tzolov As Amazon Bedrock is rapidly involved and the AI 21 Jamba model is just generally available, I wonder: should I keep moving on this PR to support this new model or wait for this PR to be merged and then create a new PR to support the new model?

Check this out: https://aws.amazon.com/about-aws/whats-new/2024/06/ai21-labs-jamba-instruct-model-amazon-bedrock/

maxjiang153 avatar Jun 30 '24 04:06 maxjiang153

@maxjiang153 it would be great if you can update the PR with the latest changes in the project, so it can be easily reviewed by @tzolov. Thanks to both very much !

DEG-7 avatar Jul 18 '24 12:07 DEG-7

waiting ....

lameroot avatar Jul 22 '24 12:07 lameroot

@DEG-7 I see that maintaining this PR with upstream is heavy work, I'm not sure when team members will work on this PR, but I'll keep an eye out for when it will be reviewed or merged.

maxjiang153 avatar Jul 23 '24 00:07 maxjiang153

Excuse me, are there any dates when this PR will be merged?

lameroot avatar Aug 08 '24 09:08 lameroot

I understand the difficulties keeping it up to date with main an appreciate the efforts. We have fallen behind in PR, in particular those around bedrock because we haven't had the time to wrap our head yet our the big changes going on. I apologize. The Alibaba models are important to the project and I do expect it to be part of the 1.0 release. I would like to make sorting out the bedrock story a priority for us after this M2 release.

markpollack avatar Aug 20 '24 19:08 markpollack

Hey @maxjiang153,

First, apologies for the delay. You've put in a tremendous amount of work and we've been too slow to react.

To a large extent, this is due to the size of this PR. Usually, PRs of this scope would never be merged in a single go.

I've started exploring the Bedrock Converse API and also trying to check the code in your PR.

My initial goal is to:

  1. Start a new dedicated module spring-ai-bedrock-converse and leave the old (e.g. invoke-model) in its existing spring-ai-bedrock module (perhaps we can rename it later to spring-ai-bedrock-invoke or similar)
  2. Implement the core Converse API and Abstract classes in a single PR
  3. Implement the Anthropic support in a second PR (+ related documentation)

Please let me know if you're still interested in contributing to this work. Either way, if we use your code, we will always add your name as a contributor (same for the commit).

tzolov avatar Oct 24 '24 11:10 tzolov

Hi @tzolov, it's been a while. I still would like to contribute to this PR, and AWS has released lots of new models since this PR was created.

However, with the Bedrock Converse API, all of the models have a very similar invoke method. I think implementing a new spring-ai-bedrock-converse module might be a good idea. The most important part, I think, is whether we still need to keep different models with different implementations like the old spring-ai-bedrock module, or if we should implement a single converse API that is compatible with all models supported by the Bedrock Converse API.

Let me know if you have any thoughts.

maxjiang153 avatar Oct 25 '24 02:10 maxjiang153

@maxjiang153

I still would like to contribute to this PR,

This is great thank you.

AWS has released lots of new models since this PR was created.

Spring AI API has changed significantly as well. The function calling is different, chat-model, metadata ... . Now we have observation support. .. I will try to put together some foundation in separate a branch and then we can start from there. Will try to reuse the code from your PR when possible.

The most important part, I think, is whether we still need to keep different models with different implementations like the old spring-ai-bedrock module, or if we should implement a single converse API that is compatible with all models supported by the Bedrock Converse API.

I'm still exploring the Converse API. Looks like we will be able to reduce the implementation to a single BedrockConverseChatModel implementation indeed. But we will need to support the specific per-model provider options: additionalModelRequestFields , additionalModelResponseFieldPaths. Maybe having dedicated per-model options class will be enough. Don't know yet.

tzolov avatar Oct 25 '24 18:10 tzolov

Based on the Converse API and Converse Stream API definitions, all the models have the same request structure. The only differences are the additionalModelRequestFields and additionalModelResponseFieldPaths.

In my original design, I kept ChatOptions for each single model like this one: Anthropic3ChatOptions, and created a BedrockConverseApiUtils class to convert the ChatOptions fields to additionalModelRequestFields.

So I'm thinking we could keep this conversion mechanism. It might work like this: We could have a BedrockConverseChatModel to unify all models that Amazon Bedrock supports, and we could have different ChatOptions for each single model or maybe create an AbstractBedrockConverseChatOptions with different model implementations.

Also, I wonder if I should continue working on this PR or if I should create a new PR instead.

maxjiang153 avatar Oct 26 '24 02:10 maxjiang153

tbh BedrockConverseChatModel seems weird, because the Converse API is not an actual model; it could represent a lot of models.

maxjiang153 avatar Oct 26 '24 02:10 maxjiang153

@maxjiang153 - I've created PR #1650 with you as co-author. While I rewrote the integration to match latest Spring AI API changes, I incorporated elements from your PR. Please review as I may have missed something.

Type-safe options will be addressed in separate PRs.

tzolov avatar Nov 02 '24 14:11 tzolov

Nice work, I'll close this PR

maxjiang153 avatar Nov 02 '24 15:11 maxjiang153

@maxjiang153 FYI, here are the follow up tasks, I could think of: https://github.com/spring-projects/spring-ai/issues/1758 and some early ideas: https://github.com/spring-projects/spring-ai/pull/1760

tzolov avatar Nov 19 '24 10:11 tzolov