-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(aws): support reasoning blocks for claude 3.7 #7768
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
reminder that I'll need to bump the version of core that AWS depends on when #7769 gets released. |
90819b7
to
09460f0
Compare
libs/langchain-aws/src/types.ts
Outdated
@@ -19,3 +19,26 @@ export type BedrockToolChoice = | |||
| ToolChoice.AutoMember | |||
| ToolChoice.ToolMember; | |||
export type ChatBedrockConverseToolType = BindToolsInput | BedrockTool; | |||
|
|||
export type MessageContentReasoningBlockReasoningText = { | |||
type: "reasoningContent"; |
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.
These are our internal representations?
Why don't we just preserve the ones returned from 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.
They sort of forced our hand, although I could've kept to it a little more faithfully.
Concatenation of their delta blocks won't result in a full content block. They're structurally dissimilar, so I need to translate those to make that work.
Otherwise they discriminate their content block types via nesting them under mutually exclusive keys on the content block object, while we use the type
field as a discriminator. I'm just translating from their discriminator scheme to ours here. I suppose I could've made them be { type: "reasoningContent", reasoningContent: { ... } }
instead of spreading the reasoningContent
object there directly into the block, but that felt like poor design.
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.
Ok - only other concern then is to try to keep it inline with changes in Python 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.
Not sure who's picking up the python one, but we're consciously not forcing structural similarity between the two libs anymore unless there's a good reason to (e.g. they need to interop). Winds up causing too many pythonic conventions that feel weird in JS.
Edit, to elaborate for external parties: The goal behind this decision is to make the JS lib more predictable (and therefore easier to adopt and use) for JS/TS devs. We'll still aim to keep things functionally and conceptually similar, and if a design works well for both sides there's no reason to waste cycles redoing it for the other. We only want to be forcing close similarity in cases when doing so adds more direct value to users than the alternative would.
09460f0
to
7fd3d6e
Compare
No description provided.