Skip to content

Fix MiniMax M3 partial 3D vision rotary embeddings - #49164

Merged
vasqu merged 7 commits into
huggingface:mainfrom
cuichenx:chcui/tess/fix-minimax-m3-vision-3d-rope
Sep 30, 2026
Merged

vasqu merged 7 commits into
huggingface:mainfrom
cuichenx:chcui/tess/fix-minimax-m3-vision-3d-rope

Conversation

@cuichenx

@cuichenx cuichenx commented Sep 28, 2026 •

Copy link
Copy Markdown

CPU CI GPU run-slow

Restore MiniMax M3's partial 3D vision RoPE after #48105. Comparing Transformers 5.16.1 and 5.17.0 with identical weights and inputs exposed a change in the mathematical rotation.

Evidence

For head dimension 80 and base θ, define ω_m[j] = θ^(-j/m), j = 0,…,m−1. The angles before duplication are:

5.16.1 5.17.0
Angles [t·ω_13, h·ω_13, w·ω_13] [t·ω_20, h·ω_20]
Rotated dimensions 78; two unchanged 80

The caller supplies [T,H,W], but inherited 2D recomposition selects only columns 0 and 1, dropping W. Frequency spacing and rotation pairings also change. At (t,h,w)=(0,0,1), 5.17 applies the identity while 5.16 rotates the width band—even in exact arithmetic.

In a small one-layer vision-model comparison, the maximum absolute output difference was 1.2683868e-4; restoring the 5.16 3D calculation reduced it to 0.0. This is a controlled reproduction, not a production quality benchmark.

Fix

Preserve #48105's modular inheritance, forward(x, position_ids) interface, and recomposition inside the rotary module. Override MiniMax's frequency initialization and recomposition to retain its three axes and partial rotation; regenerate the modeling file from modular source.

Also preserve 5.16's FP32-per-forward frequency calculation: .half()/.bfloat16() rounds inherited buffers, and casting them back to FP32 cannot recover their precision. This is separate from the geometric error above. Performance was not benchmarked.

Validation

  • PYTHONPATH=src python -m pytest tests/models/minimax_m3_vl/test_vision_rotary.py -q: 4 passed, covering head dimensions 8/80, all three axes, the unchanged tail, and BF16/FP16 conversion. Geometry tests fail before the fix; precision tests fail with geometry alone corrected.
  • make style: all five checks passed in PR mode. Independent review passed. Full-suite and production-model GPU testing were not run.

AI-assisted implementation and investigation; independently reproduced, with no overlapping fix found and no prior issue coordination.

Signed-off-by: Chen Cui <chcui@nvidia.com>
Signed-off-by: Chen Cui <chcui@nvidia.com>

@vasqu vasqu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@zucchini-nlp if you could take a last look. Refactored it to be as close as possible to what we are used to

NOTE: The dtype stuff is not necessary. If someone intentionally moves something with .to(...), then that is on the user not on us. We keep the buffer in fp32 unless someone manually does that movement

@vasqu

vasqu commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

@cuichenx I will merge the current version as it should fix your numerical issue. The dtype thing is explained in #49164 (review)

Lmk if there is still some discrepancy and we will patch it (we release today)

@vasqu
vasqu enabled auto-merge September 30, 2026 15:22
@vasqu
vasqu disabled auto-merge September 30, 2026 15:23
@github-actions

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: minimax_m3_vl

@vasqu
vasqu enabled auto-merge September 30, 2026 15:28
@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 36733929354:2
Result: success | Jobs: 5 | Tests: 365 | Failures: 0 | Duration: 5m 17s

@vasqu
vasqu added this pull request to the merge queue Sep 30, 2026
@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@vasqu
vasqu removed this pull request from the merge queue due to a manual request Sep 30, 2026
@vasqu
vasqu merged commit 4fcb1ff into huggingface:main Sep 30, 2026
39 checks passed
@cuichenx

Copy link
Copy Markdown
Author

@vasqu Sorry missed the earlier message. Yes, the merged implementation fixes the discrepancy issue. Thanks for helping and thanks @zucchini-nlp for your reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants