Repository navigation
🚨 Remove the ep_router expert-parallel style
#49615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,7 +553,7 @@ class MoeExpertsParallel(TensorParallelLayer): | |
| def should_use_local_tensors(self, module): | ||
| return True | ||
|
|
||
| def transform_inputs_pre_forward(self, module, args, kwargs, mesh, *, is_expert_parallel=False): | ||
| def transform_inputs_pre_forward(self, module, args, kwargs, mesh): | ||
| hidden_states, *routing_args = args | ||
| tp_group = mesh.get_group() if mesh.ndim == 1 else mesh.get_group("tp") | ||
| if isinstance(hidden_states, DTensor): | ||
|
|
@@ -564,46 +564,15 @@ def transform_inputs_pre_forward(self, module, args, kwargs, mesh, *, is_expert_ | |
| top_k_index, top_k_weights, *extra_args = routing_args | ||
| if isinstance(top_k_weights, DTensor): | ||
| top_k_weights = top_k_weights.to_local() | ||
| if not is_expert_parallel: | ||
| top_k_weights = _AllReduceBackward.apply(top_k_weights, tp_group) | ||
| top_k_weights = _AllReduceBackward.apply(top_k_weights, tp_group) | ||
| routing_args = [top_k_index, top_k_weights, *extra_args] | ||
|
|
||
| return (hidden_states, *routing_args), kwargs | ||
|
|
||
| def install_forward(self, module, mesh, *, is_expert_parallel=False): | ||
| """Install the transforms but pass `is_expert_parallel` in the forward call.""" | ||
| original_forward = module.forward | ||
| output_source = ( | ||
| Partial() | ||
| if any( | ||
| isinstance(param, DTensor) and any(not placement.is_replicate() for placement in param.placements) | ||
| for param in module.parameters() | ||
| ) | ||
| else Replicate() | ||
| ) | ||
|
|
||
| def tp_forward(*args, **kwargs): | ||
| args, kwargs = self.transform_inputs_pre_forward( | ||
| module, args, kwargs, mesh, is_expert_parallel=is_expert_parallel | ||
| ) | ||
| with self.context_around_forward(module, mesh): | ||
| output = original_forward(*args, **kwargs) | ||
| return self.transform_output_post_forward(module, output, mesh, source=output_source) | ||
|
|
||
| module.forward = tp_forward | ||
| return module | ||
|
|
||
| def transform_output_post_forward(self, module, output, mesh, source=None): | ||
| def transform_output_post_forward(self, module, output, mesh): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we even need this override anymore?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the all-reduce itself is still needed: the base |
||
| if output is None: | ||
| return None | ||
|
|
||
| has_sharded_parameters = any( | ||
| isinstance(param, DTensor) and any(not placement.is_replicate() for placement in param.placements) | ||
| for param in module.parameters() | ||
| ) | ||
| if not has_sharded_parameters: | ||
| return output | ||
|
|
||
| # each rank ran its shard of every expert: sum the partial outputs | ||
| process_group = mesh.get_group() if mesh.ndim == 1 else mesh.get_group("tp") | ||
| return _AllReduceForward.apply(output, process_group) | ||
|
|
||
|
|
@@ -718,7 +687,6 @@ def gather_state_dict_for_save( | |
| # Lazy import because expert_parallel.py imports from this file (circular import otherwise). | ||
| _MOVED_TO_EXPERT_PARALLEL = ( | ||
| "MoEParamShard", | ||
| "EpRouterParallel", | ||
| "EpDispatchExpertsParallel", | ||
| "apply_expert_parallelism", | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toep_dispatch_experts/ep_implementation="all_reduce". added it to the breaking changes in the description, with a test