fix: The content field returns None #4689
Open
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
WalkthroughA single file modification updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/llm/src/protocols/openai/chat_completions/aggregator.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nachiketb-nvidia
Repo: ai-dynamo/dynamo PR: 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.
Learnt from: nachiketb-nvidia
Repo: ai-dynamo/dynamo PR: 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
Repo: ai-dynamo/dynamo PR: 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/aggregator.rs
📚 Learning: 2025-08-22T19:55:41.608Z
Learnt from: nachiketb-nvidia
Repo: ai-dynamo/dynamo PR: 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/aggregator.rs
⏰ 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). (8)
- GitHub Check: Build and Test - dynamo
- GitHub Check: clippy (.)
- GitHub Check: clippy (lib/runtime/examples)
- GitHub Check: tests (lib/bindings/python)
- GitHub Check: tests (launch/dynamo-run)
- GitHub Check: clippy (launch/dynamo-run)
- GitHub Check: clippy (lib/bindings/python)
- GitHub Check: tests (.)
Comment on lines
286
to
+292
| content: if delta.text.is_empty() { | ||
| None | ||
| // If we have reasoning content, provide an empty string instead of None. | ||
| if delta.reasoning_content.is_some() { | ||
| Some(String::new()) | ||
| } else { | ||
| None | ||
| } |
Contributor
There was a problem hiding this comment.
Add test coverage for the empty content with reasoning_content scenario.
The code change at lines 286-292 intentionally returns Some(String::new()) for the content field when reasoning_content is present but text is empty. While this behavior is clearly documented in the inline comment, there is no test coverage for this specific scenario. Add a test case to verify this behavior and prevent regressions.
🤖 Prompt for AI Agents
In lib/llm/src/protocols/openai/chat_completions/aggregator.rs around lines
286-292, add a unit test that constructs a delta with empty text and a
Some(reasoning_content) value, runs it through the aggregator code path that
builds the output struct, and asserts that the resulting content field is
Some(String::new()) (not None) while reasoning_content is preserved; place the
test alongside existing aggregator tests, name it clearly (e.g.,
empty_text_with_reasoning_returns_empty_string), and include any necessary
setup/mocks so it executes deterministically in CI.
Contributor
|
@drnikolaev In three months we will hit a problem. The fix is deleting this line. We will think: "Why did we add this line?". This PR needs to explain what problem you are fixing, and why we should not delete this line in three months (without referring to any NVIDIA internal tools). |
grahamking
reviewed
Dec 2, 2025
| role: delta.role.expect("delta should have a Role"), | ||
| content: if delta.text.is_empty() { | ||
| None | ||
| // If we have reasoning content, provide an empty string instead of None. |
Contributor
|
@ayushag-nv to help repro the original issue and review or update the fix on this one |
Contributor
|
Command Used: python -m dynamo.vllm --model nvidia/Llama-3_3-Nemotron-Super-49B-v1_5 --trust-remote-code --tensor-parallel-size 2 --max-model-len=65536 --gpu-memory-utilization 0.95 Not reproducible on main |
ayushag-nv
reviewed
Dec 5, 2025
Comment on lines
+288
to
+292
| if delta.reasoning_content.is_some() { | ||
| Some(String::new()) | ||
| } else { | ||
| None | ||
| } |
Contributor
There was a problem hiding this comment.
this fix does not make sense to me.
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:
Context: TODO
Fixes nvbug: https://nvbugs/5662384
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.