Repository navigation
fix(frontend): adopt MiniMax M2 thinking toggle fix - #15586
mattwittwer wants to merge 4 commits into
Conversation
Signed-off-by: Matthew Wittwer <mwittwer@nvidia.com>
github-actions
Bot
added
fix
frontend
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughPreprocessing now carries ChangesReasoning state in preprocessing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change aligns MiniMax reasoning state with the rendered prompt and is covered by new tests. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description covers the overview, implementation details, and reviewer starting point. It does not meet the required issue-link requirement: it confirms that no issue is linked, and a dependency on another pull request does not replace an issue reference.
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
❌ Dynamo PR CI failed — run 37394397068 (attempt 1) on
|
| Other | Jobs |
|---|---|
| dynamo-runtime | ❌ 1 ✅ 7 |
Failure details
1 job failed: one dynamo_llm unit test, minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override, fails an assertion in cargo test. No jobs were cancelled.
❌ dynamo-runtime / rust-gpu: minimax_m2 disabled-thinking test assertion failed (preprocessor.rs:10980)
Job: dynamo-runtime / rust-gpu · Failed step: Run Rust checks (block-manager + media-ffmpeg + MM routing + integration tests) · Logs: gh run view --job 112063124005 -R ai-dynamo/dynamo --log-failed
thread 'preprocessor::tests::minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override' panicked at lib/llm/src/preprocessor.rs:10980:17:
thinking/Some(Object {"type": String("function"), "function": Object {"name": String("calculate")}}): disabled thinking must close the empty block: "<minimax:tool_call>Use the calculator tool for 937 * 18 + 42.\n]~b]ai\n<think>\n"
test preprocessor::tests::minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override ... FAILED
failures:
preprocessor::tests::minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override
test result: FAILED. 3117 passed; 1 failed; 8 ignored; 0 measured; 0 filtered out; finished in 37.00s
error: test failed, to rerun pass `--lib`
In the dynamo_llm lib unit tests (3117 passed, 1 failed), preprocessor::tests::minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override panics at lib/llm/src/preprocessor.rs:10980. The test case uses the named tool choice calculate. With thinking disabled, the rendered MiniMax-M2 prompt ends with an open <think>\n but the assertion expects the empty think block to be closed. The prompt also starts with <minimax:tool_call> in front of the user text. The other panics in the log come from passing tests that inject panics on purpose; they are not related.
For agents
{"pr": 15586, "run_id": 37394397068, "run_attempt": 1, "head_sha": "418fa306e6f1b5863291fbabb191439e5be5666c", "failures": [{"job": "dynamo-runtime / rust-gpu", "job_id": 112063124005, "failed_step": "Run Rust checks (block-manager + media-ffmpeg + MM routing + integration tests)", "signature": "dynamo_llm unit test panic: disabled thinking must close the empty block (preprocessor.rs:10980)", "tests": ["preprocessor::tests::minimax_m2_disabled_thinking_closes_prompt_and_forwards_no_override"], "log_cmd": "gh run view --job 112063124005 -R ai-dynamo/dynamo --log-failed"}]}Posted automatically by Devin for run 37394397068. Updated on every full-CI run of this PR.
Signed-off-by: Matthew Wittwer <mwittwer@nvidia.com>
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The compile failure remains at the reviewed head: Cargo.toml and Cargo.lock still pin dynamo-renderer 5.4.0, which does not export PromptReasoningState or the new RenderedPrompt reasoning-state methods used by preprocessor.rs. Update and lock dynamo-renderer to a published version containing the required API before merging.
- Original discussion: The reviewed head still pins
dynamo-renderer5.4.0 in bothCargo.tomlandCargo.lock, but that release does not exportPromptReasoningState; the new import therefore fails with E0432 and preventsdynamo-llmand dependent images from compiling. Update and lock the renderer dependency to a published version containing the required API. - Original discussion: The reviewed head still pins dynamo-renderer 5.4.0, which does not export PromptReasoningState, so the new import prevents dynamo-llm and dependent images from compiling. Rebase onto the supplied base, which pins renderer 6.0.0, or update the renderer dependency and lockfiles explicitly.
Signed-off-by: Matthew Wittwer <mwittwer@nvidia.com>
Signed-off-by: Matthew Wittwer <mwittwer@nvidia.com>
mattwittwer
changed the title
mattwittwer
changed the title
| .preprocess_request(&request, None) | ||
| .await | ||
| .unwrap(); | ||
| assert!(!injected, "{key}/{choice:?}"); |
There was a problem hiding this comment.
BEFORE:
assert!(!injected, "{key}/{choice:?}");
...
.apply_tool_choice_guided_decoding(&request, &mut prepared, injected)
...
assert!(injected, "{args}");
AFTER:
assert!(!injected.unified, "{key}/{choice:?}");
...
.apply_tool_choice_guided_decoding(&request, &mut prepared, injected.unified)
...
assert!(injected.unified, "{args}");Since #14950, preprocess_request returns PromptReasoningPrefill, so after a rebase onto main this line and L11043 fail with E0600 and L11006 fails with E0308. Use injected.unified, the value generate passes to apply_tool_choice_guided_decoding.
| for key in ["thinking", "enable_thinking"] { | ||
| for choice in &choices { | ||
| let mut request = | ||
| minimax_m2_request(serde_json::json!({key: false}), choice.clone()); |
There was a problem hiding this comment.
BEFORE:
for key in ["thinking", "enable_thinking"] {
for choice in &choices {
let mut request =
minimax_m2_request(serde_json::json!({key: false}), choice.clone());
AFTER:
for args in [
serde_json::json!({"thinking": false}),
serde_json::json!({"enable_thinking": false}),
serde_json::json!({"thinking_mode": "chat"}),
] {
for choice in &choices {
let mut request = minimax_m2_request(args.clone(), choice.clone());normalize_thinking_arg writes both bools, so both keys reach the same template branch. {"thinking_mode": "chat"} stays a string, the one case where the renderer's 'chat' branch and is_reasoning_disabled_by_request must agree on their own, so it is worth a third row ({key} becomes {args} in the messages below).
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn minimax_m2_disabled_named_json_returns_tool_call() { |
There was a problem hiding this comment.
async fn minimax_m2_disabled_named_json_returns_tool_call() {
// Bare JSON becomes a named tool call with thinking disabled,
// both whole and fragmented across deltas.This test never renders a prompt, so it passes without frontend-crates#327 (CI on renderer 6.0.0 passed it) and covers postprocessing rather than the template fix. Would it sit better next to tool_choice_minimax_m2_required_thinking_disabled_keeps_tool_xml in tests/postprocessor_parsing_stream.rs?
| assert!(OpenAIPreprocessor::is_reasoning_disabled_by_request( | ||
| Some("minimax_m2"), | ||
| request.chat_template_args.as_ref(), | ||
| )); |
There was a problem hiding this comment.
assert!(OpenAIPreprocessor::is_reasoning_disabled_by_request(
Some("minimax_m2"),
request.chat_template_args.as_ref(),
));Follow-up idea: this gate reads only request args, so a template that ignores the toggle (a custom MiniMax template, stock DeepSeek-R1) still leaks reasoning into content with thinking disabled. Keeping the parser on while the rendered prompt leaves <think> open would cover them; not for this PR, happy to leave it here.
This branch has not been deployed
2 participants
Overview:
MiniMax M2.7 requests with thinking disabled could return reasoning text, a stray
</think>, and tool arguments as plain content instead of a tool call. The stock template opened a reasoning block while Dynamo disabled its reasoning parser.Add regression tests for the template correction in frontend-crates #327. The current Dynamo diff contains tests only; adoption of the corrected renderer release remains pending.
Details:
thinking: falseorenable_thinking: falsecloses the prompt, disables frontend reasoning parsing, and forwards noreasoning_endedoverride. Named and omitted tool choices exercise guided-JSON selection.reasoning_ended: falsebehavior.finish_reason: tool_calls. This verifies postprocessing, not backend generation.Where should the reviewer start?
Start with the
minimax_m2_*tests inlib/llm/src/preprocessor.rs. They cover disabled/default/enabled thinking through preprocessing and named-tool postprocessing with synthetic output. The template fix is in the companion frontend-crates PR #327.Related Issues
🚫 This PR is NOT linked to an issue:
Depends on ai-dynamo/frontend-crates#327.
Summary by CodeRabbit