Repository navigation
🚨 Remove the ep_router expert-parallel style - #49615
IlyasMoutawwakil wants to merge 15 commits into
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. Pages changed in this PR: |
7d13b07 to
1cbdb8c
Compare
ep_router expert-parallel styleep_router expert-parallel plan
vasqu
left a comment
There was a problem hiding this comment.
Not sure whether to fully remove or keep a BC reference at least? and warn when it is used -> remap to dispatch
Every model's experts run on `ep_dispatch_experts`, except Llama 4's, whose EP plan matched no expert-parallel mode and was never applied; its `ep_router` rule goes too. `DistributedConfig(ep_strategy="all_reduce")` gives the same masking and all-reduce, and a plan that still names `ep_router` now says so. `moe_tp_experts` stays for tensor-parallel experts; only its unused expert-parallel flag goes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1cbdb8c to
9845688
Compare
The class, its registration and the docs describing the router-masked plan were still there. A plan that names `ep_router` now raises an error pointing to `ep_dispatch_experts` and `ep_implementation="all_reduce"` instead of a bare KeyError. `moe_tp_experts` loses its `is_expert_parallel` flag (no caller passes it) and the `install_forward` override that only existed to pass it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # docs/source/en/expert_parallelism.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ep_router expert-parallel planep_router expert-parallel style
|
I think it's best to fully remove it otherwise that will create so much confusion |
| "layers.*.feed_forward.gate_proj": "colwise", | ||
| "layers.*.feed_forward.up_proj": "colwise", | ||
| "layers.*.feed_forward.down_proj": "rowwise", | ||
| "layers.*.feed_forward.router": "ep_router", |
There was a problem hiding this comment.
is is the only file that has ep_router left ?
There was a problem hiding this comment.
No: ep_router was in nine model configs (Beam, DiffusionGemma, Gemma 4, HY-V4, Inkling, Llama 4, Mellum, MiMo-V2-Flash, Qwen3-MoE), and this PR removes it from all of them, modular files included. Llama 4 loses its whole base_model_ep_plan (4bad170). After this PR the only ep_router left in the repo is the error mixin.py raises for a user plan that still names it.
There was a problem hiding this comment.
can we not dispatch on llama4's moe instead?
There was a problem hiding this comment.
not without refactoring llama4's moe: its experts take every token already scaled by its router score (hidden_states only), not (hidden_states, top_k_index, top_k_weights), so there's nothing for dispatch to route. moving it to the standard experts interface is a follow-up
The base inlined the router's backward all-reduce check; this branch removes the `ep_router` style, so the class stays gone. In `MoeExpertsParallel`, the routing weights' backward all-reduce is the inline check, without the expert-parallel condition this branch removes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vasqu
left a comment
There was a problem hiding this comment.
Mostly agree here, it's better to remove this indeed otherwise we keep some weird references overall
| | Configuration | Result | | ||
| | :--- | :--- | | ||
| | `DistributedConfig(tp_size=4, fsdp_size=2, ep_size=4)` | Each TP group of four shares one batch. Training dispatches tokens, each rank taking a quarter of the batch; inference uses `"all_reduce"`. A masked `ep_plan` uses masking and all-reduce throughout. | | ||
| | `DistributedConfig(tp_size=4, fsdp_size=2, ep_size=4)` | Each TP group of four shares one batch. Training dispatches tokens, each rank taking a quarter of the batch; inference uses `"all_reduce"`; `ep_implementation="all_reduce"` uses it throughout. | |
| | `DistributedConfig(tp_size=1, fsdp_size=8, ep_size=4)` | Dispatch with an independent batch on each rank, no slicing, and experts FSDP-sharded across pairs of ranks. | | ||
| | `DistributedConfig(tp_size=2, fsdp_size=4, ep_size=4)` | Dispatch with a TP pair per batch, each pair slicing its batch in two; two batches per expert group. | | ||
| | `DistributedConfig(tp_size=8, ep_size=8)` | Every rank shares one batch. Training dispatches tokens, each rank taking an eighth of the batch; inference uses `"all_reduce"`. A masked `ep_plan` uses masking and all-reduce throughout. | | ||
| | `DistributedConfig(tp_size=8, ep_size=8)` | Every rank shares one batch. Training dispatches tokens, each rank taking an eighth of the batch; inference uses `"all_reduce"`; `ep_implementation="all_reduce"` uses it throughout. | |
| ## Combining with FSDP2 | ||
|
|
||
| Tensor and expert parallelism shard the weights across `tp`, but the optimizer state and the modules without a rule are still replicated on every rank of the group, which limits how large a model you can train. Add [FSDP2](./fsdp) on a second mesh dimension with `fsdp_size`. With masking and all-reduce, keep `ep_size=tp_size` and pass `ep_plan={"layers.*.mlp.gate": "ep_router", "layers.*.mlp.experts": "moe_tp_experts"}``. | ||
| Tensor and expert parallelism shard the weights across `tp`, but the optimizer state and the modules without a rule are still replicated on every rank of the group, which limits how large a model you can train. Add [FSDP2](./fsdp) on a second mesh dimension with `fsdp_size`. With masking and all-reduce, keep `ep_size=tp_size` and pass `ep_implementation="all_reduce"`. |
There was a problem hiding this comment.
is this still correct we shard across their own meshes tp/ep no?
There was a problem hiding this comment.
right, fixed: they shard across their own meshes, tp and ep
| if {"ep_router", "moe_tp_experts"}.issubset(ep_plan.values()): | ||
| # Legacy masked EP: the EP group is the TP group, every rank keeps every token. | ||
| model = apply_tensor_parallelism(model, tp_mesh, ep_plan) |
There was a problem hiding this comment.
this is a weird artifect ig if we keep moe tp experts on tp plan it still works as expected only adding it to ep plan is properly ignored right?
this is somewhat breaking as well so best to mention this change more explicitly
There was a problem hiding this comment.
yes, in the tp plan it works as before. in the ep plan it only meant something next to ep_router (the masked path), and alone it would now apply nothing and leave the experts unsharded, so it raises and points to ep_dispatch_experts / ep_implementation="all_reduce". added it to the breaking changes in the description, with a test
| if torch.is_grad_enabled() and top_k_weights.requires_grad: | ||
| top_k_weights = _AllReduceBackward.apply(top_k_weights, tp_group) | ||
| if torch.is_grad_enabled() and top_k_weights.requires_grad: | ||
| top_k_weights = _AllReduceBackward.apply(top_k_weights, tp_group) |
There was a problem hiding this comment.
we should use our helper here then depending on what we do on 1 in the stack
There was a problem hiding this comment.
done, it calls _AllReduceBackward.apply unconditionally now, the grad check lives inside it since #49429
| "layers.*.feed_forward.gate_proj": "colwise", | ||
| "layers.*.feed_forward.up_proj": "colwise", | ||
| "layers.*.feed_forward.down_proj": "rowwise", | ||
| "layers.*.feed_forward.router": "ep_router", |
There was a problem hiding this comment.
can we not dispatch on llama4's moe instead?
# Conflicts: # src/transformers/distributed/expert_parallel.py # src/transformers/distributed/tensor_parallel.py
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: llama4 |
Depends on #49613.
After #49607 and #49613, no model's EP plan uses the router-masked plan (
ep_routeron the router,moe_tp_expertson the experts).ep_dispatch_expertscovers it:DistributedConfig(ep_implementation="all_reduce")gives the same masking and all-reduce (#49429). So this PR removes theep_routerstyle.Changes
distributed/expert_parallel.py:EpRouterParalleland its"ep_router"registration are removed.distributed/mixin.py:ep_routerraises an error that points toep_dispatch_expertsandep_implementation="all_reduce".ep_planthat namesmoe_tp_experts: withoutep_routerit would apply nothing and leave the experts unsharded.moe_tp_expertskeeps working fromtp_plan.distributed/configuration_utils.py: theep_size=tp_sizecheck of the masked plan is removed.AllReduceEPchecks it.distributed/tensor_parallel.py:moe_tp_expertsloses itsis_expert_parallelflag, which no caller passes. It also loses theinstall_forwardoverride that only passed that flag.integrations/tensor_parallel.py: theRouterParallelandEpRouterParallelaliases are removed.ep_routerrule and had no experts rule, so it never applied. Withep_size > 1, attention and the experts stayed replicated.hidden_states, so they cannot useep_dispatch_experts.ep_size > 1now raises the "noep_plan" error.ep_routerplan examples are replaced withep_implementation="all_reduce".EpRouterParallelleaves the parallel styles table.🚨 Breaking changes
ep_routerstyle is removed. A plan that names it raises an error with the replacement.moe_tp_expertsin anep_planno longer selects masked EP; it raises an error with the replacement. In atp_planit works as before.EpRouterParalleland itsRouterParallelalias are removed.ep_size > 1raises instead of silently replicating.Tests
tests/expert_parallelandtests/tensor_parallelon CPU, including both legacy plans being refusedtiktoken🤖 Generated with Claude Code