Skip to content
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(provider/amazon-bedrock): Add reasoning support #4980

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Und3rf10w
Copy link
Contributor

@Und3rf10w Und3rf10w commented Feb 25, 2025

This adds reasoning support for Claude models in Amazon Bedrock, as noted in issue #4977.

I have tested these changes in my environment, and confirmed that we receive reasoning in both streaming and non-streaming configurations.

In streaming and non-streaming, both the signature and reasoning text are properly extracted. You may want to convert this to use providerOptions instead of relying on additionalModelRequestFields in BedrockChatSettings, but I don't know the best practice for that.

@Und3rf10w Und3rf10w changed the title Added bedrock reasoning support Add bedrock reasoning support Feb 25, 2025
@Und3rf10w Und3rf10w changed the title Add bedrock reasoning support feat(provider/amazon-bedrock): Add reasoning support Feb 25, 2025
@shaper
Copy link
Contributor

shaper commented Feb 25, 2025

Thank you! I will take a look at this soon.

@Und3rf10w
Copy link
Contributor Author

Und3rf10w commented Feb 25, 2025

After some testing (from e7aeec0), it looks like something goes wrong when you have tool use involved with reasoning enabled.

I can at least get this error back when I'm doing a generation with a tool enabled, but not sure how to resolve this:

The model returned the following errors: messages.1.content.0.type: Expected thinking or redacted_thinking, but found text. When thinking is enabled, a final assistant message must start with a thinking block (preceeding the lastmost set of tool_use and tool_result blocks). We recommend you include thinking blocks from previous turns. To avoid this requirement, disable thinking.

EDIT: Resolved in 31bef7c

The issue was I wasn't adding the reasoningContent part in convertToBedrockChatMessages. I can confirm this now works in all the cases I have tested.

@david-arteaga
Copy link

For what it's worth I took this patch and manually applied it locally and it works with sonnet 3.7 with both generateText and streamText

@shaper
Copy link
Contributor

shaper commented Feb 27, 2025

This is on my radar and I aim to get to it soon, thank you for your continued work here and contributions. We plan to work with you to get this landed as soon as we can.

@Und3rf10w
Copy link
Contributor Author

This has literally been my experience with trying to write this, I swear:

image

I feel like every time I think I have it done right, I do some testing, then experience some random edge case and find I did something wrong.


@shaper, do you want to populate reasoningDetails[] on a response from doGenerate that includes a tool invocation?

For example, right now, as of 468a750, if we call generateText without any tools, then the reasoningDetails comes back populated:

image

However, if we call generateText with tools, then it will be empty, but can be extracted from the steps or response messages:

image

And later in the response.messages:

image

I genuinely have no idea how that's supposed to return, but something important to note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants