Skip to content

Fix: Nemotron 3 rescue whitespace-only final_content, not just None - #41846

Merged
bbrowning merged 3 commits into
vllm-project:mainfrom
Naveassaf:bugfix/nemotron_parser_whitespace
May 10, 2026
Merged

bbrowning merged 3 commits into
vllm-project:mainfrom
Naveassaf:bugfix/nemotron_parser_whitespace

Conversation

@Naveassaf

Copy link
Copy Markdown
Contributor

Summary

Nemotron3's parser overrides DeepSeek-R1's parser to handle the enable_thinking=False / force_nonempty_content=True cases, where the model is asked to skip thinking but R1's parser still routes everything into reasoning whenever it can't cleanly parse a <think>...</think> block. To recover, the Nemotron parser swaps reasoning with final_content so the answer is delivered as content rather than reasoning.

The swap was gated on final_content is None, which misses the case where the model emits whitespace-only content (e.g. a stray "\n" or " " after ). In that case final_content is a non-None string, the swap is skipped, and the client receives whitespace as the response while the actual answer remains stranded in reasoning.

This PR changes the Nemotron3 parser to match the upstream model-side fix shipped in the Nemotron Super checkpoint: https://huggingface.co/nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4/commit/4f0cf9daaeb7a4d5e23f80a00e7ed15f0e03caf6

Signed-off-by: Nave Assaf <nassaf@nvidia.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the NemotronV3ReasoningParser to treat whitespace-only final_content as empty when deciding whether to swap it with the reasoning field. Feedback suggests refining this logic to ensure the swap only occurs if the reasoning field itself contains non-whitespace content, preventing redundant or potentially invalid swaps.

I am having trouble creating individual review comments. Click here to see my feedback.

vllm/reasoning/nemotron_v3_reasoning_parser.py (29)

high

The current logic swaps reasoning and final_content whenever final_content is empty or whitespace. However, if reasoning is also None or whitespace-only, this swap could result in final_content becoming None (if reasoning was None) or remaining whitespace-only. This doesn't help 'rescue' an answer and might even violate the intent of force_nonempty_content by replacing a whitespace string with None. It is safer to only perform the swap if there is actual non-whitespace content in the reasoning field to be rescued.

            and (final_content is None or not final_content.strip())
            and (reasoning and reasoning.strip())

@bbrowning bbrowning changed the title Fix: rescue whitespace-only final_content (e.g. n), not just None Fix: Nemotron 3 rescue whitespace-only final_content, not just None May 8, 2026

@bbrowning bbrowning left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally we'd also add a test for this case to prevent regression. However, given that this exactly matches the change already merged upstream in the model's HuggingFace repo and it's a very small change, will approve this as-is.

If you're up for it, I'd happily approve a follow-up PR to add some additional unit test coverage for this reasoning parser for this or other edge cases it handles but that isn't covered by tests.

@bbrowning bbrowning added verified Run pre-commit for new contributors without triggering other tests ready ONLY add when PR is ready to merge/full CI is needed labels May 8, 2026
@bbrowning
bbrowning enabled auto-merge (squash) May 8, 2026 20:48
@bbrowning
bbrowning merged commit dcb3135 into vllm-project:main May 10, 2026
39 checks passed
weifang231 pushed a commit to weifang231/eb-vllm that referenced this pull request May 13, 2026
…llm-project#41846)

Signed-off-by: Nave Assaf <nassaf@nvidia.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mfylcek pushed a commit to mfylcek/vllm that referenced this pull request May 19, 2026
…llm-project#41846)

Signed-off-by: Nave Assaf <nassaf@nvidia.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
hustcui pushed a commit to hustcui/vllm-hust that referenced this pull request Jul 10, 2026
…llm-project#41846)

Signed-off-by: Nave Assaf <nassaf@nvidia.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed verified Run pre-commit for new contributors without triggering other tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants