chore: deprecate duplicate params in nvext #2754
Conversation
ryan-lempka
force-pushed
the
rlempka/deprecate-duplicate-params-nvext
branch
from
152a9b1 to
6c5a9da
Compare
WalkthroughIntroduces deprecation-aware helpers for resolving overlapping CommonExt vs NvExt fields and applies them to guided options and ignore_eos across OpenAI completions and chat completions. Adds get_ignore_eos accessors, centralizes precedence (CommonExt over NvExt), and emits deprecation warnings when NvExt values are considered. Updates default ignore_eos resolution in protocols. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Provider Getter
participant CE as CommonExt (root)
participant NV as NvExt
participant Helper as choose_with_deprecation()
participant Log as Deprecation Logger
Caller->>Helper: resolve(field, CE.value?, NV.value?)
alt NV has value
Helper->>Log: emit_nvext_deprecation_warning(field, nv=Some, common=CE.is_some())
end
alt CE has value
Helper-->>Caller: return CE.value
else NV has value
Helper-->>Caller: return NV.value
else none
Helper-->>Caller: return None
end
sequenceDiagram
autonumber
actor API as Request (NvCreate*Request)
participant Provider as OpenAIStopConditionsProvider
participant Helper as choose_with_deprecation()
note over API,Provider: Compute effective ignore_eos
API->>Provider: get_ignore_eos()
Provider->>Helper: (\"ignore_eos\", common_ignore_eos?, nvext_ignore_eos?)
Helper-->>Provider: Option<bool>
Provider-->>API: Option<bool>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Signed-off-by: Ryan Lempka <rlempka@nvidia.com>
ryan-lempka
force-pushed
the
rlempka/deprecate-duplicate-params-nvext
branch
from
6c5a9da to
618c96d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
lib/llm/src/protocols/openai/common_ext.rs (3)
70-85: Centralized deprecation warnings: add a tracing target for easy filteringRecommend adding a target so downstreams can silence or route these warnings.
- tracing::warn!( + tracing::warn!(target: "deprecations", "DEPRECATION WARNING: 'nvext.{field_name}' is deprecated and will be removed in a future release. Use '{field_name}' at the top level or in 'extra_body' instead." ); ... - tracing::warn!( + tracing::warn!(target: "deprecations", "DEPRECATION WARNING: 'nvext.{field_name}' is deprecated and will be removed in a future release. Top-level '{field_name}' takes precedence. Use '{field_name}' at the top level or in 'extra_body' instead." );
87-98: Helper looks solid; consider a ref variant to unify guided_json handlingAdding a reference-returning variant avoids bespoke code paths for ref-based getters.
pub fn choose_with_deprecation<T: Clone>( @@ common.cloned().or_else(|| nv.cloned()) } + +/// Reference-returning variant for fields whose getters return references. +pub fn choose_ref_with_deprecation<'a, T>( + field: &'static str, + common: Option<&'a T>, + nv: Option<&'a T>, +) -> Option<&'a T> { + if nv.is_some() { + emit_nvext_deprecation_warning(field, true, common.is_some()); + } + common.or(nv) +}
195-213: Unit tests for precedence — LGTMCovers all three cases (common wins, nvext fallback, both None). Consider (optional) a log-assert test in a follow-up if you want to guarantee deprecation emits.
lib/llm/src/protocols/openai.rs (1)
64-68: Default get_ignore_eos centralization — LGTM; remove redundant per-type overridesThis default covers all providers. You can drop identical overrides in completions.rs and chat_completions.rs to reduce duplication.
lib/llm/src/protocols/openai/completions.rs (2)
148-153: guided_json: correct precedence and warning; consider using ref helper if addedIf you add choose_ref_with_deprecation, you can simplify this to one call.
- // Note: This one needs special handling since it returns a reference - if let Some(nvext) = &self.nvext - && nvext.guided_json.is_some() - { - emit_nvext_deprecation_warning("guided_json", true, self.common.guided_json.is_some()); - } - self.common - .guided_json - .as_ref() - .or_else(|| self.nvext.as_ref().and_then(|nv| nv.guided_json.as_ref())) + choose_ref_with_deprecation( + "guided_json", + self.common.guided_json.as_ref(), + self.nvext.as_ref().and_then(|nv| nv.guided_json.as_ref()), + )
218-226: Redundant get_ignore_eos override; rely on trait defaultSame logic as the default in openai.rs; recommend removing to avoid drift.
- /// Get the effective ignore_eos value, considering both CommonExt and NvExt. - /// CommonExt (root-level) takes precedence over NvExt. - fn get_ignore_eos(&self) -> Option<bool> { - choose_with_deprecation( - "ignore_eos", - self.get_common_ignore_eos().as_ref(), - NvExtProvider::nvext(self).and_then(|nv| nv.ignore_eos.as_ref()), - ) - }lib/llm/src/protocols/openai/chat_completions.rs (2)
154-159: guided_json: correct precedence and warning; consider ref helper to reduce bespoke codeMirrors the completions path; can be simplified with choose_ref_with_deprecation if added.
- // Note: This one needs special handling since it returns a reference - if let Some(nvext) = &self.nvext - && nvext.guided_json.is_some() - { - emit_nvext_deprecation_warning("guided_json", true, self.common.guided_json.is_some()); - } - self.common - .guided_json - .as_ref() - .or_else(|| self.nvext.as_ref().and_then(|nv| nv.guided_json.as_ref())) + choose_ref_with_deprecation( + "guided_json", + self.common.guided_json.as_ref(), + self.nvext.as_ref().and_then(|nv| nv.guided_json.as_ref()), + )
246-251: Redundant get_ignore_eos override; rely on trait defaultMatches the default implementation; safe to remove.
- /// Get the effective ignore_eos value, considering both CommonExt and NvExt. - /// CommonExt (root-level) takes precedence over NvExt. - fn get_ignore_eos(&self) -> Option<bool> { - choose_with_deprecation( - "ignore_eos", - self.get_common_ignore_eos().as_ref(), - NvExtProvider::nvext(self).and_then(|nv| nv.ignore_eos.as_ref()), - ) - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/llm/src/protocols/openai.rs(2 hunks)lib/llm/src/protocols/openai/chat_completions.rs(3 hunks)lib/llm/src/protocols/openai/common_ext.rs(2 hunks)lib/llm/src/protocols/openai/completions.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/completions.rslib/llm/src/protocols/openai/chat_completions.rs
🧬 Code graph analysis (3)
lib/llm/src/protocols/openai.rs (3)
lib/llm/src/protocols/openai/chat_completions.rs (1)
common_ext(148-150)lib/llm/src/protocols/openai/common_ext.rs (2)
common_ext(60-60)choose_with_deprecation(88-97)lib/llm/src/protocols/openai/completions.rs (1)
common_ext(142-144)
lib/llm/src/protocols/openai/completions.rs (2)
lib/llm/src/protocols/openai/common_ext.rs (3)
common_ext(60-60)choose_with_deprecation(88-97)emit_nvext_deprecation_warning(71-85)lib/llm/src/protocols/openai.rs (3)
nvext(43-43)nvext(53-53)get_ignore_eos(63-69)
lib/llm/src/protocols/openai/chat_completions.rs (2)
lib/llm/src/protocols/openai/common_ext.rs (7)
common_ext(60-60)choose_with_deprecation(88-97)emit_nvext_deprecation_warning(71-85)get_guided_regex(64-64)get_guided_grammar(65-65)get_guided_choice(66-66)get_guided_decoding_backend(67-67)lib/llm/src/protocols/openai/completions.rs (9)
common_ext(142-144)nvext(88-90)nvext(136-138)nvext(210-212)get_guided_regex(160-166)get_guided_grammar(168-176)get_guided_choice(178-184)get_guided_decoding_backend(186-194)get_ignore_eos(220-226)
⏰ 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). (3)
- GitHub Check: Build and Test - dynamo
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (13)
lib/llm/src/protocols/openai.rs (3)
11-11: Import of deprecation helper — LGTM
146-168: StopConditions extraction uses unified ignore_eos — LGTMPrecedence and validation are preserved.
62-68: All deprecation helpers correctly applied
- Every
get_guided_*getter in bothcompletions.rs(§160–188) andchat_completions.rs(§166–194) uses thechoose_with_deprecationhelper.- Both
get_guided_jsonimplementations emit an NV-ext deprecation warning viaemit_nvext_deprecation_warning.- The
get_ignore_eosmethod inopenai.rsdispatches throughchoose_with_deprecationfor the “ignore_eos” field.No further changes required.
lib/llm/src/protocols/openai/completions.rs (5)
27-29: Scoped imports for helpers — LGTM
160-166: guided_regex via helper — LGTM
168-176: guided_grammar via helper — LGTM
178-184: guided_choice via helper — LGTM
186-194: guided_decoding_backend via helper — LGTMlib/llm/src/protocols/openai/chat_completions.rs (5)
24-26: Scoped imports for helpers — LGTM
167-171: guided_regex via helper — LGTM
175-181: guided_grammar via helper — LGTM
185-189: guided_choice via helper — LGTM
193-199: guided_decoding_backend via helper — LGTM
Signed-off-by: Ryan Lempka <rlempka@nvidia.com> Signed-off-by: Jason Zhou <jasonzho@jasonzho-mlt.client.nvidia.com>
Signed-off-by: Ryan Lempka <rlempka@nvidia.com> Signed-off-by: Michael Shin <michaelshin@users.noreply.github.com>
Signed-off-by: Ryan Lempka <rlempka@nvidia.com> Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
Signed-off-by: Ryan Lempka <rlempka@nvidia.com> Signed-off-by: nnshah1 <neelays@nvidia.com>
Overview:
Deprecate parameters that are available both under
nvextand at the top level of the request. Going forward engine-level args should be placed at the top-level.This removes duplication within the API and aligns with the conventions most common today in API schemas associated with backends like vLLM and SGLang.
Details:
Parameters deprecated
nvext.ignore_eos-> Useignore_eosnvext.guided_json-> Useguided_jsonnvext.guided_regex-> Useguided_regexnvext.guided_grammar-> Useguided_grammarnvext.guided_choice-> Useguided_choicenvext.guided_decoding_backend-> Useguided_decoding_backendChanges:
choose_with_deprecation()helper for consistent deprecation handlingnvextparameters are usedNext Steps:
top_kandrepetition_penaltyto top-level and deprecate innvextnvextparameters entirelyWhere should the reviewer start?
Review the helper functions added to emit deprecation warnings and that all parameters to be deprecated have been covered.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Documentation
Tests