Repository navigation
🚨 Separate EP logic into its own file - #49679
Conversation
cbb8a4d to
2948f5d
Compare
|
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. |
4193ff1 to
4481fe3
Compare
| ) | ||
|
|
||
|
|
||
| def __getattr__(name: str): |
There was a problem hiding this comment.
not so sure about this one. Wdyt ? @ArthurZucker @vasqu
There was a problem hiding this comment.
dang yea kind of a wear workaround but its for cases where you import from the file right? Imo ok to keep as its for BC but we should add a small comment to explain this!
6fc8d85 to
4909c46
Compare
a4aaabd to
3e6c08c
Compare
3e6c08c to
8c17784
Compare
8c17784 to
b68ebc4
Compare
b68ebc4 to
9ef53bb
Compare
IlyasMoutawwakil
left a comment
There was a problem hiding this comment.
Thanks ! very much needed ! left a couple comments, my one concern is the double file dependency which is preferably avoidable imo, it'd be much better if we draw clear lines between the diff parallelisms in terms of integration as well, the registery can be somewhere central
| # Set to None to run distributed EP tests for every model with a plan. | ||
| # Representative MoE model types covered by distributed EP tests. | ||
| EP_DISTRIBUTED_TEST_MODEL_TYPES = { | ||
| "qwen4_exp_text", | ||
| "qwen3_moe", | ||
| "glm_moe_dsa", | ||
| "deepseek_v4", | ||
| "glm4_moe_lite", | ||
| "glm4_moe", | ||
| "olmoe", | ||
| "qwen2_moe", | ||
| "cohere2_moe", | ||
| # VLM | ||
| "glm4v_moe", | ||
| } |
There was a problem hiding this comment.
in general these allow lists keeps a lot of problems under the radar, i'd much more prefer making the tests cheaper an enabling them on all MoEs, and VLMs, simple lm decoders are generally fine having one representative, but moes/vlms/alms/omnis can implement some things very differently (ernie text/vision experts, DSAs, attn sinks, ..)
There was a problem hiding this comment.
they are already tiny models running. I once enabled them for a lot of models but @ydshieh was complaining that it was still taking a lot of time. Maybe I can distribute it on more machines ?
There was a problem hiding this comment.
OR, we can keep it this way and wait for my transformer training CI dashboard coming up next. I am planning to run small training through HF jobs to check bitwise correctness on all models. Will post internally about the idea soon
There was a problem hiding this comment.
yeh i was also surprised when i ran them all locally they weren't that slow but collectively they might be, the biggest slowdown might be in the multi-process spawning, i wonder if we can use threads to simulate a multi-proc setup for example, wdyt ?
There was a problem hiding this comment.
or cache it once you are on a single machine and config does not change
There was a problem hiding this comment.
I still dont see why we cant run all for nightly at least? Its fine to have a small list for the fast CI but nightly should run all no?
There was a problem hiding this comment.
yes yes i agree, a once per day nightly is better ! same as we do for export.
but we need a way to @slow all models except a list of models that continue to run in fast ci
There was a problem hiding this comment.
agree but this belongs to another PR tho
| return output | ||
|
|
||
|
|
||
| if is_torch_distributed_available(): |
There was a problem hiding this comment.
no need for guarding ig, both ALL_PARALLEL_STYLES and the layers are defined
There was a problem hiding this comment.
maybe we should move ALL_PARALLEL_STYLES to something more general and let tp / ep define their own list => update the ALL_PARALLEL_STYLES based on both of theirs
atm it's still very tp focused and needs a more centralized point imo
ArthurZucker
left a comment
There was a problem hiding this comment.
Yep let's be careful with BC but otherwise full gassss
| parallelism=6, | ||
| ) | ||
|
|
||
| expert_parallel_ci_job = CircleCIJob( |
There was a problem hiding this comment.
not sure that needs to be separated!
There was a problem hiding this comment.
imo better to separate, it was already weird enough imo that tp and ep were used in the same context of tests
There was a problem hiding this comment.
yeah I prefer separating as well
vasqu
left a comment
There was a problem hiding this comment.
Some smaller comments but overall much needed
| parallelism=6, | ||
| ) | ||
|
|
||
| expert_parallel_ci_job = CircleCIJob( |
There was a problem hiding this comment.
imo better to separate, it was already weird enough imo that tp and ep were used in the same context of tests
| ) | ||
|
|
||
|
|
||
| def __getattr__(name: str): |
There was a problem hiding this comment.
dang yea kind of a wear workaround but its for cases where you import from the file right? Imo ok to keep as its for BC but we should add a small comment to explain this!
| return output | ||
|
|
||
|
|
||
| if is_torch_distributed_available(): |
There was a problem hiding this comment.
maybe we should move ALL_PARALLEL_STYLES to something more general and let tp / ep define their own list => update the ALL_PARALLEL_STYLES based on both of theirs
atm it's still very tp focused and needs a more centralized point imo
| # Set to None to run distributed EP tests for every model with a plan. | ||
| # Representative MoE model types covered by distributed EP tests. | ||
| EP_DISTRIBUTED_TEST_MODEL_TYPES = { | ||
| "qwen4_exp_text", | ||
| "qwen3_moe", | ||
| "glm_moe_dsa", | ||
| "deepseek_v4", | ||
| "glm4_moe_lite", | ||
| "glm4_moe", | ||
| "olmoe", | ||
| "qwen2_moe", | ||
| "cohere2_moe", | ||
| # VLM | ||
| "glm4v_moe", | ||
| } |
There was a problem hiding this comment.
I still dont see why we cant run all for nightly at least? Its fine to have a small list for the fast CI but nightly should run all no?
- Consolidated the registration of parallel styles into the `ParallelInterface` class for better organization. - Updated the `expert_parallel.py` and `tensor_parallel.py` files to streamline the registration process of expert parallel strategies. - Enhanced documentation for clarity regarding the usage of tensor parallel strategies and their registration. - Removed redundant code related to `MoeExpertsParallel` and `MoeIdentityParallel` classes, which are now properly registered in the new structure.
| ALL_PARALLEL_STYLES.register("moe_tp_experts", MoeExpertsParallel()) | ||
| ALL_PARALLEL_STYLES.register("moe_identity_expert", MoeIdentityParallel()) | ||
| # Every rank keeps the full param, e.g. a per-tensor quantization scale | ||
| ALL_PARALLEL_STYLES.register("replicate", TensorParallelLayer()) |
There was a problem hiding this comment.
for non shardable scales f850c82
not ep specific tho
|
[For maintainers] Suggested jobs to run (before merge) run-slow: deepseek_v3, diffusion_gemma, glm4v_moe, granitemoehybrid, jamba, nemotron_h, olmoe, openai_privacy_filter, qwen3_omni_moe |
Only moving, no behaviour change.
Referencing #49429 as it needs it
What changed
distributed/expert_parallel.py: all MoE/EP styles (moe_tp_experts,moe_identity_expert,grouped_gemm,ep_router,ep_dispatch_experts,megamoe_router,megamoe_experts) andapply_expert_parallelism. The EP styles get anExpertParallelLayerbase class with the same hooks asTensorParallelLayer.distributed/parallel_utils.py: the DTensor helpers shared by both files.tensor_parallel.pykeeps only the dense styles. Still one registry,ALL_PARALLEL_STYLES;expert_parallel.pyregisters its styles into it.tests/test_expert_parallel_mixin.py:ExpertParallelTesterMixinwith thetest_ep_*tests and its own model allowlist, moved out of the TP mixin.Breaking changes 🚨
ExpertParallelLayer, notTensorParallelLayer/MoeExpertsParallel. Hooks are unchanged._use_local_dtensor_params,_AllReduceForward,_AllReduceBackwardmoved toparallel_utils.py(private, no shim).TensorParallelTesterMixinno longer carries the EP tests; addExpertParallelTesterMixinto get them.Old imports from
transformers.distributed.tensor_parallelkeep working with aFutureWarninguntil v5.23.