fix(quantization): Qwen3-VL MoE PTQ on transformers>=5.12 (NVBug 6518551) - #2046
Open
Edwardf0t1 wants to merge 1 commit into
Open
fix(quantization): Qwen3-VL MoE PTQ on transformers>=5.12 (NVBug 6518551)#2046Edwardf0t1 wants to merge 1 commit into
Edwardf0t1 wants to merge 1 commit into
Conversation
…551)
transformers 5.12 moved `Qwen3VLMoeTextExperts` onto the standard
`@use_experts_implementation` fused layout: `hidden_size`/`expert_dim` were
renamed to `hidden_dim`/`intermediate_dim`, `gate_up_proj` was transposed to
(num_experts, 2*intermediate_dim, hidden_dim), and the forward now calls
`F.linear` twice per expert instead of using `torch.bmm`.
The legacy `_QuantQwen3VLMoeTextExperts` wrapper only understands the older
layout, yet stayed statically registered. Since `register_fused_experts_on_the_fly`
skips any type already in `QuantModuleRegistry`, that registration shadowed the
generic `_QuantFusedExperts` that already handles the new layout correctly, and
conversion crashed with:
AttributeError: 'QuantQwen3VLMoeTextExperts' object has no attribute 'hidden_size'
Detect the new layout via the `_apply_gate` class attribute set by the
`@use_experts_implementation` decorator and skip the static registration, letting
on-the-fly detection claim it. The legacy wrapper must stay registered on
transformers<5.12: that layout is structurally indistinguishable from a generic
fused-experts module, but its `torch.bmm`-based forward cannot be intercepted by
the generic wrapper, which would silently quantize nothing.
Add regression tests exercising the real transformers module on both layouts:
registration matches the installed layout, conversion is numerically transparent,
and calibration actually collects amax. All three reproduce the reported
AttributeError without the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe Qwen3-VL MoE quantization plugin now distinguishes legacy expert layouts from Transformers 5.12+ fused layouts. Tests cover registration, conversion equivalence, and FP8 calibration. ChangesQwen3-VL MoE compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 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 `@tests/unit/torch/quantization/plugins/test_fused_experts.py`:
- Around line 1429-1493: Add a brief justification comment before each local
transformers import in _make_qwen3_vl_moe_experts, _qwen3_vl_moe_is_new_layout,
and _experts_type, identifying transformers as an optional dependency and/or
explaining the version-dependent module path; keep the imports local unless
moving them to module scope is appropriate.
🪄 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: 96d93cc8-5a43-4a2d-9759-012150fdfc22
📒 Files selected for processing (3)
CHANGELOG.rstmodelopt/torch/quantization/plugins/huggingface.pytests/unit/torch/quantization/plugins/test_fused_experts.py
Comment on lines
+1429
to
+1493
| def _make_qwen3_vl_moe_experts(): | ||
| """Build a tiny real ``Qwen3VLMoeTextExperts`` with initialized weights.""" | ||
| from transformers.models.qwen3_vl_moe.configuration_qwen3_vl_moe import Qwen3VLMoeTextConfig | ||
| from transformers.models.qwen3_vl_moe.modeling_qwen3_vl_moe import Qwen3VLMoeTextExperts | ||
|
|
||
| config = Qwen3VLMoeTextConfig( | ||
| hidden_size=QWEN_HIDDEN_DIM, | ||
| intermediate_size=QWEN_HIDDEN_DIM, | ||
| moe_intermediate_size=QWEN_INTERMEDIATE_DIM, | ||
| num_experts=QWEN_NUM_EXPERTS, | ||
| num_experts_per_tok=QWEN_TOP_K, | ||
| num_hidden_layers=1, | ||
| num_attention_heads=2, | ||
| num_key_value_heads=2, | ||
| vocab_size=128, | ||
| ) | ||
| # The fused forward of transformers>=5.12 dispatches on this; ``eager`` is the only | ||
| # backend that routes through ``F.linear`` and therefore through the quantizer hooks. | ||
| config._experts_implementation = "eager" | ||
| experts = Qwen3VLMoeTextExperts(config) | ||
| # Weights are created with ``torch.empty``; fill them so comparisons are meaningful. | ||
| torch.manual_seed(0) | ||
| with torch.no_grad(): | ||
| for param in experts.parameters(): | ||
| param.normal_(std=0.02) | ||
| return experts | ||
|
|
||
|
|
||
| def _qwen3_vl_moe_is_new_layout(): | ||
| """True when transformers>=5.12 moved the experts onto the generic fused layout.""" | ||
| from transformers.models.qwen3_vl_moe.modeling_qwen3_vl_moe import Qwen3VLMoeTextExperts | ||
|
|
||
| return hasattr(Qwen3VLMoeTextExperts, "_apply_gate") | ||
|
|
||
|
|
||
| def _qwen3_vl_moe_forward_args(): | ||
| """Routing inputs for the installed layout, sized so every expert is hit. | ||
|
|
||
| The two layouts disagree on both argument order and routing-weight shape, and the | ||
| pre-5.12 eval-mode forward computes a dense weighted sum over all experts. Zeroing the | ||
| weights outside the top-k keeps that dense path equal to the sparse per-expert loop. | ||
| """ | ||
| seq_len = QWEN_NUM_EXPERTS // QWEN_TOP_K | ||
| torch.manual_seed(0) | ||
| hidden_states = torch.randn(seq_len, QWEN_HIDDEN_DIM) | ||
| router_indices = torch.arange(QWEN_NUM_EXPERTS, dtype=torch.long).reshape(seq_len, QWEN_TOP_K) | ||
| top_k_weights = torch.softmax(torch.randn(seq_len, QWEN_TOP_K), dim=-1) | ||
|
|
||
| if _qwen3_vl_moe_is_new_layout(): | ||
| return hidden_states, router_indices, top_k_weights | ||
|
|
||
| routing_weights = torch.zeros(seq_len, QWEN_NUM_EXPERTS) | ||
| routing_weights.scatter_(1, router_indices, top_k_weights) | ||
| return hidden_states.unsqueeze(0), routing_weights, router_indices | ||
|
|
||
|
|
||
| class TestQwen3VLMoeTextExperts: | ||
| """The registered wrapper must match the installed transformers layout (nvbug 6518551).""" | ||
|
|
||
| @staticmethod | ||
| def _experts_type(): | ||
| from transformers.models.qwen3_vl_moe.modeling_qwen3_vl_moe import Qwen3VLMoeTextExperts | ||
|
|
||
| return Qwen3VLMoeTextExperts | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a justification comment for the local transformers imports.
_make_qwen3_vl_moe_experts (lines 1431-1432), _qwen3_vl_moe_is_new_layout (line 1459), and _experts_type (line 1490) each import from transformers.models.qwen3_vl_moe... inside the function body. None of these local imports carries a comment naming the reason.
Move these imports to module scope, or add a brief comment justifying the local import (e.g., noting that the module path changed across transformers versions or that transformers is an optional dependency of the plugin under test).
As per path instructions for tests/**/*.py: "Imports inside functions or test methods without explicit justification... The only acceptable in-function imports are for circular imports or optional dependencies... and those should carry a brief comment naming the reason."
🤖 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 `@tests/unit/torch/quantization/plugins/test_fused_experts.py` around lines
1429 - 1493, Add a brief justification comment before each local transformers
import in _make_qwen3_vl_moe_experts, _qwen3_vl_moe_is_new_layout, and
_experts_type, identifying transformers as an optional dependency and/or
explaining the version-dependent module path; keep the imports local unless
moving them to module scope is appropriate.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2046 +/- ##
==========================================
- Coverage 77.44% 69.42% -8.03%
==========================================
Files 522 522
Lines 58452 58452
==========================================
- Hits 45271 40579 -4692
- Misses 13181 17873 +4692
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cjluo-nv
reviewed
Aug 4, 2026
cjluo-nv
left a comment
cjluo-nv
left a comment
Collaborator
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Small, well-scoped fix: the static registration of the legacy _QuantQwen3VLMoeTextExperts wrapper is now skipped when the class carries _apply_gate (transformers>=5.12's @use_experts_implementation fused layout), so register_fused_experts_on_the_fly claims the module with the generic _QuantFusedExperts. That matches how Mixtral/Qwen3-MoE/MiniMax already flow through _fused_experts_wrapper_class, i.e. no new abstraction is introduced, and CHANGELOG + tests are included. No licensing surface touched. A few things I'd like the owner to eyeball before this lands:
- Detection hinges on an undocumented transformers internal. The whole fix is
not hasattr(Qwen3VLMoeTextExperts, "_apply_gate"). If that private attribute is renamed/removed in a later 5.x while the fused layout stays, the legacy wrapper silently re-registers and users get the same confusingAttributeError: ... has no attribute 'hidden_size'. The file already has a version constant precedent (TRANSFORMERS_VERSION_GE_5_0); consider OR-ing in a version check, and/or adding a guard in_QuantQwen3VLMoeTextExperts._setup(e.g. assert the legacygate_up_projorientation / absence of_apply_gate) so a future rename fails with an actionable message instead of the original bug. I can't verify from the repo that_apply_gateis set on the class in 5.12 — that rests entirely on the author's two-environment run. - Export path for the new layout is untested here. Coverage stops at calibration (
test_quantize_collects_amax). Because the wrapper changes, the exported per-expert naming for Qwen3-VL MoE now goes through_export_fused_experts(per-expert<idx>.gate_proj/...) on >=5.12 vs. the legacy wrapper's unrollednn.ModuleListnaming on <5.12 — i.e. the exported checkpoint layout differs by installed transformers version for the same model. That is probably the intended/mainline behavior (same as Mixtral), but someone with unified-export context should confirm no downstream naming regression, since there is no Qwen3-VL-specific handling inmodelopt/torch/export. - Tests hard-import
transformers.models.qwen3_vl_moe.pyproject.tomlallowstransformers>=4.56, where that module doesn't exist yet; the three new tests would error (not skip) on the supported floor. A module/class-levelpytest.importorskip("transformers.models.qwen3_vl_moe")would keep them hermetic. - Minor:
test_registration_matches_installed_layoutasserts global registry state (registered is None) for the real transformers class, so any other test in the session that quantizes a Qwen3-VL-MoE model on the fly without unregistering would make it fail spuriously. Low risk today, just noting the coupling.
cjluo-nv
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change: Bug fix
Fixes NVBug 6518551 / OMNIML-5581: PTQ of
Qwen3-VL-30B-A3B-Instructontransformers5.12.1 crashes duringmtq.quantize:Root cause. transformers 5.12 moved
Qwen3VLMoeTextExpertsonto the standard@use_experts_implementationfused layout:transformers<5.12transformers>=5.12hidden_size,expert_dimhidden_dim,intermediate_dimgate_up_proj(E, hidden, 2*expert_dim)(E, 2*intermediate_dim, hidden)torch.bmm/@F.lineartwice per expertThe legacy
_QuantQwen3VLMoeTextExpertswrapper only understands the older layout, yet stayed statically registered at import time. Becauseregister_fused_experts_on_the_flyskips any type already inQuantModuleRegistry, that stale registration shadowed the generic_QuantFusedExperts— which already handles the new layout correctly (Mixtral, Qwen3-MoE, Qwen3.5-MoE, MiniMax-M2/M3 all go through it).Fix. Detect the new layout via the
_apply_gateclass attribute that@use_experts_implementationsets, and skip the static registration so on-the-fly detection claims it with_QuantFusedExperts.The legacy wrapper must stay registered on
transformers<5.12: that layout is structurally indistinguishable from a generic fused-experts module (num_experts+ 3-Dgate_up_proj/down_proj), but itstorch.bmm-based forward is not intercepted by the generic wrapper, so simply deleting the registration would silently quantize nothing.pyproject.tomlsupportstransformers>=4.56,<5.13, so both layouts must keep working.Usage
Testing
New
TestQwen3VLMoeTextExpertsintests/unit/torch/quantization/plugins/test_fused_experts.pyexercises the real transformers module (not a synthetic stand-in) and adapts to the installed layout:test_registration_matches_installed_layout— new layout is left to on-the-fly detection and resolves to_QuantFusedExperts; old layout keeps the explicit registration.test_convert_and_forward_matches_reference— conversion succeeds and is numerically transparent before calibration.test_quantize_collects_amax— calibration actually reaches the experts rather than silently no-op'ing.Verified in two environments (there was no prior test coverage for quantized Qwen3-VL MoE experts):
transformers==4.57.6(old)transformers==5.12.1(new)AttributeErrortest_fused_experts.pytests/unit/torch/quantization/plugins/pre-commit run --files ...passes on all changed files.Before your PR is "Ready for review"
transformers<5.12keeps the legacy wrapper unchanged.CONTRIBUTING.md: N/AAdditional Information
Reported by Kenny Kang (SW-GPU) on GB200×4,
nvcr.io/nvidia/pytorch:26.05-py3, modelopt 0.46.0rc0. Committed for ModelOpt 0.46.0.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests