fix: pad bshd samples to their own length, not the rollout-global max - #2698
Open
EazyReal wants to merge 1 commit into
Open
fix: pad bshd samples to their own length, not the rollout-global max#2698EazyReal wants to merge 1 commit into
EazyReal wants to merge 1 commit into
Conversation
EazyReal
marked this pull request as ready for review
August 22, 2026 21:42
EazyReal
requested review from
Shi-Dong,
Zhichenzzz,
fzyzcjy,
guapisolo,
jybsuper,
maocheng23 and
yueming-yuan
as code owners
August 22, 2026 21:42
get_rollout_data computes one max_seq_len = max(total_lengths) and fills max_seq_lens with it, so a single long outlier inflates every sample in the rollout (~10x for short samples at 128k+), and pad tokens still dispatch through MoE. Round each sample's own total_length up to pad_size instead. The two max_seq_lens[0] consumers (get_batch, fill_replay_data) assert one padded length per bshd microbatch: build-time CP slicing of rollout log probs uses each sample's own padded length, so a microbatch with multiple padded lengths would misalign silently. validate_args warns at startup when bshd is combined with --micro-batch-size > 1. Measured on a 128x H100 DeepSeek-V4-Flash run at 262,144-token context: a 14,010-token sample padded to 132,096 OOMed the expert MLP at 79 GB; with per-sample padded lengths the same step peaks at 38.5 GB. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EazyReal
force-pushed
the
upstream-pr/per-sample-pad-lengths
branch
from
August 23, 2026 00:43
e78cfb3 to
31cdab5
Compare
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.
Summary
bshduses a rectangular[batch, sequence, heads, head-dim]tensor. Miles previously chose that sequence dimension from the longest sample in the entire rollout, so one long outlier forced even unrelated short samples in other microbatches to carry the same padding. This changesmax_seq_lensfrom a repeated rollout-global maximum to each sample's own aligned padded length.Symptom
Measured on a 128x H100 DeepSeek-V4-Flash run at 262,144-token context: a 14,010-token sample padded to 132,096 OOMed the expert MLP at 79 GB. Pad tokens still dispatch through MoE. With per-sample padded lengths, the same step peaks at 38.5 GB.
Root Cause
get_rollout_datacomputedmax(total_lengths), rounded it for the existing TP/CP/compression alignment requirements, and copied that value into every entry ofmax_seq_lens. Because padding was sized across the rollout rather than at the microbatch that consumes a sample, a single outlier inflated every sample's token count (about 9.4x in the measured case).max_seq_lensis also layout metadata, not only a memory hint: token tensors, rollout log probabilities, replay tensors, and response-logit offsets must use the same padded length when applying context-parallel slicing.Fix
get_rollout_data, round each sample's owntotal_lengthup with the existingpad_sizerule. This preserves all current alignment constraints while avoiding padding across unrelated samples.max_seq_lensas the single source of BSHD padding geometry. Existing per-sample consumers continue to read the corresponding entry, including build-time CP slicing of rollout log probabilities.get_batchandfill_replay_data, assert that every sample in one BSHD microbatch has the same padded length. BSHD is rectangular, and these paths currently form the batch frommax_seq_lens[0]; accepting different padded lengths would make token, log-probability, and replay layouts disagree silently.--micro-batch-size > 1, with remediation to use one sample or bucket samples by padded length.Why This Shape
The change is intentionally made at the existing
max_seq_lensproducer, before rollout-derived tensors are CP-sliced. Computing a larger value only later inget_batchwould be incorrect because those tensors would already have been sliced using different per-sample geometry. The guards therefore fail closed at the two remaining rectangular-batch consumers instead of hiding a layout mismatch.This is the smallest change that fixes the production configuration without introducing a second padding owner or reorganizing scheduling. It supports:
Multi-sample microbatches with different padded lengths are rejected with a targeted error. Fully general support requires establishing the final microbatch membership before CP slicing, assigning one shared maximum to that group, and then slicing every dependent field with that same value. The TODO at the producer records that separate scheduling/data-flow change; taking
max()only at batch consumption would not be sufficient.Performance Trade-off
Per-sample padding can produce more BSHD sequence shapes than one rollout-global target. That reduces dense token work and memory, but may trade away some kernel, execution-plan, or compilation-cache reuse. This PR therefore claims the measured memory result above, not a universal step-time improvement.
--data-pad-size-multiplierremains the existing granularity control: increasing it coarsens aligned-length buckets, trading more padding for fewer distinct shapes. A separate global-max mode is not added without evidence that its cache benefit outweighs the observed OOM risk in a Miles workload.Verification
New CPU coverage in
tests/fast/backends/training_utils/test_bshd_per_sample_pad.pyverifies:get_batchandfill_replay_data.The focused regression file and the complete argument-validation test file pass 146/146 locally on CPU-only torch. The 38.5 GB result above is the same change validated on the H100 run.
Remaining Boundary
max_seq_lensis still computed before multimodal token expansion, matching the previous ordering rather than introducing a regression. No checked-in BSHD recipe uses multimodal token expansion; supporting that combination should recompute the padded lengths after expansion.