-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Amazon Bedrock Provider #2016
Amazon Bedrock Provider #2016
Conversation
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 working on this! I left a few comments.
packages/amazon-bedrock/CHANGELOG.md
Outdated
@@ -0,0 +1 @@ | |||
# @ai-sdk/amazon-bedrock |
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.
file can be removed, will be automatically created by changesets
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.
removed
packages/amazon-bedrock/package.json
Outdated
@@ -0,0 +1,72 @@ | |||
{ | |||
"name": "@ai-sdk/amazon-bedrock", | |||
"version": "0.0.29", |
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.
set to 0.0.0.
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.
done
"dependencies": { | ||
"@ai-sdk/provider": "0.0.10", | ||
"@ai-sdk/provider-utils": "0.0.14", | ||
"@aws-sdk/client-bedrock-runtime": "^3.598.0" |
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.
preference is usually to call the api directly using fetch, unless it's not easily possible because of e.g. auth. does the bedrock runtime have any limitations (e.g. does it work on edge)?
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.
There is a rest API for these calls so I could use fetch but I would have to convert the auth and I also would have to define all the types the library provides explicitly.
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.
auth can make this fairly complicated and is the reason why i used a similar approach for the vertex provider. fine to leave as is.
btw, would you mind documenting how auth works? (e.g. in a separate file, or in something similar to https://github.com/vercel/ai/blob/main/content/providers/01-ai-sdk-providers/11-google-vertex.mdx
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.
Added documentation with auth and model capabilities etc.
?.map(part => ({ | ||
toolCallType: 'function', | ||
toolCallId: part.toolUse?.toolUseId ?? generateId(), | ||
toolName: part.toolUse?.name ?? 'unknown', |
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.
when can this happen? can setting it to unknown
lead to issues such as tools sharing the same name?
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 theory it shouldn't happen but the type of toolUseId
and name
are both string | undefined
. Would it be better to leave it as an empty string?
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.
strange, wonder when this could happen. how about tool-${generateId()}
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.
Updated
/** | ||
Additional model parameters field paths to return in the response. | ||
Converse returns the requested fields as a JSON Pointer object in | ||
the additionalModelResponseFields field. | ||
|
||
The following is example JSON for additionalModelResponseFieldPaths. | ||
|
||
[ "/stop_sequence" ] | ||
*/ | ||
additionalModelResponseFields?: string[]; |
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.
are those captured in any way? or could this be removed?
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.
Removed this and the system prompt settings properties
// TODO: support image URL | ||
bytes: part.image as Uint8Array, |
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.
e.g. the anthropic provider supports this via image download
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.
Added image download
throw new Error( | ||
`Unsupported message role '${role}'. Please use the system options on the provider instead.`, | ||
); |
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.
support this instead of having a separate field. check out the anthropic provider for an example. this is a major incompatibility
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.
Copied the way the anthropic provider did it
throw new Error( | ||
`Unsupported message role '${role}'. Please use the assistant role with toolUse content block instead.`, | ||
); |
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.
must have for tool roundtrips.
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.
Added
export * from '../bedrock-chat-language-model'; | ||
export * from '../bedrock-chat-settings'; |
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.
file / package not needed
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.
Sorry I'm not following, are you saying to remove this file completely, or just remove the bedrock-chat-settings export line?
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.
remove the internal export completely. I only have it for openai bc it's needed by azure openai. otherwise I try to keep the api surface as small as possible
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.
removed
packages/amazon-bedrock/package.json
Outdated
"./internal": { | ||
"types": "./internal/dist/index.d.ts", | ||
"import": "./internal/dist/index.mjs", | ||
"module": "./internal/dist/index.mjs", | ||
"require": "./internal/dist/index.js" | ||
} |
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.
remove
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.
Remove the whole ./internal
section?
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.
yes
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.
removed
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
"@aws-sdk/client-bedrock-runtime": "^3.598.0" | ||
}, | ||
"devDependencies": { | ||
"@smithy/types": "^3.1.0", |
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.
what is this for?
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.
I used this to force the aws-sdk-client-mock to use this specific version because it gives a bunch of type errors if I don't
@jon-spaeth this is looking pretty good. small comments aside, some docs on how to do authentication and examples under |
@lgrammel I think I got everything you requested. Let me know if there is anything else! Thanks for the quick turnaround on this. It will be majorly helpful in my day to day. |
Thanks this looks great! I'll take it from here. Superseeded by #2045 |
Adding an Amazon Bedrock provider package (@ai-sdk/amazon-bedrock) with the new ConverseApi