[trainer] fix: make the multi-trajectory padding row survive THD context parallel - #7449
Open
gaohongkui wants to merge 1 commit into
Open
[trainer] fix: make the multi-trajectory padding row survive THD context parallel#7449gaohongkui wants to merge 1 commit into
gaohongkui wants to merge 1 commit into
Conversation
…ext parallel `construct_minimal_padding_template` builds a 2-token no-op sample, which is below what the THD context-parallel split can handle. `preprocess_packed_seqs` pads each row to `align_size = tp * cp * 2` and hands CP rank r the slice `d[half * r : half * (r + 1)]`, where `half` comes from the padded length while `d` holds only the valid tokens. At tp=cp=2 a 2-token row pads to 8, `half` is 2, and rank 1 asks for `d[2:4]` of a 2-element tensor -- empty, which raises: RuntimeError: The expanded size of the tensor (2) must match the existing size (0) at non-singleton dimension 0. Target sizes: [2]. Tensor sizes: [0] verl's own copy of that function clamps the slice (verl-project#6001), but the vendored copies in mbridge and Megatron-Bridge -- which the megatron engine dispatches to -- do not. Fixes are filed for both (ISEEKYAN/mbridge#157, NVIDIA-NeMo/Megatron-Bridge#5614); sizing the row to the alignment here removes the dependency on which bridge version is installed. Only batches whose size is not already divisible create padding rows at all, so a fixed `train_batch_size * rollout.n` never hits this; it appears once the trajectory count per prompt varies. The row still contributes no gradient and is still excluded from metrics, so nothing else about it changes. Co-authored-by: Claude
gaohongkui
requested review from
PeterSH6,
eric-haibin-lin,
tongyx361 and
vermouth1992
as code owners
August 17, 2026 09:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
construct_minimal_padding_templatebuilds the synthetic divisibility row as 2 tokens(one prompt + one response). That is below what the THD context-parallel split can handle.
preprocess_packed_seqspads each row toalign_size = tp * cp * 2and then hands CPrank r the slice
d[half * r : half * (r + 1)], wherehalfis derived from thepadded length while
d = input_ids[i, attention_mask[i]]holds only the validtokens. For a 2-token row at tp=cp=2 the row pads to 8,
halfis 2, and rank 1 asks ford[2:4]of a 2-element tensor — an empty slice, which raises on assignment:This is not a claim that verl's own
preprocess_packed_seqsis broken — #6001 clampedthat slice and this repo carries the fix. The problem is that the megatron engine dispatches
the forward to the bridge, and the vendored copies there never picked up #6001. Real
traceback from Qwen3.6-35B-A3B GRPO, THD + TP2/CP2 on H20-141G:
Inspecting the loaded modules at runtime confirmed it, rather than inferring it: verl's copy
has the clamp, both vendored copies do not.
I have filed the clamp for the bridges as well — ISEEKYAN/mbridge#157 and
NVIDIA-NeMo/Megatron-Bridge#5614 (two vendored copies there). This PR is the verl half of
the same fix, not a substitute for them: sizing the synthetic row to the alignment means
verl no longer depends on which bridge version happens to be installed, and it is the only
half that helps users on an already-released bridge.
Scope: only batches whose size is not already divisible create padding rows, so a fixed
train_batch_size * rollout.nwith one trajectory per prompt never hits this. It appearsonce the trajectory count per prompt varies — which is exactly what
padding_utilsexistsfor (see the module docstring).
On the constant
Measured smallest valid length at which the unclamped split stops raising:
align_sizeSo 128 total tokens is not a minimum requirement — it is a value that is a multiple of
tp * cp * 2for everytp * cpup to 64, so the row never needs alignment padding on anyrealistic topology and the constant does not have to be revisited when the parallel layout
changes. The cost is 128 no-op tokens per padding row against a per-GPU token budget in the
tens of thousands. Happy to derive it from the config instead if you would prefer that —
padding_utilsdoes not currently seetp/cp, which is why I used a constant.The row still has
response_maskall zero andis_paddingset, so it contributes nogradient and stays out of metrics. The one place that reads
tag["seq_len"]withoutfiltering
is_paddingisglobal_seqlen_lstin_compute_metrics, feedingget_seqlen_balanced_partitionsandlog_seqlen_unbalance; there the true length isstrictly better information, and the effect on the balance metric is ~126 tokens against
hundreds of thousands per rank.
Checklist Before Starting
padding_utils,construct_minimal_padding_template,upsample_batch_to_divisible,preprocess_packed_seqs,context parallel padding(PRs, allstates). The related prior work is [bug] Bug: CP preprocess mixes rmpad tokens with padded layout — crashes on short / heavily padded sequences #5981 / [data] fix: pad data in preprocess_packed_seqs if shorter than align_size #6001 / [data] fix: pad data in preprocess_packed_seqs if shorter than align_size #6035 / fix(mcore): pad valid tokens to padded length in CP preprocess to avoid OOB indexing #6088, which fixed the clamp in
verl's own copy — this PR fixes the producer so the vendored bridge copies stop being load
bearing. No open PR touches
padding_utils.py.[{modules}] {type}: {description}Test
New CPU test
tests/trainer/test_padding_utils_on_cpu.py(17 cases):It pins the property that makes the row safe (its length is a multiple of the alignment for
every
tp * cpup to 64), that the row still contributes no gradient and no reward, theexact failure at 2 tokens on the unclamped path, and — via
torch.equal— that clamping is apure guard for rows of length 128 / 4096 / 14000 at both CP ranks.
Repo checks:
End to end on a cluster: with the longer row, THD + CP=2 completes 4 training steps and
reward magnitude matches an otherwise identical CP=1 run (0.797/0.697/0.473/0.472 vs
0.847/0.415/0.483/0.457 — GRPO rollout is sampled, so only magnitude and trend are
comparable). Peak reserved memory 99.2 GB (CP=2) vs 113.9 GB (CP=1) of 141 GB. Causality was
established by single-variable isolation: reverting only the row length, with CP and the
token budget unchanged, reproduces the crash above.
API and Usage Example
No API change.
_PADDING_TOKENS_PER_SIDEis module-private and affects only the syntheticrows appended for divisibility.
AI assistance
This change was prepared with AI assistance (Claude). I reviewed every changed line, ran the
tests and checks above myself, and can defend the change end-to-end.