Skip to content

Fix reasoning parsing when streaming from AWS SageMaker AI#2265

Open
alvarobartt wants to merge 3 commits into
strands-agents:mainfrom
alvarobartt:main
Open

Fix reasoning parsing when streaming from AWS SageMaker AI#2265
alvarobartt wants to merge 3 commits into
strands-agents:mainfrom
alvarobartt:main

Conversation

@alvarobartt
Copy link
Copy Markdown

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_content field within the delta when streaming has been deprecated in favour of reasoning.

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

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/strands/models/sagemaker.py 0.00% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Comment thread src/strands/models/sagemaker.py Outdated
Comment thread src/strands/models/sagemaker.py Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Issue: No unit tests cover the reasoning content streaming path. Codecov confirms 0% patch coverage. There are no existing tests for reasoning content in test_sagemaker.py at all.

Suggestion: Please add at minimum:

  1. A test for streaming with "reasoning_content" key (backwards compatibility)
  2. A test for streaming with "reasoning" key (new vLLM v0.16.0+ behavior)
  3. A test for the non-streaming path with the same keys

These should be straightforward to add following the existing test_stream_with_streaming_enabled pattern.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Assessment: Request Changes

The fix addresses a real issue (vLLM v0.16.0+ deprecating reasoning_content in favor of reasoning), but the current implementation has a subtle correctness issue: it checks for key existence rather than value truthiness, which differs from how text content is handled on the line above and from other model providers. This could cause empty/None reasoning deltas to be emitted.

Review Categories
  • Correctness: The any(k in dict) pattern + dict.get(key, default) approach has edge cases where None values slip through. Using or-based fallback is both simpler and more robust.
  • Completeness: The non-streaming path (line 461) has the same bug but wasn't fixed. Both paths should be consistent.
  • Testing: No unit tests exist for reasoning content parsing. The PR checklist also indicates tests were not added.

Thanks for tracking down the root cause to the specific vLLM version change — the PR description is very well-researched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant