-
Notifications
You must be signed in to change notification settings - Fork 447
fix(models): filter reasoningContent in Bedrock requests using DeepSeek #652
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
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.
Hi thank you for the contribution.
We would like to get this fix in there are a few considerations before we can
- Is this simply a bedrock bug? I have initiated conversations internally regarding this. I am hoping to have the answer quickly.
- Should we filter incoming or outgoing? It seems that this PR filters the existing messages. I am more biased toward preventing this from entering the Messages array/Session Management eagerly rather than filtering when we call back again. There is definitely a valid use case for accessing these reasoning text blocks, but until we receive a request for that, it seems strange to pollute the messages array with content that is invalid. In my opinion, it is a one way decision for filtering on send, and a two way for filtering on receive, but I may switch no this.
- Unit tests are needed
Regardless, I will follow up regarding point 1, since that is most important and may resolve this issue for us.
Hi, I did a quick look at the issue across GitHub https://github.com/search?q=%22User+messages+cannot+contain+reasoning+content.+Please+remove+the+reasoning+content+and+try+again.%22&type=issues It seems there may be a bug with DeepSeek specifically langchain-ai/langchain-aws#408 where the signature is not being returned. To test this we should test with another reasoning model. The fix may be that we simply drop reasoningContent if the response is invalid aka signature is not present. |
Hey @dbschmigelski, thanks for your review! I tried with Bedrock Claude 3.7 Sonnet and it's working fine, I mainly use Bedrock and unfortunately I currently don't have access to the Claude 4 models so cannot test them from my side. Regarding preventing the reasoning content from entering the messages array/session management, I thought that it would be good to have a log of the reasoning content generated when using S3SessionManager to understand the agent's thought process for traceability, however if you deem that it is more suitable to avoid filtering and stop it from reaching the messages array I completely agree with your decision as there is no immediate valid use case for it. Let me know what the next steps should be, |
Will do, currently speaking internally with the Bedrock team to receive guidance on this. Apologies for the delay. We won't let this sit for too long, but I want to avoid making a change here, it getting fixed in a couple days, then us dealing with backwards compatibility issues forever. As for Will let you know when we have guidance. |
Understood, thank you for your response! Also, I got access to the Claude 4 models on Bedrock and tried all 3 of them which all seem to be working fine, not getting the error that I get with DeepSeek R1. Seems like this is specific to only DeepSeek. |
Hi, just pinging as an update. It is still believed that this is a bug in the DeepSeek implementation in Bedrock. We are engaging internally. We would rather solve this at the root than patch over it at the moment. Leaving this PR open in the event we cannot solve it quickly. |
Update, Bedrock is not going to change the behavior on their end so we will be proceeding with this fix. |
@dbschmigelski Understood thanks! Let me know if any changes should be made to this fix on my side and will do them. |
Hi, after several unfortunate git conflicts, I made a commit on the fork. We will follow the guardrails approach and perform the filtering on the outgoing request. Meaning the reasoningContent is stored in the SessionManager if once is provided. This does introduce model specific logic which is unfortunate. To make the logic more scalable in the future has been added to #780. |
Acknowledged 👍 |
…o avoid ValidationException
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.
Should we have an integ test for this?
(perhaps a follow-up; we need a suite of tests running against N models I think)
I agree, I think this plays into #780 where we will want to state the features capable of the models/providers then test them. |
…ek (strands-agents#652) * Fix: strip reasoningContent from messages before sending to Bedrock to avoid ValidationException * Using Message class instead of dict in _strip_reasoning_content_from_message(). * fix(models): filter reasoningContent blocks on Bedrock requests using DeepSeek * fix: formatting and linting * fix: formatting and linting * remove unrelated registry formatting * linting * add log --------- Co-authored-by: Dean Schmigelski <[email protected]>
Fix: strip reasoningContent from messages before sending to Bedrock to avoid ValidationException
Description
This is a PR in a response to bug I found #651 which updates the Bedrock model class in the Strands Agents SDK to strip reasoning content before calling the converse_stream operation to avoid the validation exception.
Related Issues
#651
Documentation PR
Type of Change
Bug fix
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
I locally installed my version of the lib with changes I made and tested it on my code which I was getting the error for and it started working with no ValidationException error.
hatch run prepare
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.