-
Notifications
You must be signed in to change notification settings - Fork 323
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: AWS Bedrock provider #318
base: main
Are you sure you want to change the base?
Conversation
Hey, thanks for this PR that tackles #317 🎉, we'll get a review going on this soon but in the meantime, do compare to the other model providers to ensure you've hit all the right traits and patterns! Also, do make sure you pass clippy and fmt for our CI to pass as well! |
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.
Hey, great work on this PR. Ik working with some of the AWS libraries and tech requires some boilerplate with their libraries so it's pretty cool to have some integrations with. Pointed out some things we do with our other providers that should be matched here.
Feel free to join our discord if you'd like to chat with us more conversationally about these things otherwise you can leave questions or other things here!
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.
Don't forget tests! These are usually just mocking the response and testing the serializsation/deserialization aptly the Message <-> Message conversions!
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.
Thanks for the review!
I changed things as mentioned in PR comments (had to do newtype wrapping because I ran into orphan rule problems)
Will add more tests soon.
9b03dd7
to
937787e
Compare
)), | ||
}?; | ||
|
||
if let Some(tool_use) = choice.iter().find_map(|content| match content { |
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.
@0xMochan can you please explain why I have to "pick" tool call here in order to get model to execute tool ?
I am aware things will change around tool calls soon in order to support tool results get back to model so I didn't want to poke too much things around here...
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.
So you don't have to pick a tool call here, you can process all of the tool calls and convert them to generic forms of tool calls -- the main model can handle it. Something like:
let mut content = content
.iter()
.map(|c| match c {
AssistantContent::Text { text } => completion::AssistantContent::text(text),
AssistantContent::Refusal { refusal } => {
completion::AssistantContent::text(refusal)
}
})
.collect::<Vec<_>>();
content.extend(
tool_calls
.iter()
.map(|call| {
completion::AssistantContent::tool_call(
&call.function.name,
&call.function.name,
call.function.arguments.clone(),
)
})
.collect::<Vec<_>>(),
);
Ok(content)
Currently, rig does not actually call multiple tools, but I have a branch that I'm currently working on to enable that behavior. Returning multiple tools in the general completion response will pass that to the agent completion
code which will then dispatch tool-calling.
Hey! Really nice feature here! Really needed on my end so it's nice to see a PR already open for it :) |
I think there might be a better way of implementing this btw. The Rust types of the bedrock library are very strict and limiting for a lot of features (e.g. prompt caching) when using the Converse API, but if this crate can instead just act as a provider over an existing agent, the wrapper object can just use the |
I've made the changes on this PR which also has some MCP tools integration: #328. It seems cleaner that way, what do you guys think? |
Hi, thank you for helping with this PR! I would leave this decision for core maintainers to decide. @0xMochan |
This PR include some breaking changes to core package, I think that deserves it's own issue and PR |
What do you mean by this? With the wrapper I made you can use one of the already existing |
bdebb44
to
2e17582
Compare
Added new model provider
2e17582
to
6ba23e3
Compare
Added new model provider as a separate crate because it depends on AWS SDK.
Relates to issue
Important:
Access to Amazon Bedrock foundation models isn't granted by default. You can request access, or modify access, to foundation models only by using the Amazon Bedrock console.
This is what you would get if you ran all example programs:
agent_with_bedrock.txt
document_with_bedrock.txt
embedding_with_bedrock.txt
extractor_with_bedrock.txt
image_with_bedrock.txt
rag_with_bedrock.txt