Top9 #4760
Top9 #4760
Conversation
Signed-off-by: Che Ruan <[email protected]>
Signed-off-by: Che Ruan <[email protected]>
Signed-off-by: Che Ruan <[email protected]>
Signed-off-by: Che Ruan <[email protected]>
Signed-off-by: Mercykid-bash <[email protected]>
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mix_placement feature for Mixture-of-Experts models, primarily targeting DeepseekV2/V3 on Ascend hardware. The changes involve modifications to configuration, expert selection logic, and weight loading patches. While the overall direction seems correct for enabling this new functionality, I've identified a critical bug in the expert group calculation within moe_mlp.py that will lead to incorrect behavior. I've also pointed out a high-severity maintainability issue related to a hardcoded magic number in the expert selection logic. Please address these points to ensure correctness and code quality.
vllm_ascend/ops/fused_moe/moe_mlp.py
Outdated
| group_diff = torch.diff(group_list) | ||
| new_group = torch.cat([group_diff[0].unsqueeze(0), group_diff], dim=0) |
There was a problem hiding this comment.
There is a bug in the calculation of new_group. When group_list is a cumulative sum of token counts, this logic incorrectly calculates the token count for the first group. For example, if group_list is [10, 25, 30], the token counts per group are [10, 15, 5]. The current code produces [15, 15, 5], using the second group's count for the first group. Additionally, this will raise an IndexError if group_list has fewer than two elements.
The correct approach is to combine the first element of group_list with the differences of the rest of the list.
| group_diff = torch.diff(group_list) | |
| new_group = torch.cat([group_diff[0].unsqueeze(0), group_diff], dim=0) | |
| group_diff = torch.diff(group_list) | |
| new_group = torch.cat([group_list[:1], group_diff]) |
| device=topk_ids.device) | ||
|
|
||
| pad_shared_expert_weights = torch.full((topk_weights.shape[0], 1), | ||
| 0.4, |
There was a problem hiding this comment.
The value 0.4 is hardcoded as the weight for the padded shared expert. This is a magic number which harms readability and maintainability. It should be defined as a named constant with a descriptive name at the top of the file (e.g., SHARED_EXPERT_DEFAULT_WEIGHT), or passed as a parameter if it's meant to be configurable.
| self.expert_load_balancer.check_expert_map_tensor() | ||
| self.global_redundant_expert_num = ( | ||
| self.expert_load_balancer.get_global_redundant_expert_num()) | ||
| # self.global_redundant_expert_num = ( |
There was a problem hiding this comment.
Remove unused code instead of commenting it out
| if self._shared_experts is None: | ||
| shared_out = None | ||
| else: | ||
| shared_out = self._shared_experts(hidden_states) |
There was a problem hiding this comment.
This can be condensed into a one-liner: shared_out = self._shared_experts(hidden_states) if self._shared_experts is not None else None.
| fused_moe_out = self.experts( | ||
| hidden_states=hidden_states, router_logits=router_logits | ||
| ) | ||
| ascend_config = get_ascend_config() |
| if self.shared_experts is None: | ||
| assert shared_output is None |
There was a problem hiding this comment.
Using a bare assert doesn't provide enough context for debugging if this fails. Please replace this with an explicit check that raises an exception with a descriptive error message.
Signed-off-by: Mercykid-bash <[email protected]>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Mercykid-bash <[email protected]>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
4 participants
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?