Skip to content

[BugFix] Fix minimax m2 model rotary_dim#30384

Merged
vllm-bot merged 1 commit intovllm-project:mainfrom
rogeryoungh:m2-fix-rotary
Dec 10, 2025
Merged

[BugFix] Fix minimax m2 model rotary_dim#30384
vllm-bot merged 1 commit intovllm-project:mainfrom
rogeryoungh:m2-fix-rotary

Conversation

@rogeryoungh
Copy link
Contributor

@rogeryoungh rogeryoungh commented Dec 10, 2025

Purpose

After #29966, get_rope always reads partial_rotary_factor from the configuration and performs the multiplication again, even if rotary_dim has already been scaled. This leads to the factor being applied repeatedly, incorrectly reducing the effective rotational dimension.

Another way to fix #30349.

Test Result

lm_eval --model local-completions \
    --model_args base_url=http://localhost:10086/v1/completions,tokenizer=/model,model=/model \
    --tasks gsm8k_cot  \
    --batch_size 128 \
    --num_fewshot 5 

local-completions (base_url=http://localhost:10086/v1/completions,tokenizer=/model,model=/model), gen_kwargs: (None), limit: None, num_fewshot: 5, batch_size: 128
|  Tasks  |Version|     Filter     |n-shot|  Metric   |   |Value |   |Stderr|
|---------|------:|----------------|-----:|-----------|---|-----:|---|-----:|
|gsm8k_cot|      3|flexible-extract|     5|exact_match||0.9219|±  |0.0074|
|         |       |strict-match    |     5|exact_match||0.9098|±  |0.0079|

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: xuebi <xuebi@minimaxi.com>
@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly addresses a bug in the minimax_m2 model where the rotary dimension was being scaled twice. The issue stemmed from passing an already-scaled rotary_dim to the get_rope function, which would then apply the partial_rotary_factor again. The proposed fix of passing self.head_dim as the rotary_dim to get_rope is the correct approach, as it provides the unscaled base dimension, allowing get_rope to perform the scaling correctly. The change is logical, minimal, and effectively resolves the described problem.

@jeejeelee jeejeelee added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 10, 2025
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the fix

@vllm-bot vllm-bot merged commit d017bce into vllm-project:main Dec 10, 2025
50 of 56 checks passed
Majid-Taheri pushed a commit to Majid-Taheri/vllm that referenced this pull request Dec 23, 2025
Signed-off-by: xuebi <xuebi@minimaxi.com>
Co-authored-by: xuebi <xuebi@minimaxi.com>
Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
dsuhinin pushed a commit to dsuhinin/vllm that referenced this pull request Jan 21, 2026
Signed-off-by: xuebi <xuebi@minimaxi.com>
Co-authored-by: xuebi <xuebi@minimaxi.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants