Repository navigation
CP layout integration - #6776
Conversation
394c7c4 to
790d12c
Compare
790d12c to
623bf9d
Compare
|
/claude strict-review |
| return batch | ||
|
|
||
|
|
||
| def _get_batch_on_this_cp_rank_contiguous( |
There was a problem hiding this comment.
This file is already way too big (not your fault).
Can you move this function to megatron/core/context_parallel/utils.py or similar?
|
Please update PR description as well |
4c87105 to
fb9538a
Compare
|
/claude strict-review |
|
/ok to test fb9538a |
fb9538a to
2acc3d1
Compare
|
/ok to test 2acc3d1 |
Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com> Signed-off-by: Haoran Zhang <haoranz@nvidia.com>
2acc3d1 to
e368ac8
Compare
|
/ok to test e368ac8 |
|
/claude review |
There was a problem hiding this comment.
Light review — a few things worth addressing before merge.
1. forward_step reads args but never binds it (pretrain_hybrid.py:333)
forward_step has no args = get_args(), and there is no module-level args in pretrain_hybrid.py (the only bindings are locals at lines 113, 238, 367, 431). args.seq_length on line 346 is pre-existing and already raises NameError whenever cu_seqlens is not None, but this PR adds a second read on the same path:
if args.linear_cp_layout == "contiguous" and args.context_parallel_size > 1:Compare pretrain_gpt.py:303, which binds args = get_args() at the top of its forward_step.
Worth noting that the new test cannot catch this — it does:
monkeypatch.setattr(pretrain_hybrid, "args", SimpleNamespace(...), raising=False)raising=False creates a module attribute that does not exist in production, so the test passes while the real code path fails. Suggested fix in forward_step:
def forward_step(data_iterator, model: HybridModel):
...
args = get_args()
timers = get_timers()Then drop raising=False from the test (or monkeypatch pretrain_hybrid.get_args) so it exercises the real lookup.
2. _get_batch_on_this_cp_rank_contiguous uses an allow-list where the zigzag path uses a deny-list
megatron/core/context_parallel/utils.py shards only ('tokens', 'labels', 'loss_mask', 'position_ids'), while _get_batch_on_this_cp_rank_per_sequence_balancing (megatron/core/utils.py:2536) shards everything except an explicit METADATA_KEYS set. Any future sequence-dim batch key will be silently left unsharded on the contiguous path — a wrong-results failure mode rather than an error. Consider mirroring the deny-list so the two paths stay in sync, or at least raising on an unrecognized non-metadata tensor key.
3. Docstring bullet order does not match dispatch precedence
In get_batch_on_this_cp_rank the new Contiguous CP bullet is appended last, but the implementation checks use_contiguous_cp first, so it overrides use_per_sequence_balancing and is_hybrid_cp. Worth moving the bullet to the top and stating that it takes precedence — otherwise a caller passing both use_per_sequence_balancing=True and use_contiguous_cp=True will reasonably expect the former to win.
4. Test coverage for the new integration points
The layout unit tests from #6764 cover ContextParallelLayoutManager in isolation, and the new tests cover batch sharding plus packed_seq_params selection. Not covered by any test in this PR:
HybridStack.forward/checkpointed_forwardwithcp_size > 1andlinear_cp_layout != attention_cp_layout— thatprepare_layer/finalize_layerfire at the right layer indices and that the recompute and non-recompute paths agree.sequence_is_contiguous=TrueinMambaContextParallel/GDPContextParallel— thepre_conv_ssm/post_conv_ssmbranches that now skip_undo/_redo_attention_load_balancing. A round-trip test assertingpost_conv_ssm(pre_conv_ssm(x))reconstructsxunder both flag values would pin this down.- The new
_validate_cp_layoutsrejections (invalid layout string,attention_cp_layout='contiguous'with CP, MTP,hybrid_context_parallel) — cheappytest.raisescases, no GPU needed.
5. Minor: linear_cp_layout is only honored by HybridStack
_validate_cp_layouts accepts linear_cp_layout='contiguous' for any TransformerConfig, but the layout manager is built only in HybridStack, and sequence_is_contiguous is threaded only into MambaMixer / GatedDeltaProductMixer. A GPT model with linear-attention layers (linear_attention_freq) would accept the flag and silently ignore it. If contiguous is hybrid-only for now, consider rejecting it elsewhere or documenting the scope on the config field.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/32801918056 |
Signed-off-by: Haoran Zhang <haoranz@nvidia.com> Co-authored-by: Yuzhong Wang <yuzhongw@nvidia.com>
Single repair commit for the 2026-10-16 main->dev sync of the 100-commit main slice ending at 302ed23 onto dev dee456a. Dependencies - pyproject.toml: add gigatoken to the training and mlm extras, which code imported from main requires. - uv.lock: regenerated with uv 0.7.2 (Dockerfile pin); adds gigatoken 0.10.0, awkward 2.14.0 and awkward-cpp 57; no removals. Tests adapted from main's API to dev's API - transformer/test_multi_token_prediction.py: dev's MTPLossLoggingHelper exposes reduced values via reduce_loss_in_tracker()/tracker["values"]. - data/test_get_batch.py: dev's pretrain_hybrid.get_batch reads args.linear_cp_layout and args.context_parallel_size. - inference/contexts/test_dynamic_prefix_caching.py: on dev the engine records the Mamba match count before admission; set it like the engine. - models/mimo/test_mimo_1f1b_schedule.py: pass use_layer_wise_param_layout so DistributedDataParallelConfig agrees with the layer-wise optimizer. - transformer/experimental_attention_variant/test_attention_variant_dsa.py: dynamic context parallel on dev requires max_seqlen_per_dp_cp_rank. - transformer/test_multi_token_prediction.py (main NVIDIA#6119 speculative-decoding capture test): dev stores the MTP input on model._decoder_hidden_states_cache, not on inference_context.mtp_decoder_hidden_states. Golden values - moe/gpt3_moe_mcore_te_ep8_resume_torch_dist_dist_muon dev H100 golden refreshed from the exact merge-candidate run: main 0560fbb (NVIDIA#6192) makes the LayerWise Muon child grad statistics span the intra distributed-optimizer group; reverting only that line reproduces dev's previous golden values exactly. - tests: set max_seqlen_per_dp_cp_rank in dev dynamic-CP/scheduler test configs (transformer_layer, transformer_config_dynamic_cp, dsv4_hybrid_attention_cp) to satisfy main NVIDIA#5902 validation; dev arguments.py already derives it when a sequence-packing scheduler is set. - tests: transformer/test_thd_cuda_graph.py balanced DSA dynamic-pack case with max_seqlen_per_dp_cp_rank=None now expects main NVIDIA#5902's earlier ModelParallelConfig validation message. - tests: extend main NVIDIA#6284 mimo overlap-lifecycle optimizer mock with the dev _optimizer_state_offloader/_layer_wise_non_distopt_child attributes (NVIDIA#6244). - tests: extend main NVIDIA#6119 HybridModel MTP forward stub with dev moe_n_hash_layers and pg_collection.dp_cp used by the dev forward. Megatron-FSDP v2 symmetric memory (requester-approved alignment with main) - experimental/dbuffer.py, experimental/parameter_group.py: take main's version. Symmetric-memory MFSDP raises "requires PyTorch 2.12 or later" on PyTorch builds without torch.distributed._symmetric_memory .is_symm_mem_tensor, as on main. This drops dev's allocation-flag fallback (ef8d090, 4a534df); with it, the symmetric path's losses diverged across ranks on dev's PyTorch 2.12 pre-release image and test_fully_shard_symmetric_memory_matches_default_and_profiles_nccl[3] failed on the merge candidate. - mfsdp_v2 tests: skip the symmetric-memory cases on such builds (test_symmetric_memory.py, the symmetric_memory params of test_overlap.py and test_cuda_graph.py, and test_mcore_adapter.py nccl_ub). They run again once the image provides is_symm_mem_tensor. Hybrid context parallelism (review 5570704991) - models/hybrid/hybrid_block.py: HybridStack rejects linear_cp_layout='contiguous' with context parallelism when the segment contains GDN ('G') or KDA ('K') layers. main NVIDIA#6776 feeds contiguous shards to linear layers, but dev's GDN/KDA chunkwise and headwise paths still read their input layout from cp_partition_mode (zigzag by default) and would scramble token order. Contiguous Mamba/GDP and the zigzag GDN/KDA paths are unchanged. - ssm/test_hybrid_block.py: cover the GDN/KDA rejection and the zigzag GDN and contiguous Mamba builds under CP=2. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
Single repair commit for the 2026-10-16 main->dev sync of the 100-commit main slice ending at 302ed23 onto dev dee456a. Dependencies - pyproject.toml: add gigatoken to the training and mlm extras, which code imported from main requires. - uv.lock: regenerated with uv 0.7.2 (Dockerfile pin); adds gigatoken 0.10.0, awkward 2.14.0 and awkward-cpp 57; no removals. Tests adapted from main's API to dev's API - transformer/test_multi_token_prediction.py: dev's MTPLossLoggingHelper exposes reduced values via reduce_loss_in_tracker()/tracker["values"]. - data/test_get_batch.py: dev's pretrain_hybrid.get_batch reads args.linear_cp_layout and args.context_parallel_size. - inference/contexts/test_dynamic_prefix_caching.py: on dev the engine records the Mamba match count before admission; set it like the engine. - models/mimo/test_mimo_1f1b_schedule.py: pass use_layer_wise_param_layout so DistributedDataParallelConfig agrees with the layer-wise optimizer. - transformer/experimental_attention_variant/test_attention_variant_dsa.py: dynamic context parallel on dev requires max_seqlen_per_dp_cp_rank. - transformer/test_multi_token_prediction.py (main NVIDIA#6119 speculative-decoding capture test): dev stores the MTP input on model._decoder_hidden_states_cache, not on inference_context.mtp_decoder_hidden_states. Golden values - moe/gpt3_moe_mcore_te_ep8_resume_torch_dist_dist_muon dev H100 golden refreshed from the exact merge-candidate run: main 0560fbb (NVIDIA#6192) makes the LayerWise Muon child grad statistics span the intra distributed-optimizer group; reverting only that line reproduces dev's previous golden values exactly. - tests: set max_seqlen_per_dp_cp_rank in dev dynamic-CP/scheduler test configs (transformer_layer, transformer_config_dynamic_cp, dsv4_hybrid_attention_cp) to satisfy main NVIDIA#5902 validation; dev arguments.py already derives it when a sequence-packing scheduler is set. - tests: transformer/test_thd_cuda_graph.py balanced DSA dynamic-pack case with max_seqlen_per_dp_cp_rank=None now expects main NVIDIA#5902's earlier ModelParallelConfig validation message. - tests: extend main NVIDIA#6284 mimo overlap-lifecycle optimizer mock with the dev _optimizer_state_offloader/_layer_wise_non_distopt_child attributes (NVIDIA#6244). - tests: extend main NVIDIA#6119 HybridModel MTP forward stub with dev moe_n_hash_layers and pg_collection.dp_cp used by the dev forward. Megatron-FSDP v2 symmetric memory (requester-approved alignment with main) - experimental/dbuffer.py, experimental/parameter_group.py: take main's version. Symmetric-memory MFSDP raises "requires PyTorch 2.12 or later" on PyTorch builds without torch.distributed._symmetric_memory .is_symm_mem_tensor, as on main. This drops dev's allocation-flag fallback (ef8d090, 4a534df); with it, the symmetric path's losses diverged across ranks on dev's PyTorch 2.12 pre-release image and test_fully_shard_symmetric_memory_matches_default_and_profiles_nccl[3] failed on the merge candidate. - mfsdp_v2 tests: skip the symmetric-memory cases on such builds (test_symmetric_memory.py, the symmetric_memory params of test_overlap.py and test_cuda_graph.py, and test_mcore_adapter.py nccl_ub). They run again once the image provides is_symm_mem_tensor. Hybrid context parallelism (review 5570704991) - models/hybrid/hybrid_block.py: HybridStack rejects linear_cp_layout='contiguous' with context parallelism when the segment contains GDN ('G') or KDA ('K') layers. main NVIDIA#6776 feeds contiguous shards to linear layers, but dev's GDN/KDA chunkwise and headwise paths still read their input layout from cp_partition_mode (zigzag by default) and would scramble token order. Contiguous Mamba/GDP and the zigzag GDN/KDA paths are unchanged. - ssm/test_hybrid_block.py: cover the GDN/KDA rejection and the zigzag GDN and contiguous Mamba builds under CP=2. - tests: transformer/test_multi_token_prediction.py TestMultiTokenPredictionHybrid (main NVIDIA#6119) stub config sets cross_entropy_loss_fusion=False and cross_entropy_fusion_impl="native" (the TransformerConfig defaults); dev NVIDIA#8105 HybridModel.forward reads them when labels are given (review 5573599695). Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
Background
TE softmax attention requires a zigzag CP layout, while linear attention does not. With
chunkwise CP, using the zigzag layout for linear-attention layers doubles their communication. This PR wires the
megatron/core/context_parallelmodule from #6764 into hybrid models.Summary
linear_cp_layoutandattention_cp_layouttoTransformerConfig, both defaulting tozigzag, so existing runs are unchanged.Tests