fix: no reasoning parser by default #2939
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughCLI help text for --dyn-reasoning-parser was clarified across backends to indicate optional usage. In Rust, DeltaGenerator.reasoning_parser was made optional, with guards added to skip reasoning parsing when not configured. Streaming postprocessing now treats all text as normal when no parser is present. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant D as DeltaGenerator
participant P as ReasoningParser (optional)
participant PP as Postprocessor/Streamer
C->>D: Provide delta chunk
alt Parser configured
D->>P: Parse delta into {normal_text, reasoning_content}
P-->>D: Parsed parts
D->>PP: Emit normal_text + reasoning_content (as reasoning)
else No parser configured
note right of D: Reasoning parsing skipped
D->>PP: Emit delta as normal text only
end
PP-->>C: Streamed response chunk
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/llm/src/protocols/openai/chat_completions/delta.rs (2)
100-106: Treat empty/whitespace parser names as “unspecified”Avoid enabling Basic parser when the configured name is an empty string. Trim and filter empties.
Apply this diff:
- let reasoning_parser = options - .runtime_config - .reasoning_parser - .as_deref() - .map(|parser_name| ReasoningParserType::get_reasoning_parser_from_name(parser_name)); + let reasoning_parser = options + .runtime_config + .reasoning_parser + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(ReasoningParserType::get_reasoning_parser_from_name);
335-347: Simplify branch; removeis_some()check andDefaultdependencyYou can rely solely on
create_reasoning_content(...)to decide and avoid requiringParserResult: Default. Also reduces branching.Apply this diff:
- // Handle reasoning parsing if enabled, otherwise treat all text as normal - let (normal_text, reasoning_content) = if self.reasoning_parser.is_some() { - let reasoning_parser_result = self - .create_reasoning_content(&delta.text, &delta.token_ids) - .unwrap_or_default(); - ( - reasoning_parser_result.get_some_normal_text(), - reasoning_parser_result.get_some_reasoning(), - ) - } else { - // No reasoning parser configured, treat all text as normal - (delta.text, None) - }; + // Handle reasoning parsing when configured; otherwise treat all text as normal + let (normal_text, reasoning_content) = + match self.create_reasoning_content(&delta.text, &delta.token_ids) { + Some(r) => (r.get_some_normal_text(), r.get_some_reasoning()), + None => (delta.text, None), + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/backends/sglang/src/dynamo/sglang/args.py(1 hunks)components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py(1 hunks)components/backends/vllm/src/dynamo/vllm/args.py(1 hunks)lib/llm/src/protocols/openai/chat_completions/delta.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nachiketb-nvidia
PR: ai-dynamo/dynamo#2750
File: lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py:24-48
Timestamp: 2025-08-27T20:27:44.446Z
Learning: In the BasicReasoningParser class in lib/bindings/python/src/dynamo/reasoning_parser/basic_parser.py, the self._in_reasoning state variable in the detect_and_parse_reasoning method is intentionally included as an example for tweakability rather than for strict functional consistency between one-shot and streaming parsing methods.
Learnt from: nachiketb-nvidia
PR: ai-dynamo/dynamo#2656
File: lib/llm/src/protocols/openai/chat_completions/delta.rs:320-327
Timestamp: 2025-08-22T19:55:41.608Z
Learning: There are two separate DeltaGenerator classes in the codebase: one for chat completions (lib/llm/src/protocols/openai/chat_completions/delta.rs with object "chat.completion.chunk") and one for text completions (lib/llm/src/protocols/openai/completions/delta.rs with object "text_completion"). They have different create_choice method signatures and serve different OpenAI API endpoints. The reasoning parsing functionality is only relevant to the chat completions DeltaGenerator.
📚 Learning: 2025-08-22T19:55:41.608Z
Learnt from: nachiketb-nvidia
PR: ai-dynamo/dynamo#2656
File: lib/llm/src/protocols/openai/chat_completions/delta.rs:320-327
Timestamp: 2025-08-22T19:55:41.608Z
Learning: There are two separate DeltaGenerator classes in the codebase: one for chat completions (lib/llm/src/protocols/openai/chat_completions/delta.rs with object "chat.completion.chunk") and one for text completions (lib/llm/src/protocols/openai/completions/delta.rs with object "text_completion"). They have different create_choice method signatures and serve different OpenAI API endpoints. The reasoning parsing functionality is only relevant to the chat completions DeltaGenerator.
Applied to files:
lib/llm/src/protocols/openai/chat_completions/delta.rs
📚 Learning: 2025-08-22T19:55:41.608Z
Learnt from: nachiketb-nvidia
PR: ai-dynamo/dynamo#2656
File: lib/llm/src/protocols/openai/chat_completions/delta.rs:320-327
Timestamp: 2025-08-22T19:55:41.608Z
Learning: The create_choice method exists on multiple different objects in the codebase. The DeltaGenerator::create_choice in lib/llm/src/protocols/openai/chat_completions/delta.rs has its own signature that was updated to include reasoning_content, but other objects in lib/llm/src/engines.rs have their own separate create_choice methods with different signatures that are not related to chat completions.
Applied to files:
lib/llm/src/protocols/openai/chat_completions/delta.rs
📚 Learning: 2025-09-08T21:18:43.464Z
Learnt from: nachiketb-nvidia
PR: ai-dynamo/dynamo#2936
File: lib/parsers/src/reasoning/granite_parser.rs:42-46
Timestamp: 2025-09-08T21:18:43.464Z
Learning: In GraniteReasoningParser in lib/parsers/src/reasoning/granite_parser.rs, the think_start_tokens and think_end_tokens are hardcoded in the constructor with fixed values, so unwrap() calls on these vectors are safe and won't panic.
Applied to files:
lib/llm/src/protocols/openai/chat_completions/delta.rs
🧬 Code graph analysis (1)
lib/llm/src/protocols/openai/chat_completions/delta.rs (2)
lib/bindings/python/rust/llm/local_model.rs (1)
reasoning_parser(76-78)lib/parsers/src/reasoning/mod.rs (1)
get_reasoning_parser_from_name(171-187)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test - vllm
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (5)
components/backends/sglang/src/dynamo/sglang/args.py (1)
45-46: Help text clarification looks goodAccurately communicates the new default (no parsing when unspecified). Matches PR intent.
components/backends/vllm/src/dynamo/vllm/args.py (1)
117-118: Consistent optionality messagingHelp text now clearly reflects that parsing is optional; aligns with system behavior.
components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py (1)
286-287: Clearer CLI helpMatches the new default (no parser ⇒ no reasoning parsing). Good consistency across backends.
lib/llm/src/protocols/openai/chat_completions/delta.rs (2)
67-69: Struct field made optional — correct directionMaking
reasoning_parseranOptionwith doc explaining “None ⇒ no parsing” cleanly implements the new default.
201-203: Early return when no parser — correctShort-circuiting on
Noneavoids unnecessary work and matches the “no parser ⇒ no reasoning” behavior.
Signed-off-by: Neal Vaidya <[email protected]>
Signed-off-by: Neal Vaidya <[email protected]>
nealvaidya
force-pushed
the
nealv/no_default_reasoning
branch
from
a485124 to
caf67d8
Compare
Contributor
|
/ok to test caf67d8 |
grahamking
approved these changes
Sep 9, 2025
tedzhouhk
pushed a commit
that referenced
this pull request
Sep 10, 2025
Signed-off-by: Neal Vaidya <[email protected]> Signed-off-by: hongkuanz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview:
Previously, we defaulted to using the
BasicReasoningParserif no reasoning parser was specified, which extracts anything between<think> </think>tags as being reasoning content. With this PR, when no reasoning parser is specified, all content is treated as "Normal" non-reasoning content. This aligns with the behavior of vllm and sglang.Note
This PR retains the behavior that specifying a non-existent reasoning parser causes dynamo to fallback to the basic parser, rather than no parser. I think this is probably still the desired behavior.
Summary by CodeRabbit