Skip to content

[model] fix: improve apply_rope_fusion assert message for Qwen3-VL#3328

Open
yaoyu-33 wants to merge 1 commit intomainfrom
fix/qwen3vl-rope-fusion-assert-3296
Open

[model] fix: improve apply_rope_fusion assert message for Qwen3-VL#3328
yaoyu-33 wants to merge 1 commit intomainfrom
fix/qwen3vl-rope-fusion-assert-3296

Conversation

@yaoyu-33
Copy link
Copy Markdown
Contributor

@yaoyu-33 yaoyu-33 commented Apr 14, 2026

Summary

  • Replaces bare assert not config.apply_rope_fusion in rope.py with a descriptive assertion message and detailed comments explaining why fused RoPE is unsupported for Qwen3-VL / Qwen3.5-VL
  • Verified on cluster that even removing the assert and forcing apply_rope_fusion=True never actually enables TE fused kernels due to three upstream guards:
    1. validate_rope_fusion_compatibility() blocks position_embedding_type="mrope"
    2. Qwen3VLSelfAttention calls apply_rotary_pos_emb_absolute() directly (bypasses fused dispatch)
    3. Per-token absolute freq format (seq_len, bs, 1, 2*dim) is incompatible with TE's expected (max_seqlen, 1, 1, 2*dim)

Closes #3296

Test plan

  • Existing unit tests pass (tests/unit_tests/models/qwen_vl/)
  • No functional change — assert behavior is identical, only the error message improves

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced configuration validation error messages to help users better understand which settings are incompatible and why.

…3296)

The bare `assert not config.apply_rope_fusion` gave no context on why
fusion is unsupported. Add a detailed comment and assertion message
explaining the three layers that prevent fused RoPE for Qwen3-VL /
Qwen3.5-VL: incompatible absolute freq format, custom attention bypass,
and upstream validation guard.

Closes #3296

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot bot commented Apr 14, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yaoyu-33 yaoyu-33 added the docs-only With great power comes great responsibility. label Apr 14, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 808e89a9-9e93-4c6c-a1ee-e1a59e4ede6f

📥 Commits

Reviewing files that changed from the base of the PR and between 9b38f65 and d91435c.

📒 Files selected for processing (1)
  • src/megatron/bridge/models/qwen_vl/modelling_qwen3_vl/rope.py

📝 Walkthrough

Walkthrough

A bare assertion preventing apply_rope_fusion configuration in Qwen3-VL and Qwen3.5-VL models was replaced with a guarded assertion that provides a detailed error message explaining why the feature must remain disabled.

Changes

Cohort / File(s) Summary
Error Message Enhancement
src/megatron/bridge/models/qwen_vl/modelling_qwen3_vl/rope.py
Added explicit, descriptive error message to assertion that validates apply_rope_fusion is disabled for Qwen3-VL and Qwen3.5-VL models, replacing bare assert statement.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: improving the assert message for apply_rope_fusion in Qwen3-VL, which is exactly what the changeset accomplishes.
Linked Issues check ✅ Passed The PR adequately addresses issue #3296 by replacing the bare assert with a descriptive message and detailed comments explaining why apply_rope_fusion is unsupported, providing the clarification requested.
Out of Scope Changes check ✅ Passed All changes are scoped to improving the assertion message and documentation in rope.py as specified by the linked issue; no unrelated changes are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Test Results For Major Changes ✅ Passed Minor change improving assertion error message with no functional impact; existing unit tests documented to pass.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/qwen3vl-rope-fusion-assert-3296

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Labels

docs-only With great power comes great responsibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] qwen35 apply_rope_fusion

1 participant