Skip to content

Fix reasoning parsing when streaming from AWS SageMaker AI#2265

Open
alvarobartt wants to merge 1 commit intostrands-agents:mainfrom
alvarobartt:main
Open

Fix reasoning parsing when streaming from AWS SageMaker AI#2265
alvarobartt wants to merge 1 commit intostrands-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.

@alvarobartt alvarobartt deployed to manual-approval May 8, 2026 09:36 — with GitHub Actions Active
@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!

# 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"}):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

@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