[BestJuly]

What

Migrate the examples/multimodal_dev Qwen3.5-VL language decoder from
GPTModel to HybridModel, ahead of the deprecation of the GPTModel code path.

Why

The Qwen3.5-VL decoder is a hybrid GatedDeltaNet / full-attention stack. On the
GPT path that structure was expressed implicitly through
--experimental-attention-variant gated_delta_net plus
--linear-attention-freq 4, with the attention/MLP ordering baked into
TransformerLayer. HybridModel expresses it explicitly through
--hybrid-layer-pattern, which is where this model family belongs long term.

Changes

Model wiring (examples/multimodal_dev)

• MultimodalModel / Qwen35VLModel now take hybrid_stack_spec and
  hybrid_layer_pattern instead of a transformer_layer_spec and an
  mtp_block_spec. get_qwen35_vl_language_spec is removed.
• Each historical Qwen block becomes two hybrid layers: a GatedDeltaNet (G)
  or full-attention (*) layer followed by a dense (-) or MoE (E) MLP
  layer. Three GDN blocks then one attention block reproduces the
  --linear-attention-freq 4 cadence exactly.
• MTP depths replicate the last block, matching get_gpt_mtp_block_spec, which
  derived the MTP spec from the final decoder layer.
• Variant configs are converted from block counts to hybrid layer counts,
  because HybridModel counts one layer per pattern symbol.

mRoPE support in HybridModel (megatron/core/models/hybrid/hybrid_model.py)

• Build a MultimodalRotaryEmbedding when position_embedding_type == 'mrope'
  and compute sectioned (optionally raw, fused-kernel) freqs in forward,
  mirroring GPTModel line for line, including the
  apply_rope_fusion / rotary_interleaved / fused_single_qkv_rope /
  inference conditions and the flash-decode NotImplementedError.
• Widen the HybridModelConfig.position_embedding_type literal accordingly.

GatedDeltaNet (megatron/core/ssm/gated_delta_net.py)

• Import fla.ops.cp.build_cp_context separately from the rest of FLA so an FLA
  build without the CP extension still enables GatedDeltaNet, and raise a
  descriptive error only when chunkwise CP is actually requested.

Launcher (scripts/run_qwen35_vl.sh)

• Derive the pattern from NUM_LAYERS, NUM_EXPERTS and the new
  MTP_NUM_LAYERS knob; pass the MTP arguments only when MTP is enabled.
• NUM_LAYERS need not be a multiple of 4: trailing blocks stay GatedDeltaNet,
  which is what --linear-attention-freq 4 produced, so shallow proxy runs keep
  working.

Tests

• examples/multimodal_dev/tests/test_hybrid_migration.py — factory wiring plus
  structural equivalence between the hybrid pattern and the former GPT layout.
  The key check cross-validates validate_segment_layers (hybrid) against
  get_linear_attention_pattern (GPT) so a semantic drift on either side fails.
• tests/unit_tests/models/test_hybrid_model.py — HybridModel constructor with
  position_embedding_type='mrope'.
• examples/multimodal_dev/tests/test_cp_thd_correctness.py — ported to the
  hybrid stack spec.

Checkpoint compatibility

The decoder checkpoint layout changes: one GPT decoder layer is split across two
hybrid layer indices, the final-norm key changes, and hybrid MTP uses a nested
HybridStack. An existing GPTModel-format Qwen3.5-VL checkpoint is therefore not
directly loadable. tools/checkpoint/gpt_hybrid_conversion.py does not cover
this case either — it explicitly rejects GDN (G) and MTP checkpoints. This is
documented in examples/multimodal_dev/README.md.

Scope of risk

The megatron/core changes are additive: the mRoPE branches only run when
position_embedding_type == 'mrope', which no existing HybridModel
configuration uses, and the FLA import change only alters behavior on builds
that lack fla.ops.cp. Everything else is confined to examples/multimodal_dev.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[QiZhangNV]

While reviewing this migration, I noticed that the configured position embedding type does not match the model that is actually constructed.

