Repository navigation
Add KDA (Kimi Delta Attention) hybrid layers with GLM-style two-stage gates - #7054
Conversation
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
There was a problem hiding this comment.
🟢 Approval recommended
The logic change is small, follows existing get_fp8_disabled_context usage patterns in related indexer code, and the only feedback is minor comment wording accuracy.
Pull request overview
Adjusts DSA indexer precision behavior under hybrid FP8 training to reduce quantization noise in the top‑k index score path, improving train/inference alignment for sparse-attention selection.
Changes:
- Disable FP8 quantization for
linear_wkandlinear_weights_projduring module construction so their parameters stay in higher precision when FP8 params are enabled. - Disable FP8 quantization for the K-path (
linear_wk,k_norm, RoPE, optional activation rotation) andlinear_weights_projduring forward, while keeping the Q-path (linear_wq_b) quantized.
File summaries
| File | Description |
|---|---|
| megatron/core/transformer/experimental_attention_variant/dsa.py | Split DSA indexer Q vs K/weights-projection execution so index-score-critical ops run with FP8 disabled. |
Review details
Suppressed comments (1)
megatron/core/transformer/experimental_attention_variant/dsa.py:1401
- The updated comments call out “(FP8)” and “run in BF16”, but
get_fp8_disabled_contextonly disables TE quantization and doesn’t force a specific dtype. Rewording to “quantized” vs “FP8/FP4 disabled” avoids implying these paths are always FP8/BF16 regardless of the model’s base precision configuration.
# q linear and apply rope to q (FP8)
# =========================================
# [seqlen, batch, q_lora_rank] -> [seqlen, batch, index_n_heads * index_head_dim]
q, _ = self.linear_wq_b(qr)
# [seqlen, batch, index_n_heads * index_head_dim]
# -> [seqlen, batch, index_n_heads, index_head_dim]
q = q.reshape(seqlen, bsz, self.index_n_heads, self.index_head_dim)
q = self._apply_rope(q, rotary_pos_emb, mscale, cu_seqlens=cu_seqlens_q)
if self.config.dsa_indexer_rotate_activation:
q = rotate_activation(q)
# =========================================
# k linear, k_norm, rotate, and weights_proj run in BF16 (FP8 disabled).
# =========================================
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5bef51d to
be805e5
Compare
095d51c to
3fceb07
Compare
3fceb07 to
beb4be3
Compare
… k-pool indexer Points megatron-core at the head of NVIDIA/Megatron-LM#7054 ("GLM5.3 Flash (KDA + mHC + KPool DSA) support + FP8", HollowMan6) instead of implementing the k-pool indexer ourselves. That PR supersedes our #7427: it carries the same KDA and mHC work plus the pooled indexer, was opened two weeks earlier, is not a draft, and its config field names already match this provider (dsa_indexer_kpool, dsa_indexer_kpool_always_select_tail, mhc_norm_eps_inside_sqrt). Its pooling matches the HF/vLLM semantics independently reverse-engineered from vLLM's Triton kernel: a per-dimension softmax over each pool's slots of `gate_score + ape`, weighting the keys. It also handles the two parts that are easy to get wrong -- packed-sequence boundaries (_kpool_compress_keys_per_seg) and the context-parallel gather/reorder of the gate score. megatron-bridge stays on 8e5f0e13: bridge main needs hydra-core>=1.3.4 and SkyRL pins ==1.3.2, so only megatron-core moves, isolating the variable. That is safe because the core rev bridge main validates against (f6c33bde) is an ANCESTOR of the PR head (ahead 60, behind 0). Two SkyRL-side changes are required: - Map index_kpool_compress_ape / index_kpool_compress_gate. megatron-core only creates these when dsa_indexer_kpool > 1 and initializes them randomly (nn.init.normal_ on the gate), so a pooled run without these mappings would silently train against random pooling weights. They are bare nn.Parameters on DSAIndexer rather than module weights, so AutoMapping cannot infer a parallelism type ("Cannot determine parallelism type for module 'DSAIndexer'"); the indexer is duplicated across TP ranks, so they are ReplicatedMapping. - Relax the >dsa_indexer_topk guard to fire only for kpool <= 1, which is now the only regime megatron-core cannot handle. Note index_kpool is an attribute of DSAIndexer, not DSAttention, so the guard reads it from the config. Testing: upstream's own k-pool unit tests pass here (44), the new test_glm5_next_kpool.py passes (6), and the GLM-5.3-Flash 4-layer model-level parity row passes -- Megatron logprobs against vLLM plus the weight-sync round trip, with kpool=4 active and the pooling weights loaded from the checkpoint. All seven SkyRL GLM modules still import across 60 commits of megatron-core drift. Not yet covered: a >dsa_indexer_topk parity row. Below the budget every pool is selectable, so the existing row cannot distinguish correct pooling weights from wrong ones -- only sequences past 2048 exercise pool selection itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ementing the DSA k-pool indexer Lifts the dsa_indexer_topk (2048) ceiling on Megatron training. megatron-bridge stays pinned at 8e5f0e13 and SkyRL keeps its own glm5_next bridge package; only megatron-core moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
thanks for this PR @HollowMan6, also testing this branch out now for RL support on SkyRL |
|
@Phlip79 I validated GLM 5.3 flash RL with LoRA together with vLLM/Megatron-Bridge/verl using geo3k, you can find the convergence results at vllm-project/vllm#56327 (comment) |
|
/ok to test 22c4f37 |
|
/ok to test f8248c0 |
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
- Reject unsupported packed deterministic convolution paths - Preserve BF16 KDA parameters when FP8 initialization is enabled - Validate KDA recompute modules and head dimensions - Add focused KDA regression tests Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
Signed-off-by: Hollow Man <hollowman@opensuse.org>
kevalmorabia97
left a comment
There was a problem hiding this comment.
LGTM from the Model Optimizer side. Tested 92bd36ec (merged into #7521) and NVIDIA-NeMo/Megatron-Bridge#6044 in a NeMo 26.10 container on GB300: ModelOpt GPTQ PTQ, distillation with save + resume, and quantized HF export on a 4-layer GLM-5.3-Flash, plus full-size GLM-5.3-Flash QAD on 32 nodes (EP32 x CP4, packed SFT) with distributed-optimizer save + resume, matching the loss and grad norms of our earlier full PTQ + QAD + vLLM eval study on older heads of this stack.
NVIDIA-NeMo/Megatron-Bridge#6044 and our GLM-5.3-Flash support (NVIDIA/Model-Optimizer#2539) are waiting on this PR, so we'd appreciate a merge when possible. Thanks @HollowMan6!
|
/ok to test 177c4f1 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/40762445544 |
Fixes #7983
What does this PR do?
Add KDA (Kimi Delta Attention) hybrid layers to
HybridModel, including the two-stage low-rank gates used by GLM-5.3-Flash, and an option to keep the KDA projections in BF16 under FP8 training.This PR was split into four, as discussed with @Phlip79 in the comments. It now carries only the KDA layers; the other parts live in their own PRs, each independent of the others and based on
main:fp8_utils.py: Guard blockwise FP8 backward (E5M2) wgrad against all-zero gradient blocks #7523GLM-5.3-Flash at a glance (from its HF
config.json): 45 layers = 34 × KDA (linear_attention) + 11 × DSA (deepseek_sparse_attention) over NoPE MLA; mHC with 4 residual streams; KDAgate_lower_bound = -5.0; 1 MTP depth; 288 routed experts. KDA layers use the newKsymbol in the hybrid layer pattern.Changes
dev:KimiDeltaAttentioninssm/gated_delta_net/kda.py(channel-wise gated DeltaNet on FLAchunk_kda, separatebeta_proj, headwise / chunkwise CP, packed THD),resolve_cp_groupandcp_partition_modeinpacked_seq_params.py,nvtx_rangeinutils.py. CP layout conversion goes throughHybridStack'sContextParallelLayoutManager(megatron/core/context_parallel/layout.py); the earliercontext_parallel_layout/port is gone.dev's KDA:kda_two_stage_gates: GLM-style low-rank gatesf_b(f_a(x))(decay) andg_b(g_a(x))(output) with a QKV-onlyin_proj. The decay gate is precomputed in FP32 with FLAfused_kda_gate(honouringkda_safe_gate/kda_lower_bound), and the output RMSNorm + sigmoid gate run in FLArms_norm_gated, so there is a single BF16 rounding after norm and gating.f_a/g_aare replicated (TELinear,parallel_mode="duplicated");f_b/g_bare column-parallel.A_log/dt_biasare FP32 and marked keep-in-FP32, so they surviveFloat16Module, optimizer construction and checkpointing.kda_disable_fp8: run every KDA projection GEMM with FP8 disabled (see Motivation).recompute_modules=["gdn"](gdn_norm_outkeeps working).KDALayerConfig, layer symbolK,kda_layerslot inHybridStackSubmodules, layer specs inhybrid_stack_specandhybrid_inference_stack_spec._GDNBase(common.py):in_projsizing for two-stage gates, per-variant gate-parameter dtype, headwise CP size override in_prepare_input_for_gated_delta_rule, chunkwise CP-context cache,is_mtp_layer.HybridModel.forward: the sameoutput_processor/output_processor_contexthook asGPTModel(caller-owned output projection and loss).TransformerConfigfields:kda_two_stage_gates,kda_safe_gate,kda_lower_bound,kda_disable_fp8,gdn_conv_pad_alignment,gdn_pre_gated_delta_rule_fusion(reserved; raisesNotImplementedErrorfor KDA).Motivation
With an FP8 actor and a vLLM rollout, every module the rollout computes in BF16 has to be computed in BF16 on the actor as well, otherwise the two diverge. The released FP8 checkpoint keeps every KDA projection (
q/k/v_proj,b_proj,f_*/g_*_proj,o_proj) in BF16 (quantization_config.modules_to_not_convert);kda_disable_fp8lets the actor reproduce that split while the MoE experts (the bulk of the FLOPs) stay in FP8.Relationship to
devand other PRsdevwith additions:kda.py, thekda_layerslot inhybrid_block.py,resolve_cp_group,nvtx_range.dev): two-stage KDA gates,kda_disable_fp8,KDALayerConfig/Ksymbol,HybridModel.output_processor.Known gaps
cp_partition_modeare included, but only CP = 1 has been validated in this PR.NotImplementedError), andgdn_pre_gated_delta_rule_fusionis not implemented for KDA.chunk_kda,fused_kda_gate,rms_norm_gated) and addsjit_fuserhelpers; there is no entry in the kernel determinism manifest yet.Run functional testsis the right CI label.Testing
tests/unit_tests/ssm/test_kda_gate_precision.py(new): FP32 gate parameters survive BF16 wrapping, gated-norm numerics, packed-THD output equals per-sequence output, forkda,kda_directandgdn.tests/unit_tests/ssm/test_hybrid_block.py: theKsymbol builds thekda_layerslot and selects the linear-attention CP layout.Issue tracking
For PRs from open-source community contributors:
Linked issue: Related to #2446, #6392
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.