Fix reasoning parsing when streaming from AWS SageMaker AI#2265
Fix reasoning parsing when streaming from AWS SageMaker AI#2265alvarobartt wants to merge 1 commit intostrands-agents:mainfrom
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| # NOTE: Both `reasoning` and `reasoning_content` need to be handled as vLLM v0.16.0 deprecated | ||
| # the `reasoning_content` in favour of `reasoning` | ||
| # See https://github.com/vllm-project/vllm/pull/33402 | ||
| if any(k in choice["delta"] for k in {"reasoning_content", "reasoning"}): |
There was a problem hiding this comment.
Issue: The condition checks for key existence but not value truthiness. If vLLM sends "reasoning": None or "reasoning": "" in the delta (common in streaming when the key is present but no reasoning content is being sent on that chunk), this will enter the block and emit an empty/None delta.
Compare with the text content check on line 358 which uses .get("content") (truthy check), and the OpenAI provider which uses choice.delta.reasoning_content (also truthy).
Suggestion: Use a truthiness check consistent with the rest of the codebase:
reasoning_text = choice["delta"].get("reasoning_content") or choice["delta"].get("reasoning")
if reasoning_text:This also simplifies line 385 since the value is already extracted.
| "chunk_type": "content_delta", | ||
| "data_type": "reasoning_content", | ||
| "data": choice["delta"]["reasoning_content"], | ||
| "data": choice["delta"].get("reasoning_content", choice["delta"].get("reasoning")), |
There was a problem hiding this comment.
Issue: dict.get(key, default) returns None when the key exists with value None—the default is only used when the key is missing. If a response contains {"reasoning_content": None, "reasoning": "actual text"}, this expression returns None instead of "actual text".
Suggestion: Use or for a falsy-fallback:
"data": choice["delta"].get("reasoning_content") or choice["delta"].get("reasoning"),|
Issue: No unit tests cover the reasoning content streaming path. Codecov confirms 0% patch coverage. There are no existing tests for reasoning content in Suggestion: Please add at minimum:
These should be straightforward to add following the existing |
|
Assessment: Request Changes The fix addresses a real issue (vLLM v0.16.0+ deprecating Review Categories
Thanks for tracking down the root cause to the specific vLLM version change — the PR description is very well-researched. |
Description
As of vllm-project/vllm#33402 released in vLLM v0.16.0, see https://github.com/vllm-project/vllm/releases/tag/v0.16.0; the
reasoning_contentfield within thedeltawhen streaming has been deprecated in favour ofreasoning.Also given that the latest AWS SageMaker AI DLC for vLLM runs with vLLM v0.20.1, see https://aws.github.io/deep-learning-containers/vllm/ and https://github.com/aws/deep-learning-containers/blob/9d519f6bca375b87422e5429803e7f2c3ca390df/docker/vllm/Dockerfile#L3, this means that the reasoning content when streaming will be ignored, whereas with this PR the content will be correctly parsed.
Note
By submitting this PR, I disclose that all the code in this PR was written entirely by me, @alvarobartt, without the use of any coding assistants or third-party agentic tools.
Related Issues
#2182 and #2191, though both of those wrongly claim that the issue is with vLLM v0.19.1 whereas https://github.com/vllm-project/vllm/releases/tag/v0.16.0 says v0.16.0 onwards.
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
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.