run_qwen35_vl.sh sets --position-embedding-type rope, while Qwen35VLModel ignores that configuration and unconditionally constructs the decoder with position_embedding_type="mrope". The runtime model therefore uses MRoPE, but the parsed arguments and checkpoint metadata describe ordinary RoPE. Configuration-driven tooling such as checkpoint converters, exporters, or generic model builders could consequently reconstruct the wrong decoder architecture.

This cannot be fixed by changing the recipe to --position-embedding-type mrope alone. The earlier MTP validation in arguments.py only accepts rope and none, so it rejects mrope, while the later validation explicitly lists mrope as supported. Please consolidate these two validation paths and make the recipe, model construction, and checkpoint metadata consistently describe the decoder as using mrope.

[hxbai]

Should we pass cp_group to mrope like other PEs?

[BestJuly]

Good catch. This was a bug and have fixed and is covered by new tests.

[hxbai]

I think is_hybrid_model also need to be set.

[BestJuly]

Agreed and have set this, and also deleted linear_attention_freq to match the logic.

[hxbai]

/claude strict-review

[claude]

[CRITICAL Correctness]

What: MultimodalRotaryEmbedding is constructed without cp_group, unlike the two sibling branches immediately above — the rope branch (RotaryEmbedding(..., cp_group=self.pg_collection.cp)) and the yarn branch (YarnRotaryEmbedding(..., cp_group=self.pg_collection.cp)) both pass it. MultimodalRotaryEmbedding.__init__ therefore takes its fallback:

self.cp_group = (
    cp_group
    if cp_group is not None
    else parallel_state.get_context_parallel_group(check_initialized=False)
)

Why it matters: two separate problems.

1. Correctness under a custom pg_collection. HybridModel accepts pg_collection precisely so a caller can supply process groups that are not the parallel_state globals. Here the CP group is silently taken from the global MPU state instead. MultimodalRotaryEmbedding.forward uses self.cp_group for get_pos_emb_on_this_cp_rank(...) whenever the per-call cp_group is None — and this model's forward passes cp_group=packed_seq_params.cp_group if packed_seq_params is not None else None, so the non-packed path (the common BSHD training path) always relies on self.cp_group. If the injected pg_collection.cp differs from the global CP group, the mRoPE frequencies get zigzag-sliced for the wrong rank — silently wrong positions, not a crash. check_initialized=False also means an uninitialized MPU yields None, which turns CP slicing off entirely rather than erroring.

2. Repo guidance. Per CLAUDE.md, new megatron/core production code should not introduce reads of global parallel_state process groups; it should thread the group through from the caller. This branch introduces one indirectly, and the correct group is already available two lines away as self.pg_collection.cp.

Note this is not theoretical for this PR: examples/multimodal_dev/models/base.py::_thd_mrope_no_cp_override mutates self.language_model.rotary_pos_emb.cp_group at runtime, so whatever this attribute is initialized to is load-bearing on the very path the PR is enabling.

(The GPT copy this was ported from has the same omission, but that is a pre-existing bug in a different file — copying it into hybrid_model.py, where the adjacent branches already do the right thing, makes the inconsistency worse.)

Fix: pass the group explicitly.

Suggested change

	self.rotary_pos_emb = MultimodalRotaryEmbedding(
	kv_channels=self.config.kv_channels,
	rotary_percent=rotary_percent,
	rotary_interleaved=self.config.rotary_interleaved,
	seq_len_interpolation_factor=seq_len_interpolation_factor,
	rotary_base=rotary_base,
	interleaved_mrope=self.config.mrope_interleaved,
	)
	self.rotary_pos_emb = MultimodalRotaryEmbedding(
	kv_channels=self.config.kv_channels,
	rotary_percent=rotary_percent,
	rotary_interleaved=self.config.rotary_interleaved,
	seq_len_interpolation_factor=seq_len_interpolation_factor,
	rotary_base=rotary_base,
	interleaved_mrope=self.config.mrope_interleaved,
	cp_group=self.pg_collection.cp,
	)

[claude]

[IMPORTANT Compatibility]

