feat(RL): add nvext Tokens-in-Tokens-Out and RL related response protocol and frontend support - #9649
Conversation
WalkthroughThis PR extends the LLM backend and OpenAI protocol to surface prompt log-probabilities and additional sampling controls through request parsing, validation, preprocessing, response building, and delta emission across both chat and completion endpoints. ChangesPrompt Log-Probabilities and Extended Sampling Support
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/llm/src/protocols/unified.rs (1)
279-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelegate
unsupported_fields()forUnifiedRequest.
OpenAIPreprocessor::nvext_passthrough_argsrelies onNvExtProvider::unsupported_fields()fallback (e.g.,cache_salt). With the current impl,UnifiedRequestalways uses the trait defaultNone, so those fields are silently dropped on this path.Suggested fix
impl NvExtProvider for UnifiedRequest { fn nvext(&self) -> Option<&NvExt> { self.inner.nvext.as_ref() } fn raw_prompt(&self) -> Option<String> { None } + + fn unsupported_fields( + &self, + ) -> Option<&std::collections::HashMap<String, serde_json::Value>> { + NvExtProvider::unsupported_fields(&self.inner) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/llm/src/protocols/unified.rs` around lines 279 - 287, The UnifiedRequest NvExtProvider impl currently only implements nvext() and raw_prompt(), so unsupported_fields() falls back to the trait default and drops passthrough fields; add an unsupported_fields(&self) implementation that delegates through the inner nvext (e.g., return self.inner.nvext.as_ref().and_then(|nv| nv.unsupported_fields())) so OpenAIPreprocessor::nvext_passthrough_args sees fields like cache_salt; keep the implementation inside the impl NvExtProvider for UnifiedRequest and reference the existing nvext() and raw_prompt() methods for placement.
🧹 Nitpick comments (1)
lib/llm/src/protocols/openai/nvext.rs (1)
45-49: ⚡ Quick winAdd explicit test coverage for the DP-rank header alias path.
Line 48 introduces
x-data-parallel-rankfallback, but there’s no direct test for alias-only input (or both headers present). Add a focused test to prevent regressions in routing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/llm/src/protocols/openai/nvext.rs` around lines 45 - 49, Add unit tests for the DP-rank header alias path to ensure routing uses HEADER_DP_RANK_ALIAS when HEADER_DP_RANK is absent and that precedence is correct when both are present: create tests that call the header-parsing logic in nvext.rs (exercising the dp_rank extraction that uses HEADER_DP_RANK .or_else(HEADER_DP_RANK_ALIAS) and the dp_rank variable) with (a) only HEADER_DP_RANK_ALIAS set and (b) both HEADER_DP_RANK and HEADER_DP_RANK_ALIAS set, asserting the returned dp_rank matches the expected value and that no panics or unexpected routing occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/llm/src/protocols/unified.rs`:
- Around line 279-287: The UnifiedRequest NvExtProvider impl currently only
implements nvext() and raw_prompt(), so unsupported_fields() falls back to the
trait default and drops passthrough fields; add an unsupported_fields(&self)
implementation that delegates through the inner nvext (e.g., return
self.inner.nvext.as_ref().and_then(|nv| nv.unsupported_fields())) so
OpenAIPreprocessor::nvext_passthrough_args sees fields like cache_salt; keep the
implementation inside the impl NvExtProvider for UnifiedRequest and reference
the existing nvext() and raw_prompt() methods for placement.
---
Nitpick comments:
In `@lib/llm/src/protocols/openai/nvext.rs`:
- Around line 45-49: Add unit tests for the DP-rank header alias path to ensure
routing uses HEADER_DP_RANK_ALIAS when HEADER_DP_RANK is absent and that
precedence is correct when both are present: create tests that call the
header-parsing logic in nvext.rs (exercising the dp_rank extraction that uses
HEADER_DP_RANK .or_else(HEADER_DP_RANK_ALIAS) and the dp_rank variable) with (a)
only HEADER_DP_RANK_ALIAS set and (b) both HEADER_DP_RANK and
HEADER_DP_RANK_ALIAS set, asserting the returned dp_rank matches the expected
value and that no panics or unexpected routing occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6eb0b0e-a231-4196-8b4e-f258b6705f6f
📒 Files selected for processing (17)
lib/llm/src/backend.rslib/llm/src/migration.rslib/llm/src/preprocessor.rslib/llm/src/protocols/common.rslib/llm/src/protocols/common/llm_backend.rslib/llm/src/protocols/openai.rslib/llm/src/protocols/openai/chat_completions.rslib/llm/src/protocols/openai/chat_completions/delta.rslib/llm/src/protocols/openai/common_ext.rslib/llm/src/protocols/openai/completions.rslib/llm/src/protocols/openai/completions/delta.rslib/llm/src/protocols/openai/nvext.rslib/llm/src/protocols/openai/validate.rslib/llm/src/protocols/unified.rslib/llm/tests/test_streaming_usage.rslib/llm/tests/tool_choice.rslib/llm/tests/tool_choice_finish_reasons.rs
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
565fd7c to
2256c2a
Compare
Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
9f52a45 to
f46b9bf
Compare
|
Rebased on top of main. |
Summary
lib/llmprotocol, preprocessor, and response plumbing for nvext Tokens-in-Tokens-Outnvext.token_data,cache_salt,extra_fields, token constraints,prompt_logprobs, detokenize, and stop token IDs through the Rust request/response pathcache_saltandstop_token_ids; rejecttruncate_prompt_tokensuntil it is supportedScope
This PR is the Rust
lib/llmhalf of the nvext Tokens-in-Tokens-Out split. It intentionally excludes the vLLM worker implementation so reviewers can focus on the public/internal request protocol and response mapping first.Where To Review
lib/llm/src/protocols/openai/nvext.rslib/llm/src/preprocessor.rslib/llm/src/protocols/openai/completions.rslib/llm/src/protocols/openai/chat_completions.rslib/llm/src/protocols/openai/validate.rsLocal Tests
cargo test -p dynamo-llm protocols::openai::completions::tests:: --libcargo test -p dynamo-llm protocols::openai::chat_completions::tests:: --librustfmt --edition 2024 --check lib/llm/src/protocols/openai/validate.rs lib/llm/src/protocols/openai/completions.rs lib/llm/src/protocols/openai/chat_completions.rsgit diff --checkFollow-up
The vLLM worker-side implementation should be reviewed separately so its diff remains scoped to
components/src/dynamo/vllm/*.Notes
supersedes #9382
Summary by CodeRabbit