Repository navigation
[Bugfix][EPLB] Record the load window by logical expert - #60686
almogtavor wants to merge 7 commits into
Conversation
The load window is recorded per physical slot. When step_interval is smaller than window_size, rows recorded under the old map survive the rearrange and get counted for the new occupant of the slot. Signed-off-by: almogtavor <almogtavor@gmail.com>
There was a problem hiding this comment.
This is a real issue with our current EPLB implementation (I mentioned it in #58473’s description as well), but I see two problems with this fix:
-
This is dangerous for async EPLB. Async EPLB transfers one layer at a time, so
_commit_eplb_maps_for_layer()for the last layers might happen very close to the next EPLB reshuffle, depending on transfer time and window/step configs. That reshuffle could use very little data for those layers, or even no data if the last layer is committed in the same step that triggers a reshuffle. This can result in really bad mappings which severely hurt e2e performance. -
For sync EPLB, this change effectively silently sets window_size to step_interval whenever window_size > step_interval, which is pretty misleading
I spent some time thinking about this while fixing #58473. I think the safest solution is to record load by logical expert, so changing the physical mapping does not invalidate the history. But this is not an easy change as we need to make sure we do it without slowing down statistics collection.
|
Thanks @itayalroy it makes sense. What do you think about doing the logical conversion once per step instead of in the forward pass? I mean leaving the forward pass as is so it keeps counting per physical slot in I think this is safer than counting by logical ID in the forward pass, since recording code stays untouched, including the code that lives in I measured it on 4xH100 with DeepSeek-V2-Lite-Chat and EP with rearranges every 200 steps. The per-step copy went from about 9.2us to 10.5us, rearrange() got about 2x faster because the window is already logical, and E2E throughput was 80.3 req/s on main vs 81.3 with this change over 6 runs each. Already updated the PR with this, but happy to change it if you prefer a different approach |
Convert expert_load_pass to logical ids when copying it into the window in step(), using a cached index rebuilt on map changes. This keeps the forward pass recording per physical slot, keeps the window history across async per-layer commits, and keeps window_size unchanged. Signed-off-by: almogtavor <almogtavor@gmail.com>
I benchmarked this PR with DeepSeek V4 Pro on 20 GB200 GPUs. Compared with upstream, the median gap between forward passes (measured with CUDA events immediately after one forward call and before the next) increased by about 11 µs, and the EPLB step itself increased by the same amount. No measurable effect on e2e performance though. I suspect the extra kernel launch accounts for much of that 11 µs. A fused Triton kernel would probably help here (it could even improve performance over upstream as we can fuse three kernels into one, compared with two kernel launches in upstream), but it would add code complexity, so not sure if it is worth it. @tlrmchlsmth @ilmarkov for vis |
Signed-off-by: almogtavor <almogtavor@gmail.com>
7a094e1 to
e5d9cdf
Compare
|
Thank you for the fix, it does make sense! Regarding adding extra torch calls on the hot path. |
|
Also, could you please add accuracy validation results (e.g. |
There was a problem hiding this comment.
Also note that if we record expert load by logical expert, we don't need this EEP w/a anymore, we can remove it and clean up some code
|
@ilmarkov yes I'll do that even though I don't see any signal for accuracy being affected. I'll soon look into your suggestion as well thanks |
The routing kernel already has the logical id, so it now adds the load there and the window no longer needs a scatter in step(). This also removes expert_load_pass_buffer and last_expert_load, since the window keeps its width when Elastic EP scales. Signed-off-by: almogtavor <almogtavor@gmail.com>
|
I moved the recording into Results on H100:
The full tables are in the description. |
|
This pull request has merge conflicts that must be resolved before it can be |
…-stale-window Signed-off-by: almogtavor <almogtavor@gmail.com> # Conflicts: # tests/kernels/moe/test_routing.py # vllm/distributed/eplb/eplb_state.py
| ): | ||
| # num_tokens_per_rank: (num_moe_layers, num_ranks) | ||
| # Spread each expert's load evenly over its replicas. | ||
| phy2log = eplb_model_state.physical_to_logical_map |
There was a problem hiding this comment.
We should use actual physical-expert counts for log_balancedness. We should record both logical and physical expert load in the existing routing kernel when log_balancedness is enabled.
Logical counts keep the rearrangement window valid across map changes, physical counts let us report actual per-rank balance. Splitting logical load evenly among replicas can hide a skewed replica and make balancedness look better than it is.
There was a problem hiding this comment.
I changed this. The routing kernel now records both logical and physical counts when log_balancedness is on, and the logged balance comes from the physical counts. The window still reads only the logical counts. With logging off, the physical buffer isn't allocated and the second add compiles out. The test results are in the PR description.
|
This pull request has merge conflicts that must be resolved before it can be |
Record physical counts in the routing kernel when log_balancedness is on, so the logged balance is exact instead of an even split over replicas. Signed-off-by: almogtavor <almogtavor@gmail.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Keep the logical load window and use single backticks in the new docstrings. Signed-off-by: almogtavor <almogtavor@gmail.com>
ilmarkov
left a comment
There was a problem hiding this comment.
Thank you for the update. Looks good. Could you add e2e throughput and latency comparison to the main? 10us overhead per layer in case of hot experts sounds not good. We need to check that e2e performance in normal conditions is not severely affected.
Also, is there a test verifying that the initial issue is solved? Test that checks that a partially refreshed window retains the right logical load after a map change.
| """ | ||
| Experts load window. | ||
| Shape: (window_size, num_moe_layers, num_physical_experts) | ||
| Shape: (window_size, num_moe_layers, num_logical_experts) |
| ) | ||
| # Concatenate, all_reduce, then unpack to original shapes. | ||
| # We assume all tensors are 2D and shape[1] (num_physical_experts) | ||
| # We assume all tensors are 2D and shape[1] (num_logical_experts) |
There was a problem hiding this comment.
Nit: For log_balancedness it can be num_physical_experts too
|
This pull request has merge conflicts that must be resolved before it can be |
Purpose
Fixes #60685.
EPLB records load per physical expert. When step_interval < window_size, part of the window still holds load recorded under the old map, and the next rearrange counts it for the expert that now sits at that physical index.
This PR records load by logical expert instead.
eplb_map_and_recordalready has the logical id, so theatomic_addnow goes to the logical id rather than the physical index.expert_load_passand the window have logical width, so old steps stay with the right expert after the map changes, and rearrange only sums the window. No extra kernel or scatter is added.The PR also removes
expert_load_pass_buffer,last_expert_loadanduse_last_expert_load. The number of logical experts does not change when Elastic EP scales, so the window stays valid across a scale and the scale rearrange uses it directly.log_balancednessneeds per rank load, so the same kernel also records physical counts into a newphysical_load_passwhenlog_balancednessis on. The window never reads these counts, and they are cleared in the samestepas the logical pass, so each logged value comes from one forward under one map. Withlog_balancednessoff the buffer is not allocated and Triton compiles the secondatomic_addout. The MegaMoE kernels share this helper, so they get the same recording.No open PR fixes this. #49700 only clears the window on an elastic EP topology change, and #64485 changes when rearrangements run, not how the window is recorded.
Test Plan
All runs are on H100, on main at
24a49203eewith this PR on top. The native build is the nightly wheelef70c6a23f, 4 commits before that, with the Python files replaced. The only native difference in those commits isvllm_flash_attn.cmake.pytest tests/kernels/moe/test_routing.py tests/distributed/test_eplb_utils.pypytest tests/kernels/moe/test_deep_gemm_mega_moe.py -k test_prepare_megamoe_eplb_maps_and_records_under_graph_replay, with@requires_sm100removed locally because this test only runs the Triton staging kernels.window_size=1000, step_interval=100, num_redundant_experts=2, log_balancedness=true, 5-shot, 2 runs per side.pytest tests/distributed/test_elastic_ep.py -k "enforce_eager_heavy or cuda_graphs_heavy"on 4xH100 with NIXL 1.5.2, main vs this PR with logging off and on. For logging on I addedlog_balancednessto the server args locally. This scales dp 2 to 4 and back under heavy traffic and checks gsm8k after each scale.log_balancednesssync, fold the physical counts through the current map and compare them with the logical pass.Test Result
test_routing.py+test_eplb_utils.py: 518 passed. Running the same tests on main gives 10 failed, which are the 8 changed routing cases,test_eplb_map_records_exact_physical_loadandtest_log_balancedness_from_physical_load. The MegaMoE test: 8 passed, with logging off and on.The physical counts folded through the map matched the logical pass in every logged check: 364 in gsm8k and 61 in Elastic EP, including at physical widths 64 (dp 2) and 128 (dp 4). No check reported a mismatch.
Record kernel time per call (us):
With logging off, the only clear slowdown is one very hot expert out of 256, up to about 10 us per layer at 16k tokens and under 1 us at 1k tokens. The 128+32 case got faster. Logging on adds 1.4 to 2.5 us per layer up to 1k tokens and 15 to 32 us at 16k tokens, and only when
log_balancednessis enabled. The two runs of main differed by at most 1.1%.gsm8k accuracy with
log_balancednesson:Every run did 3 to 6 async rearranges with no errors. Run 2 of this PR also had the temporary check and
log_balancedness_interval=10, while the other runs used 50.Elastic EP scaling: 3 passed on main, 3 passed on this PR with logging off, 3 passed on this PR with logging on. gsm8k on 256 questions:
The test also checks accuracy after scaling back to 2, and that check passed in every case.