What: deleting the early assertion does more than un-block mrope. The removed check allowed only {rope, none}; the authoritative downstream check (line 2141) allows {learned_absolute, rope, mrope, none}. So this diff simultaneously legalizes two previously-rejected combinations:

• --mtp-num-layers N --position-embedding-type mrope — intended, and the point of the PR.
• --mtp-num-layers N --position-embedding-type learned_absolute — a silent side effect nowhere mentioned in the PR description or the new comment.

Why it matters: learned_absolute + MTP was hard-rejected on every prior release of this file, so no user has ever run it and it has no functional-test coverage. MTP rolls the sequence forward per prediction depth (prepare_mtp_sequence_roll_context); with RoPE that is a pure position_ids shift, but with learned_absolute the position embedding is added inside LanguageModelEmbedding from the unshifted embedding-side positions, so whether the MTP depth-k branch sees correctly-offset absolute positions is an untested question. Turning it from "asserts immediately" into "trains and produces a number" converts a loud failure into a possible silent one. It is also strictly outside the PR's stated scope ("make the Qwen3.5-VL recipe describe the model it builds").

Fix: keep the scope of this PR to mrope. Either narrow the downstream list to the union that is actually intended/tested, or gate it explicitly. Concretely, at line 2141:

if args.mtp_num_layers:
    # MTP is compatible with position embedding types that use position_ids.
    # 'learned_absolute' is excluded: MTP's sequence roll shifts position_ids,
    # but learned_absolute adds embeddings from the unshifted embedding-side
    # positions, and that combination has no test coverage.
    supported_position_types = ["rope", "mrope", "none"]
    assert args.position_embedding_type in supported_position_types, (...)

If learned_absolute + MTP is meant to be enabled here, say so in the commit message and add a test — but that belongs in its own PR.

[claude]

[IMPORTANT Correctness]

What: the PR hard-enforces one recipe/model invariant but leaves the neighbouring one on the honour system. --position-embedding-type now raises here (and again in Qwen35VLModel.__init__) if it disagrees with the model, but --mrope-section — which the PR newly makes mandatory on the CLI — is only documented as needing to match:

• run_qwen35_vl.sh:374 — "--mrope-section must match MROPE_SECTION in ..."
• README.md:61 — "also match MROPE_SECTION in models/qwen35_vl/configuration.py."

and post_language_config deliberately declines to set it ("overriding it would let the recipe and the constructed model disagree"). Nothing checks it. The only assertion in the stack is validate_args:1702, which merely requires it to be non-None.

Why it matters: MROPE_SECTION = [11, 11, 10] is tied to ROTARY_PERCENT = 0.25 and kv_channels: sum(MROPE_SECTION) must equal int(kv_channels * ROTARY_PERCENT) // 2. The new test_mrope_section_matches_rotary_dimension asserts exactly that — for the constant, not for the CLI value that actually reaches the model. A user who writes --mrope-section 12 10 10 (right sum, wrong split) gets a model that trains to a plausible-looking loss with the T/H/W channel boundaries in the wrong place — a silent accuracy bug, not a crash. A wrong sum is worse still: _apply_interleaved_mrope indexes freqs[..., slice(offset, mrope_section[dim_idx] * 3, 3)], which silently under- or over-fills channels rather than raising.

This is exactly the class of recipe/model disagreement the PR was written to eliminate, so leaving it unenforced is inconsistent with the change's own stated goal.

Fix: validate it next to the pattern check, mirroring the hybrid_layer_pattern guard:

from examples.multimodal_dev.models.qwen35_vl.configuration import MROPE_SECTION, VISION_KWARGS

...

if list(language_config.mrope_section or []) != list(MROPE_SECTION):
    raise ValueError(
        f"Qwen3.5-VL requires --mrope-section {' '.join(map(str, MROPE_SECTION))}, "
        f"but got {language_config.mrope_section}. The section split is tied to "
        f"kv_channels and ROTARY_PERCENT; a mismatch silently misplaces the "
        f"T/H/W channel boundaries."
    )

That turns a documented convention into a checked one, and lets the docs stop carrying the burden.

[claude]

[SUGGESTION Simplification]

