Fix reasoning parsing when streaming from AWS SageMaker AI#2265
Fix reasoning parsing when streaming from AWS SageMaker AI#2265alvarobartt wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
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. |
awsarron
left a comment
There was a problem hiding this comment.
Thanks @alvarobartt for your contribution!
I added a couple of minor comments. Could we also add a unit test to ensure that this behaves as expected and doesn't regress in the future?
| # 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( |
There was a problem hiding this comment.
this is a bit hard to parse at a glance, could we simplify it, perhaps place in a private function and call that to make things more readable?
| "data": choice["delta"]["reasoning_content"], | ||
| # SAFETY: Here we guarantee that at least one of `reasoning` or `reasoning_content` | ||
| # is not None and a non-empty string | ||
| "data": choice["delta"].get("reasoning_content") |
There was a problem hiding this comment.
nit: could we write this on one line?
|
We'll need to rebase |
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.