Skip to content

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

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

benjamincburns
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Feb 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 27, 2025 11:42pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Feb 27, 2025 11:42pm

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. size:L This PR changes 100-499 lines, ignoring generated files. auto:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Feb 27, 2025
@benjamincburns benjamincburns changed the base branch from main to ben/fix-merge-content February 27, 2025 07:54
@benjamincburns benjamincburns changed the title fix(core): don't create empty text content blocks feat(aws): support reasoning blocks for claude 3.7 Feb 27, 2025
@benjamincburns
Copy link
Contributor Author

reminder that I'll need to bump the version of core that AWS depends on when #7769 gets released.

@@ -19,3 +19,26 @@ export type BedrockToolChoice =
| ToolChoice.AutoMember
| ToolChoice.ToolMember;
export type ChatBedrockConverseToolType = BindToolsInput | BedrockTool;

export type MessageContentReasoningBlockReasoningText = {
type: "reasoningContent";
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

@benjamincburns benjamincburns Feb 27, 2025

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.

Base automatically changed from ben/fix-merge-content to main February 27, 2025 18:23
@dosubot dosubot bot added the lgtm PRs that are ready to be merged as-is label Feb 27, 2025
@benjamincburns benjamincburns merged commit b593473 into main Feb 27, 2025
24 checks passed
@benjamincburns benjamincburns deleted the ben/bedrock-claude-3-7 branch February 27, 2025 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs lgtm PRs that are ready to be merged as-is size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants