feat: minimax m3 dynamo changes - #10983
Conversation
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
This comment has been minimized.
This comment has been minimized.
WalkthroughThe PR adds MiniMax-M3 reasoning and parser normalization across OpenAI, preprocessor, and Sglang frontend paths, validates assistant tool-call arguments as JSON objects, strips trailing EOS token IDs from streamed chunks, and adds a multimodal context-length fallback in model-card loading. ChangesReasoning and streaming handling
Model card context fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/llm/src/protocols/openai/chat_completions.rs (1)
149-177: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve top-level request precedence when folding reasoning controls.
Line 149 and Line 170 use
or_insert(...), but Line 177 still dropsself.thinking. If a caller sendsthinking={"type":"disabled"}orreasoning_effortalongside stalechat_template_args, the explicit request field is silently ignored. That is user-visible for MiniMax becauseOpenAIPreprocessor::is_reasoning_disabled_by_request()only consultschat_template_args, so reasoning can stay enabled even though the request disabled it. Overwrite these normalized keys or reject conflicting combinations before clearing the source field.🤖 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/chat_completions.rs` around lines 149 - 177, Preserve request-field precedence in the OpenAI chat normalization logic: in the `chat_completions` path that updates `chat_template_args`, the current `or_insert` behavior lets stale template args win over explicit `thinking` or `reasoning_effort` values before `self.thinking` is cleared. Update the normalization in the relevant method so the request-provided values always override existing `chat_template_args`, or detect and reject conflicting combinations, and ensure `OpenAIPreprocessor::is_reasoning_disabled_by_request()` still sees the intended user request after `self.thinking` is set to `None`.components/src/dynamo/frontend/sglang_processor.py (1)
581-605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClearing
pending_usageat Line 583 drops thecached_tokensmetric on the same flush.
pending_usageis set toNoneimmediately after being attached todynamo_out["usage"], but the metrics block at Line 603 readspending_usageto derivecached_tokens. On the flush that emits usage (typically the finish chunk, wherecompletion_usagearrives),_cached_tokens_from_usage(None)returnsNone, socached_tokensis silently omitted from the metrics. Note Line 612 already resetspending_usageevery flush, so the early clear is redundant for preventing cross-cycle reuse.Capture the usage for metrics before clearing:
🐛 Proposed fix
envelope: dict[str, Any] = {"_dynamo_annotated": True} + usage_for_metrics = pending_usage if choice: dynamo_out: dict[str, Any] = { "id": request_id, "choices": [choice], "created": created_ts, "model": request["model"], "object": "chat.completion.chunk", } if pending_usage: dynamo_out["usage"] = pending_usage pending_usage = None @@ cached_tokens = _cached_tokens_from_usage(pending_usage) + cached_tokens = _cached_tokens_from_usage(usage_for_metrics) if cached_tokens is not None: metrics["cached_tokens"] = cached_tokens🤖 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 `@components/src/dynamo/frontend/sglang_processor.py` around lines 581 - 605, The flush logic in sglang_processor is clearing pending_usage too early, which causes the metrics block to miss cached_tokens on the same emission. In the section that builds dynamo_out and metrics, preserve the usage value long enough for _cached_tokens_from_usage to read it, then clear pending_usage only after cached_tokens has been derived and added to metrics. Use the existing pending_usage, dynamo_out["usage"], and _cached_tokens_from_usage symbols to keep the cached token metric available on the finish chunk.
🤖 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.
Inline comments:
In `@components/src/dynamo/frontend/sglang_processor.py`:
- Line 415: `SglangProcessor` is only forwarding a single EOS id, so models with
multiple EOS tokens won’t get correct stop handling or trailing-EOS stripping.
Update the request setup in `SglangProcessor` to use the full EOS id list
resolved by `eos_token_ids()` instead of `tokenizer.eos_token_id`, and pass that
same full list into `SglangStreamingPostProcessor` so both request construction
and streaming post-processing stay aligned.
In `@lib/llm/src/model_card.rs`:
- Around line 1445-1461: The fallback logic in
model_card::architectural_max_context_length is too broad because `.or_else(|_|
...)` treats any `config.json` failure as a missing field. Change the chain so
only “field not found” cases fall back to `text_config` or
`tokenizer_config.json`, while parse/type errors from `crate::file_json_field`,
the `text_config` lookup, or the `serde_json::from_value` step are surfaced
immediately. Keep the existing flow in `model_card.rs` but make the error
handling distinguish absent keys from malformed metadata so
`effective_context_length()` is not computed from a bad repo config.
---
Outside diff comments:
In `@components/src/dynamo/frontend/sglang_processor.py`:
- Around line 581-605: The flush logic in sglang_processor is clearing
pending_usage too early, which causes the metrics block to miss cached_tokens on
the same emission. In the section that builds dynamo_out and metrics, preserve
the usage value long enough for _cached_tokens_from_usage to read it, then clear
pending_usage only after cached_tokens has been derived and added to metrics.
Use the existing pending_usage, dynamo_out["usage"], and
_cached_tokens_from_usage symbols to keep the cached token metric available on
the finish chunk.
In `@lib/llm/src/protocols/openai/chat_completions.rs`:
- Around line 149-177: Preserve request-field precedence in the OpenAI chat
normalization logic: in the `chat_completions` path that updates
`chat_template_args`, the current `or_insert` behavior lets stale template args
win over explicit `thinking` or `reasoning_effort` values before `self.thinking`
is cleared. Update the normalization in the relevant method so the
request-provided values always override existing `chat_template_args`, or detect
and reject conflicting combinations, and ensure
`OpenAIPreprocessor::is_reasoning_disabled_by_request()` still sees the intended
user request after `self.thinking` is set to `None`.
🪄 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: Enterprise
Run ID: c7adbc4e-cacd-4206-80f9-b8fea6d7eb84
📒 Files selected for processing (8)
components/src/dynamo/frontend/sglang_prepost.pycomponents/src/dynamo/frontend/sglang_processor.pycomponents/src/dynamo/frontend/tests/test_sglang_processor_unit.pylib/llm/src/http/service/openai.rslib/llm/src/model_card.rslib/llm/src/preprocessor.rslib/llm/src/protocols/openai/chat_completions.rslib/llm/src/protocols/openai/validate.rs
…ynamo Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
|
Please take a look at the Failing M3 CI [Ref]: |
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
architectural_max_context_length_from_repo (added in ai-dynamo#10983) deserializes config.json into a serde_json::Value using strict serde_json, which rejects the non-finite literal Infinity that HF configs such as Nemotron-H emit for fields like time_step_limit. The strict parse aborts before the tolerant JSON5 path in HFConfig::from_json_file runs, regressing model registration for those configs (Failed to parse JSON from file: .../config.json). Parse with json_five into a minimal struct projection instead, mirroring HFConfig::from_json_file. Only the consulted fields are captured (as raw serde_json::Value to preserve the existing per-field error messages); every other entry, including any non-finite literals, is skipped by serde. Signed-off-by: ssojrani@nvidia.com <ssojrani@nvidia.com>
Overview:
Adds MiniMax M3 support to Dynamo’s OpenAI frontend path, including request-level
thinking/thinking_modehandling, parser aliasing, special-token/EOS handling, prior tool-call message validation, and MiniMax-M3-VL model-card context-length detection for multimodal configs.Details:
Add MiniMax M3 thinking-mode support:
- Normalize OpenAI
thinkinginto both genericthinkingand MiniMax-stylethinking_mode.- Support
enabled,disabled, andadaptive.- Make SGLang force-reasoning respect
thinking_mode=disabled.Add MiniMax-M3-VL multimodal config support:
config.json.text_config.max_position_embeddingsbefore falling back to tokenizer config.Add MiniMax M3 reasoning behavior:
<mm:think>instead of generic<think>.thinking_mode=disabled.Add SGLang frontend MiniMax M3 handling:
minimax_m3,minimax_m3_nom, andminimax-m3-nomto SGLang’sminimax-m3.thinking_mode.Validate prior assistant tool-call messages:
messages[*].tool_calls[*].function.argumentsunless it is a valid JSON object string.Fix SGLang stream usage emission:
Where should the reviewer start?
Start with:
Then review:
Related Issues
Related to ai-dynamo/frontend-crates#83
Summary by CodeRabbit
New Features
enabled,disabled, andadaptive.Bug Fixes
Closes DIS-2270