feat (provider/amazon-bedrock): remove dependence on AWS SDK Bedrock library #4582
Conversation
shaper
changed the title
shaper
force-pushed
the
shaper/pr/bedrock-2a
branch
from
d77b110 to
d7c55ea
Compare
shaper
force-pushed
the
shaper/pr/bedrock-2a
branch
from
d7c55ea to
6b38a54
Compare
shaper
force-pushed
the
shaper/pr/bedrock-2a
branch
from
6b38a54 to
ad9719a
Compare
shaper
changed the title
| model: bedrock('anthropic.claude-3-haiku-20240307-v1:0'), | ||
| prompt: 'Invent a new holiday and describe its traditions.', | ||
| model: bedrock( | ||
| 'arn:aws:bedrock:us-east-2:474668406012:inference-profile/us.anthropic.claude-3-5-sonnet-20240620-v1:0', |
There was a problem hiding this comment.
we need to ideally figure out how to make it backwards compatible, or we need to announce breaking changes somehow
There was a problem hiding this comment.
Proposal:
- ship as a major version update
- (breaking change) we removed BedrockOptions from the provider settings (updated documentation to note)
- note unless we rename the environment variables to prefix with
AI_SDK_or similar, we continue to have the issue where folks could unknowingly inherit serverless runtime environment variables. Should we consider adding this prefix?
- note unless we rename the environment variables to prefix with
- I am still reviewing the model-id/ARN discrepancy issue, and we will either make that transparent (preserve existing behavior) or update docs to reflect a change
- we could improve backwards-compatibility a bit if we keep the old BedrockOptions for the credentials fields. Some folks were using that. Is it worth the extra code/complexity?
shaper
force-pushed
the
shaper/pr/bedrock-2a
branch
from
267d731 to
0b46c78
Compare
| if (topK != null) { | ||
| warnings.push({ | ||
| type: 'unsupported-setting', | ||
| setting: 'topK', | ||
| }); | ||
| } | ||
|
|
||
| // TODO: validate this is still the case. |
There was a problem hiding this comment.
@lgrammel do you remember the backstory on this warning below?
There was a problem hiding this comment.
probably didn't work back then - let's test if it works now
| @@ -5,7 +5,9 @@ import { z } from 'zod'; | |||
|
|
|||
| async function main() { | |||
| const result = await generateObject({ | |||
| model: bedrock('anthropic.claude-3-5-sonnet-20240620-v1:0'), | |||
| model: bedrock( | |||
| `arn:aws:bedrock:${process.env.AWS_REGION}:${process.env.AWS_ACCOUNT_ID}:inference-profile/us.anthropic.claude-3-5-sonnet-20240620-v1:0`, | |||
There was a problem hiding this comment.
Can we use the simple ids here?
| @@ -212,49 +220,50 @@ export class BedrockChatLanguageModel implements LanguageModelV1 { | |||
| toolName: part.toolUse?.name ?? `tool-${this.config.generateId()}`, | |||
| args: JSON.stringify(part.toolUse?.input ?? ''), | |||
| })), | |||
| finishReason: mapBedrockFinishReason(response.stopReason), | |||
| finishReason: mapBedrockFinishReason( | |||
| response.stopReason as BedrockStopReason, | |||
There was a problem hiding this comment.
why is the as needed here? shouldn't zod validation narrow the types correctly?
| LanguageModelV1StreamPart | ||
| >({ | ||
| stream: response.pipeThrough( | ||
| new TransformStream<ParseResult<any>, LanguageModelV1StreamPart>({ |
There was a problem hiding this comment.
any should be replaced with the actual type from parsing. What is the type of the chunks? we are parsing with Zod?
|
|
||
| // Process the message. | ||
| if (decoded.headers[':message-type']?.value === 'event') { | ||
| const data = new TextDecoder().decode(decoded.body); |
There was a problem hiding this comment.
extract new TextDecoder(), this is most likely a hot loop and we could init it before the loop (i.e. outside of the transform stream)
|
|
||
| // Wrap the data in the `:event-type` field to match the expected schema. | ||
| const parsedDataResult = safeParseJSON({ text: data }); | ||
| let wrappedData; |
| }; | ||
|
|
||
| // Re-parse with the expected schema. | ||
| const parsedWrappedData = safeParseJSON({ |
There was a problem hiding this comment.
no need to JSON.stringify and parse again, use safeValidate instead
| rawValue: parsedWrappedData.rawValue, | ||
| }); | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
shouldn't the error be forward in some fashion?
| if (!parsedWrappedData.success) { | ||
| controller.enqueue(parsedWrappedData); | ||
| break; | ||
| } | ||
|
|
||
| controller.enqueue({ | ||
| success: true, | ||
| value: parsedWrappedData.value, | ||
| rawValue: parsedWrappedData.rawValue, | ||
| }); |
There was a problem hiding this comment.
aren't both cases the same, i.e. controller.enqueue(parsedWrappedData); is enough?
| }, | ||
| ); | ||
| })}.amazonaws.com`, | ||
| ) ?? ''; |
There was a problem hiding this comment.
the ?? '' could hide errors
| accessKeyId: loadSetting({ | ||
| settingValue: options.accessKeyId, | ||
| settingName: 'accessKeyId', | ||
| environmentVariableName: 'AWS_ACCESS_KEY_ID', | ||
| description: 'AWS access key ID', | ||
| }), | ||
| secretAccessKey: loadSetting({ | ||
| settingValue: options.secretAccessKey, | ||
| settingName: 'secretAccessKey', | ||
| environmentVariableName: 'AWS_SECRET_ACCESS_KEY', | ||
| description: 'AWS secret access key', | ||
| }), | ||
| sessionToken: loadOptionalSetting({ | ||
| settingValue: options.sessionToken, | ||
| environmentVariableName: 'AWS_SESSION_TOKEN', | ||
| }), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I still feel we are delaying errors by pushing this down but could be wrong.
| // TODO: explore avoiding the below stringify since we do it again at | ||
| // post-time and the content could be large with attachments. | ||
| body: JSON.stringify(body), |
There was a problem hiding this comment.
might be worth resolving before landing considering attachments
would the fetch wrapper approach solve this?
| bytes: Buffer.from( | ||
| part.image ?? (part.image as Uint8Array), | ||
| ).toString('base64'), |
There was a problem hiding this comment.
do we have a helper for this in provider utils?
responseHeaders