What: now that num_layers is the hybrid layer count, the linear_attention_freq=4 still sitting in this same dict (line 343) is both dead and internally inconsistent with it. Under the HybridModel path that field is never consumed:

• TransformerBlock's get_linear_attention_pattern consumer is not reached — HybridModel builds a HybridStack from hybrid_layer_pattern.
• transformer_config.py:1768 explicitly skips the linear_attention_freq is not None assertion when is_hybrid_model.
• The FLOPs path takes the hybrid branch (training.py:1281, via get_hybrid_layer_counts), not the linear_attention_freq branch.

So the value is inert — but a reader who trusts it computes the wrong layout, because interpreted against the now-doubled num_layers it reads as "every 4th layer", i.e. half the intended full-attention count.

Why it matters: the point of this PR is that the config should describe the model that gets built. Leaving a stale sibling field next to the one whose units you just changed re-creates, inside a single dict, the recipe/model disagreement the PR removes elsewhere. The next person adding a variant has no way to tell which of the two fields is authoritative.

Fix: drop it and note where the layout now comes from:

        # Hybrid attention (GatedDeltaNet). The GDN/full-attention layout comes
        # from --hybrid-layer-pattern, not linear_attention_freq: HybridModel
        # builds its stack from the pattern and never reads that field.
        experimental_attention_variant="gated_delta_net",
        linear_conv_kernel_dim=4,

If it has to stay for a non-hybrid consumer, keep it but comment that it is ignored on the hybrid path and is in block units while num_layers here is in layer units.

[claude]

[SUGGESTION Simplification]

What: two small clarity issues in the expansion loop.

1. ATTN_LAYER_SYMBOL="${ATTN_CADENCE:0:1}" at line 468 is dead in every real configuration — the loop unconditionally reassigns it on its first iteration. It only survives when NUM_LAYERS=0, in which case the MTP loop would emit MTP blocks for a zero-layer decoder anyway.

2. The MTP loop consumes ATTN_LAYER_SYMBOL as leaked from the decoder loop's last iteration. That happens to be correct — GPT's get_gpt_mtp_block_spec does copy.copy(spec.layer_specs[-1]), so MTP replicating the final block is the right parity — but the code expresses it as an implicit side effect of loop-variable scoping rather than as the intent. If someone later reorders the loops, adds a trailing symbol, or wraps the decoder loop in a conditional, the MTP symbol changes silently and the pattern still parses.

Why it matters: the pattern string is the sole source of truth for the built architecture now that --num-layers and --linear-attention-freq are gone. A silently-wrong MTP symbol produces a model that trains fine but has a GDN MTP head where a full-attention one was intended — no error, just a different architecture from the one the comment above claims.

Fix: drop the dead pre-assignment and name the invariant explicitly.

HYBRID_LAYER_PATTERN=""
for ((block_idx = 0; block_idx < NUM_LAYERS; block_idx++)); do
    ATTN_LAYER_SYMBOL="${ATTN_CADENCE:block_idx % ${#ATTN_CADENCE}:1}"
    HYBRID_LAYER_PATTERN+="${ATTN_LAYER_SYMBOL}${MLP_LAYER_SYMBOL}"
done
# Each MTP depth replicates the final decoder block, matching the GPT path's
# copy.copy(spec.layer_specs[-1]).
LAST_BLOCK_ATTN_SYMBOL="${ATTN_CADENCE:(NUM_LAYERS - 1) % ${#ATTN_CADENCE}:1}"
for ((mtp_depth = 0; mtp_depth < MTP_NUM_LAYERS; mtp_depth++)); do
    HYBRID_LAYER_PATTERN+="/${LAST_BLOCK_ATTN_SYMBOL}${MLP_LAYER_SYMBOL}"
done

[FDecaYed]

Re-reviewed the current head e992794. The CP-group injection and architecture-specific mrope_section validation are correctly implemented with focused coverage, and the exact-head required CI checks pass. The failing codecov/patch status is advisory rather than a required dev check; its misses are concentrated in optional fused/inference and missing-extension defensive branches, so I do not consider it a merge blocker.