Skip to content

fix: Add parity in behavior and tests with vllm nemotron_v3 for tools parser - #9058

Merged
indrajit96 merged 11 commits into
mainfrom
ibhosale/nemotron_v3_vllm_tools_parser_fix
May 11, 2026
Merged

fix: Add parity in behavior and tests with vllm nemotron_v3 for tools parser#9058
indrajit96 merged 11 commits into
mainfrom
ibhosale/nemotron_v3_vllm_tools_parser_fix

Conversation

@indrajit96

@indrajit96 indrajit96 commented May 2, 2026

Copy link
Copy Markdown
Contributor

Overview:

Port vLLM Nemotron v3 reasoning parser coverage into Dynamo and align Dynamo’s parser naming/disabled-thinking behavior with the vLLM contract.

Why:

vLLM added Nemotron v3 reasoning parser with tests.
Dynamo has a separate Rust implementation of reasoning parsing, so this PR adds matching coverage to make the expected Nemotron v3 behavior explicit and trackable.

vLLM source test file:
https://github.com/vllm-project/vllm/blob/main/tests/reasoning/test_nemotron_v3_reasoning_parser.py

Missing relevant vLLM test cases:

  • test_nemotron_v3_reasoning
    • without_start_token
    • without_start_token_streaming
    • with_start_token
    • with_start_token_streaming
  • test_nemotron_v3_without_thinking_returns_content
  • test_nemotron_v3_force_nonempty_content_returns_content

Details:

  • Added nemotron_v3 as a reasoning parser alias for the existing force-reasoning ... parser.

  • Added direct vLLM parity tests for Nemotron v3 reasoning extraction:

    • with and without opening
    • non-streaming and streaming paths
  • Added postprocessor coverage for vLLM request-flag behavior:

    • enable_thinking=false returns normal content
    • force_nonempty_content=true strips a leading and returns normal content
  • Documented nemotron_v3 as the vLLM-compatible alias.

    Newly added vLLM-to-Dynamo test mapping:

    vLLM test case Newly added Dynamo coverage
    without_start_token test_nemotron_v3_detect_and_parse_vllm_cases
    with_start_token test_nemotron_v3_detect_and_parse_vllm_cases
    without_start_token_streaming test_nemotron_v3_streaming_vllm_cases
    with_start_token_streaming test_nemotron_v3_streaming_vllm_cases
    test_nemotron_v3_without_thinking_returns_content postprocessor_parsing_stream_nemotron_v3_enable_thinking_false_returns_content
    test_nemotron_v3_force_nonempty_content_returns_content postprocessor_parsing_stream_nemotron_v3_force_nonempty_strips_start_token

Additional Dynamo-only regression coverage:

Scenario Newly added Dynamo coverage
Keep disabled-reasoning <think> strip state independent per streamed choice postprocessor_parsing_stream_nemotron_v3_force_nonempty_tracks_prefix_per_choice

Where should the reviewer start?

  • lib/parsers/src/reasoning/mod.rs
    • parser alias registration
    • vLLM parity tests
  • lib/llm/src/preprocessor.rs
    • request-flag handling for nemotron_v3
    • leading <think> stripping when reasoning is disabled
  • lib/llm/tests/postprocessor_parsing_stream.rs
    • postprocessor tests for enable_thinking=false and force_nonempty_content=true
  • docs/agents/reasoning.md
    • parser-name documentation update

Open in Devin Review

Summary by CodeRabbit

  • New Features

    • Added support for nemotron_v3 as an alternative name for Nemotron reasoning parsers
    • Implemented per-request reasoning extraction control with improved handling of reasoning markers
  • Documentation

    • Updated supported reasoning parsers reference to reflect upstream vLLM mapping for Nemotron

Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
@indrajit96
indrajit96 requested a review from a team May 2, 2026 23:05
@indrajit96
indrajit96 requested review from nealvaidya and rmccorm4 May 2, 2026 23:05
@github-actions github-actions Bot added documentation Improvements or additions to documentation frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels May 2, 2026
@indrajit96
indrajit96 requested a review from KrishnanPrash May 2, 2026 23:06
@indrajit96 indrajit96 changed the title Add parity in behavior and tests with vllm nemotron_v3 for tools parser fix: Add parity in behavior and tests with vllm nemotron_v3 for tools parser May 2, 2026
@github-actions github-actions Bot added the fix label May 2, 2026
@coderabbitai

coderabbitai Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The PR adds support for disabling reasoning extraction in vLLM Nemotron v3 responses when specific chat template arguments are set. It introduces a new parser alias (nemotron_v3), implements reasoning-stripping logic for streams, and adds corresponding test coverage.

Changes

Nemotron v3 Reasoning Disabling

