Treat LayerWise bucket_size as a soft minimum and fill would-be padding with real parameters - #5415
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
a95bbb9 to
41acdae
Compare
41acdae to
3d8dd18
Compare
|
/ok to test 3d8dd18 |
…ng with real parameters The layer-wise (Muon) optimizer assigns whole parameter matrices to dp_size shards, and a bucket costs its tallest shard times dp_size. Closing a bucket the instant the accumulated size crosses its threshold can cut mid-row, so the remainder opens a second bucket that pads out again; the split itself manufactures padding that neither bucket would have had. With 128 equal expert matrices over 32 shards and a threshold near 100, that is 160 slots instead of 128. Treat the threshold as a soft minimum instead: once it is met, keep absorbing params for as long as each one fits into padding the bucket already has, and close only when the next param would make it taller. Padding inherent to the shape is untouched, and the single-dominating-param case is unchanged: PADDING_FLOOR still governs the soft minimum. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
Cover the soft-minimum bucket_size behavior directly: six equal params over four shards with a threshold landing mid-row now produce one bucket instead of a five-param bucket plus a one-param bucket, and eight params that fill every shard exactly leave no padding at all. Both assertions fail against the cut-on-threshold logic they replace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
3d8dd18 to
94846ca
Compare
|
/ok to test 94846ca |
kunlunl
left a comment
There was a problem hiding this comment.
One small perf-related thought, probably not relevant to the target case with equal-sized expert matrices: _place() tracks an online greedy assignment in backprop order, while _emit_bucket() eventually re-sorts the chunk by numel before running LPT. For mixed-size parameters, those two layouts are not always identical, so the simulated padding can occasionally differ from the final packing.
I found a synthetic aligned example with dp_size=2, bucket_size=448, and backprop-order numels [192, 192, 256, 128, 128, 192], where the new layout uses 1408 elements versus 1280 with the previous splitting behavior.
This looks perf-only rather than a correctness concern, and the pattern may not occur in realistic model layouts. It may just be worth softening the “never larger” wording, or adding a small mixed-size test to document the expected behavior. Not blocking from my side.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31502021306 |
_emit_bucket sorts a chunk before packing it, while the incremental estimate in the bucket cutter places params in backprop order. Equal sizes make sorting a no-op and the two agree, but mixed sizes send params to different shards, so the real maximum shard load can exceed the estimate. _absorbs then admits a param that does grow the bucket. Add kunlunl's counterexample from the review of NVIDIA#5415 as a test: dp_size=2 with backprop-order numels [192, 192, 256, 128, 128, 192] and bucket_size=448 emits 1408 elements where closing at the threshold would have emitted 1280. Left as documented behavior rather than fixed, since the target case is equal-sized expert matrices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
|
@kunlunl thanks, you are right and I reproduced your case exactly: The mechanism is the one you identified.
Since #5415 is already merged, I have addressed both of your suggestions in #6379, which touches the same comment:
Left as documented behavior rather than fixed, per your read that it is perf-only and unlikely in realistic layouts. Making the estimate exact would mean sorting the chunk on every param, which is not worth it for a case that does not arise with equal-sized expert matrices. Happy to revisit if it shows up in a real model. |
_emit_bucket sorts a chunk before packing it, while the incremental estimate in the bucket cutter places params in backprop order. Equal sizes make sorting a no-op and the two agree, but mixed sizes send params to different shards, so the real maximum shard load can exceed the estimate. _absorbs then admits a param that does grow the bucket. Add kunlunl's counterexample from the review of NVIDIA#5415 as a test: dp_size=2 with backprop-order numels [192, 192, 256, 128, 128, 192] and bucket_size=448 emits 1408 elements where closing at the threshold would have emitted 1280. Left as documented behavior rather than fixed, since the target case is equal-sized expert matrices. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
What
LayerWiseDistributedOptimizerassigns whole parameter matrices todp_sizeshards, and a bucket costs the tallest shard timesdp_size. It closes a bucket as soon as the params it has accumulated cross a size threshold.When that threshold lands mid-row, the bucket is cut with a partial shard row and the remainder opens a second bucket that pads out again. The split itself manufactures padding that neither bucket would have had.
Change
Treat the threshold as a soft minimum. Once it is met, keep absorbing params for as long as each one fits into padding the bucket already has, and close the bucket only when the next param would make it taller.
Effect
128 equal expert matrices (
hidden x moe_ffn = 2688 x 1856),dp_size = 32,bucket_size = ~100 matrices:Padding inherent to the shape is untouched: 100 matrices over 32 shards still occupy 128 slots either way, because the last row is genuinely partial.
Notes
PADDING_FLOORstill governs the soft minimum._emit_bucket's packing.The final size-sorted packing is never larger than the membership decision assumed.Correction: that is not true._emit_bucketsorts the chunk before packing it while the estimate walks params in backprop order, so for mixed sizes the two reach different shard loads and absorbing can enlarge the bucket. @kunlunl found a counterexample in review:dp_size=2,bucket_size=448, backprop-order numels[192, 192, 256, 128, 128, 192]emits 1408 elements where closing at the threshold emits 1280. Corrected comment and a regression test are in Balance LayerWise optimizer shards by Newton-Schulz cost, not parameter size #6379. Equal-sized params, the target case, are unaffected because sorting is then a no-op.tests/unit_tests/distributed/test_layer_wise_param_layout.py; both assertions fail against the previous cut-on-threshold logic.🤖 Generated with Claude Code