Repository navigation
Conversation
Rebalance each canonical-grid micro-batch across a module's DP ranks by a per-sample cost (encoder patches, optionally language tokens) so paired vision/language replicas keep identical sample order. Adds ReorderingBuffer (window exchange, optional prefetch overlap), dataset config fields megatron_mimo_intra_microbatch_reorder / *_reorder_encoder_cost_weight / *_reorder_language_cost_weight / *_reorder_overlap / *_reorder_window_size, and the --intra-microbatch-reorder wiring in the Qwen3.5-VL example. A dp=1 module reorders locally into the same canonical order. The example collate now always ships the cheap [B, S] fields (input_ids, position_ids, attention_mask) on every stage so data-path consumers (reorder cost, packing lengths, MRoPE) need no per-feature gating; forward_step alone decides what the model receives and always drops the tokenizer attention_mask. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…se; tidy reorder comments The reorder merge concatenates exchanged samples along the batch dim, so every rank must pad to the same sequence length; reject the combination at argument validation instead of failing inside torch.cat. Comment/docstring cleanup in reorder_buffer.py and its tests: drop internal design-doc references, fix a stale cross-reference, correct the resident-window memory note (3·W), and replace "legacy/historical" wording. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…omments Rename _ALIGN/_pad_to_align to _SERIALIZED_TENSOR_BYTE_ALIGNMENT/ _alignment_padding_bytes and shorten the comments added with the reorder wiring. No behavior change. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
- Qwen3VLGPTModel.forward: accept `mtp_input_mask` and forward it to `_postprocess`, matching GPTModel.forward. MCore's MimoModel passes this kwarg unconditionally since NVIDIA/Megatron-LM#6770, so the overridden forward raised TypeError on the first step. - training_log: read `cuda_graph_impl` with getattr. MegatronMIMOProvider is not a TransformerConfig and has no such field, so the memory report added in NVIDIA-NeMo#6003 raised AttributeError after iteration 1. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…MAX_CONNECTIONS=1
With a single hardware queue the prefetch thread's side-PG all-to-all and
the main thread's DDP all-reduce execute strictly in issue order, and the
two threads issue in rank-dependent order. One rank then queues
[all-to-all, all-reduce] while a peer queues [all-reduce, all-to-all], each
NCCL kernel waits for a peer kernel that cannot start, and training hangs.
Reproduced twice: iteration 290 on a 4-rank dp2/dp2 run and iteration 33 on
a 6-rank TP=2 run; the non-overlapped path completed 1500 iterations under
the same variable. The previous warning ("correct but no faster") was wrong.
- ReorderingBuffer: raise ValueError for overlap=True under the variable
(dp_size > 1) instead of logging a warning; docstring updated.
- Qwen3.5-VL example: reject the combination in _validate_mimo_batch_sizes
so every rank fails before model build; the --no-overlap path is allowed.
- Docs: replace the warning description with the refusal and the TP choice.
- Unit tests for the buffer guard (refuse / allow without overlap / allow
without the variable) and the example validate.
Signed-off-by: kayeon.song <kayeon.song@navercorp.com>
Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com>
Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com>
Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com>
Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
Rewrite the reorder_buffer docstrings in the Google style used by the neighbouring megatron_mimo modules (summary, Args, Returns, Raises) and drop the design narrative; keep inline comments only where they state a constraint the code depends on. Same trim for the reorder unit and functional tests. No code change (AST identical). Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…the reorder cost needs them Restore the upstream per-rank field selection in _batch_spec_for_rank: input_ids goes to non-first language stages and encoder ranks only when intra-microbatch reorder is on (its cost is computed on every stage), and attention_mask only when packing or a reorder language cost term needs it. Ranks running without reorder receive the same fields as upstream. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…y report MegatronMIMOProvider is not a TransformerConfig and has no cuda_graph_warmup_steps; with optimizer_cuda_graph=True the memory report added in NVIDIA-NeMo#6003 raised AttributeError on the first logged iteration. Same treatment as the cuda_graph_impl read in the sibling line. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…IMO train loop The MIMO loop ends only when train_iters is reached or the iterator is exhausted; at that point the ReorderingBuffer worker has already exited on its own (verified on 2 ranks), so the hook never did any work. Dropped iterators are handled by ReorderingBuffer.__del__. The train loop is now unchanged from upstream. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…e MIMO reorder page - State the Megatron Core limit that the encoder data-parallel size must not exceed the language one, and that a dp-1 module's local permutation runs synchronously. - Add Expected Metric Changes and Representative Validation Patterns in the form used by the other training pages, from the 8×A100 Qwen3.5-0.8B runs. - Mark tensor parallelism as validated; list the page in the docs toctree. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
- Unit: real_token_lengths counts mask ones per sample, keeps pad-valued real tokens, and rejects a missing or mismatched attention_mask. - Functional (2 GPUs): ReorderingBuffer with overlap=True exchanges three windows on its side NCCL group, serves the balanced shards in order, and its prefetch thread exits on its own once the source iterator is exhausted. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
…e MIMO reorder page The two-rank functional test does run the real exchange (and now the overlapped prefetch thread); attribute each validation result to the configuration that produced it. Signed-off-by: kayeon.song <kayeon.song@navercorp.com> Signed-off-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Signed-off-by: Chanwoo Park <chanwoo.park98@navercorp.com> Co-authored-by: Yoonsik Kim <yoonsik.kim90@navercorp.com> Co-authored-by: Chanwoo Park <45866990+sailor1493@users.noreply.github.com>
| self._active: List[Dict[str, Any]] = [] | ||
| self._cursor = 0 | ||
| self._exchanges = 0 | ||
| if overlap and dp_size > 1 and os.environ.get("CUDA_DEVICE_MAX_CONNECTIONS") == "1": |
There was a problem hiding this comment.
Was a warning in #4608; reproduced as a hang (iteration 290 on 4 ranks, 33 on 6 ranks with TP 2).
| if packing_kwargs is not None: | ||
| data_batch["packing_kwargs"] = packing_kwargs | ||
| # The padding mask only feeds data-path lengths; the model runs causal attention and must get None. | ||
| data_batch["attention_mask"] = None |
There was a problem hiding this comment.
attention_mask now reaches non-packing stages when the language cost term is on; the model must not receive it.
| print("[Rank {}] {}".format(torch.distributed.get_rank(), memory_string), flush=True) | ||
| cuda_graphs_enabled = ( | ||
| config.model.cuda_graph_impl != "none" | ||
| getattr(config.model, "cuda_graph_impl", "none") != "none" |
There was a problem hiding this comment.
Unrelated to reordering: MegatronMIMOProvider has no cuda_graph_* fields, so MIMO crashed here after iteration 1.
| *, | ||
| inference_params: Optional[BaseInferenceContext] = None, | ||
| loss_mask: Optional[Tensor] = None, | ||
| mtp_input_mask: Optional[Tensor] = None, |
There was a problem hiding this comment.
Unrelated to reordering: MimoModel passes this kwarg since NVIDIA/Megatron-LM#6770; MIMO failed on the first step without it.
| reorder_on = ( | ||
| scalable_dp | ||
| and bool(getattr(cfg.dataset, "megatron_mimo_intra_microbatch_reorder", False)) | ||
| and canonical_grid_size(module_dps) > 1 |
There was a problem hiding this comment.
Gated on the canonical grid, not this rank's DP size, so dp-1 ranks also enter the collective process-group setup below.
|
The two compatibility fixes included here correspond to NVBug 6864016: forwarding mtp_input_mask through Qwen3VLGPTModel.forward and handling absent CUDA graph fields in MegatronMIMO training logging. Both failure sites described by the report are covered in the diff. Tracking this as pending QA verification; this comment does not claim a completed release validation. |
|
/ok to test c55ce2c |
|
Automatic Claude reviews have been retired. To request a pull-request review, post a comment containing: Add |
What
Adds opt-in intra-microbatch reordering to MegatronMIMO scalable data-parallel reads
(#5525). With scalable reads each data rank holds a fixed
1/dpshard of every micro-batch,so the per-rank vision load is whatever the sampler assigned. Image sizes vary widely, and the
slowest vision rank gates every step. Reordering rebalances each micro-batch across a module's
DP group before the forward step:
image-placeholder token count in
input_ids(= vision patches /spatial_merge_size²),optionally plus the real token count from
attention_mask.deterministic equal-count LPT assignment over the canonical grid (LCM of the module DP
sizes), so the vision and language modules derive one identical assignment without
talking to each other; a dp-1 module permutes locally.
all_to_all_single(NCCL) and each rank reassembles itsmicro-batch in canonical order, so the
BridgeCommunicatorpositional fan-out staysaligned.
By default the next window's exchange runs on a background thread with its own NCCL group,
overlapping the current window's compute.
megatron_mimo_reorder_window_sizebalancesWmicro-batches in one exchange.
Third of three PRs splitting #4608 (original work by Chanwoo Park @sailor1493, credited as
co-author and signer), per the maintainers' split request in #4609. Parts 1 and 2 merged as
#5131 and #5525.
Differences from #4608
different DP sizes (including dp 1) agree on one assignment.
attention_maskis also shipped to every module when the language cost term is on;input_idsfollows feat(data): intra-microbatch reordering for MegatronMIMO (+ sequence packing, scalable DP) #4608 (every language PP stage when reordering). Both are decided in_batch_spec_for_rank; otherwise the per-rank field selection of improve Qwen MegatronMIMO data loading #4442 is unchanged.CUDA_DEVICE_MAX_CONNECTIONS=1. feat(data): intra-microbatch reordering for MegatronMIMO (+ sequence packing, scalable DP) #4608 only warned ("correct but nofaster"); with a single hardware queue the worker's all-to-all and the DDP all-reduce execute
in issue order, which differs per rank, and training deadlocks (reproduced at iteration 290
on 4 ranks and iteration 33 on 6 ranks with TP 2); the non-overlapped path is unaffected.
--pad-to-seq-length falseis rejected with reordering (ranks must pad to one width toconcatenate exchanged samples).
encoder_cost_weight/language_cost_weight; negative weights rejected.Configuration
All on
DirectHFSFTDatasetConfig, off by default; the Qwen3.5-VL example exposes them as--intra-microbatch-reorder,--reorder-encoder-cost-weight,--reorder-language-cost-weight,--no-overlap-intra-microbatch-reorder,--reorder-window-size. The library loader does notwire the buffer (the cost needs model token ids); the example does.
Notes for reviewers
bimodal image-size distribution the measured spread (max−min 643 → 421 image tokens, 8 ranks)
matches the best reachable value.
A100); a 3-rank run with vision dp 1 is about 5% slower with reordering, an 8-rank run with
vision dp 4 about 2% faster.
Testing
DP {(1,2), (2,4), (2,3)}, serialization round-trip for every supported dtype and MRoPE
[3,B,S]position ids, per-slot reassembly for windows of 1–3, cost module-independence(vision-style vs language-style sample), the
CUDA_DEVICE_MAX_CONNECTIONS=1refusal, theexample's batch-spec and validate paths.
single- and variable-image samples, vision rows travelling with their sample byte-for-byte;
and
ReorderingBuffer(overlap=True, window_size=2)serving three windows on its side NCCLgroup, with the prefetch thread exiting on its own once the source is exhausted.
iteration-1/2 losses (language dp 2 + vision dp 2, 30 iterations; pretrained weights
iteration-1 difference 1.3e-4, so image–text pairing is intact); per-micro-batch image-token
sums over all vision ranks identical to the unreordered run (dp 4 + dp 4, 40 micro-batches);
checkpoint resume reproduces the continuous run; language pp 2, tp 2, tp 2 + vision tp 2 and
pp 2 + dp 2 each 1000 iterations; window {1,2,4} × overlap on/off × language cost {0, 0.5}
13 × 1000 iterations; 3000 iterations with overlap; 10000 iterations without overlap under
CUDA_DEVICE_MAX_CONNECTIONS=1.Known limitations
Encoder PP > 1 is rejected with reordering; context parallelism is untested; encoder dp must
not exceed language dp, because Megatron Core's bridge fan-in with variable image-token counts
asserts (
MimoModel._attach_modality_split_sizes) — this is independent of reordering.Also included
Two small fixes unrelated to reordering:
Qwen3VLGPTModel.forwardaccepts themtp_input_maskkwarg thatMimoModelpasses since NVIDIA/Megatron-LM#6770 (MIMO trainingfails on the first step without it), and the one-shot memory report from #6003 reads
cuda_graph_impl/cuda_graph_warmup_stepswithgetattrbecauseMegatronMIMOProviderisnot a
TransformerConfig(fails after iteration 1, and withoptimizer_cuda_graph=True).Signed-off-by: kayeon.song kayeon.song@navercorp.com
Signed-off-by: Yoonsik Kim yoonsik.kim90@navercorp.com
Signed-off-by: Chanwoo Park chanwoo.park98@navercorp.com