Layer / File(s) Summary
Parser Registry
lib/parsers/src/reasoning/mod.rs
New nemotron_v3 alias is registered, mapping to ReasoningParserType::DeepseekR1. Test expectations are updated to include the new alias, and old deepseek_v4 streaming test is removed. Two new nemotron_v3 streaming and detection tests are added.
Reasoning Disable Logic
lib/llm/src/preprocessor.rs
New reasoning_disabled_by_request flag is computed from parser name and chat_template_args. New helper should_strip_reasoning_start_when_disabled() identifies Nemotron v3-family parsers requiring stripping. New stream transformer strip_leading_reasoning_start_from_stream() robustly buffers and removes leading <think> markers across chunk boundaries.
Preprocessing Integration
lib/llm/src/preprocessor.rs
postprocessor_parsing_stream is refactored to gate reasoning extraction by the new disable flag. When disabled for v3 parsers, the stream is routed through the stripping transformer instead of direct reasoning parsing. Test setup adds force_nonempty_content_true argument map and extends test cases with v3 aliases and disabled-reasoning expectations.
Stream Processing Tests
lib/llm/tests/postprocessor_parsing_stream.rs
Two new Tokio tests verify Nemotron v3 behavior: one with enable_thinking=false producing empty reasoning, another with force_nonempty_content=true stripping a leading <think> token split across chunk boundaries.
Documentation
docs/agents/reasoning.md
Table entry for nemotron3 is updated to specify vLLM upstream name (nemotron_v3) and clarify that the parser accepts both aliases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding vLLM nemotron_v3 parity to Dynamo's reasoning parser, which is the primary focus of all file changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is comprehensive and well-structured, covering all required sections of the template with detailed context about objectives, changes, test mappings, and specific files for reviewer focus.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

💬 Introducing Slack Agent: Turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value).


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/llm/src/preprocessor.rs`:
- Around line 1494-1524: The stream-end path currently discards any bytes left
in state.buffer (so a partial think prefix like "<thi" is lost); update the
branch that returns None at stream completion to check state.buffer and, if
non-empty, emit it as normal content instead of dropping it — e.g. construct an
output string from state.buffer (taking into account stripping think_start_token
if appropriate, using state.think_start_token and the same logic used earlier)
and set choice.delta.content = Some(ChatCompletionMessageContent::Text(...))
before returning the processed_response/state; ensure state.buffer is cleared
afterwards and that existing fields (state.decided, choice.delta.content) are
handled consistently with the other branch.
- Around line 1457-1516: The current strip_leading_reasoning_start_from_stream
uses a single StripReasoningStartState with shared buffer and decided fields,
which incorrectly mixes state across multiple choices in a single streamed
response; update StripReasoningStartState to track per-choice state keyed by
choice.index (e.g., a HashMap<usize, (buffer: String, decided: bool)>), and then
in the processing closure use the choice.index to lookup/create the per-choice
buffer/decided pair, operate on that pair when accumulating/checking
think_start_token, write back the possibly-trimmed output into
choice.delta.content, and remove the per-choice entry when you know the choice
is finished to avoid unbounded growth. Ensure you reference
strip_leading_reasoning_start_from_stream, StripReasoningStartState,
choice.index, state.buffer and state.decided in your changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0a851996-4dea-498b-9735-dd507f91f757

📥 Commits

Reviewing files that changed from the base of the PR and between 297119e and d49d5de.

📒 Files selected for processing (4)
  • docs/agents/reasoning.md
  • lib/llm/src/preprocessor.rs
  • lib/llm/tests/postprocessor_parsing_stream.rs
  • lib/parsers/src/reasoning/mod.rs

Comment thread lib/llm/src/preprocessor.rs
Comment thread lib/llm/src/preprocessor.rs Outdated
@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Comment thread lib/llm/src/preprocessor.rs Outdated
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
indrajit96 added 2 commits May 7, 2026 14:22
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Comment thread lib/llm/src/preprocessor.rs Outdated
Comment thread lib/llm/src/preprocessor.rs
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>

@keivenchang keivenchang 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.

thanks Indrajit — helper extraction, new PRE.6 doc, and the PARSER_CASES rename all landed cleanly. ship it.

@indrajit96
indrajit96 enabled auto-merge (squash) May 11, 2026 19:14
@indrajit96
indrajit96 merged commit f6117b8 into main May 11, 2026
103 checks passed
@indrajit96
indrajit96 deleted the ibhosale/nemotron_v3_vllm_tools_parser_fix branch May 11, 2026 19:27
krishung5 added a commit that referenced this pull request May 11, 2026
Merge with main pulls in #9058's `choice.stop_reason = None;` against
#8119's `ChatChoiceStream` (which no longer has that field). Path-
filtered CI on main's next commit (#9230, sglang-only) skipped rust-
clippy so the broken combination landed silently — exposed here by
the merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
krishung5 added a commit that referenced this pull request May 11, 2026
Merge with main pulls in #9058's `choice.stop_reason = None;` against
#8119's `ChatChoiceStream` (which no longer has that field). Path-
filtered CI on main's next commit (#9230, sglang-only) skipped rust-
clippy so the broken combination landed silently — exposed here by
the merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants