-
Notifications
You must be signed in to change notification settings - Fork 661
[Bugfix] Fix precision issues in moe_mlp (vllm-ascend main) #5025
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
[Bugfix] Fix precision issues in moe_mlp (vllm-ascend main) #5025
Conversation
Signed-off-by: tanqingshan (A) <[email protected]> Signed-off-by: tanqingshan (A) <[email protected]>
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.
Code Review
This pull request correctly fixes a bug in the cumsum_group_list function by changing group_diff[0] to group_list[0]. This ensures the logic for converting a cumulative sum list to a difference list is correct. The change is accurate and addresses the intended issue. I've also added a comment to handle a potential IndexError on an empty tensor, which would improve the robustness of the function.
| if src_list_type == 0 and dst_list_type == 1: | ||
| group_diff = torch.diff(group_list) | ||
| new_group = torch.cat([group_diff[0].unsqueeze(0), group_diff], dim=0) | ||
| new_group = torch.cat([group_list[0].unsqueeze(0), group_diff], dim=0) | ||
| return new_group |
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.
There's a potential IndexError here if group_list is an empty tensor. Accessing group_list[0] on line 49 would cause a crash. It's good practice to handle this edge case, for example by checking if the tensor is empty before proceeding.
| if src_list_type == 0 and dst_list_type == 1: | |
| group_diff = torch.diff(group_list) | |
| new_group = torch.cat([group_diff[0].unsqueeze(0), group_diff], dim=0) | |
| new_group = torch.cat([group_list[0].unsqueeze(0), group_diff], dim=0) | |
| return new_group | |
| if src_list_type == 0 and dst_list_type == 1: | |
| if not group_list.numel(): | |
| return group_list | |
| group_diff = torch.diff(group_list) | |
| new_group = torch.cat([group_list[0].unsqueeze(0), group_diff], dim=0) | |
| return new_group |
|
👋 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. |
ea908c0 to
0528d70
Compare
What this PR does / why we need it?
Use group_list[0] to replace group_diff[0] in function "cumsum_group_list" (moe_mlp.py).
The purpose is to modify it to the correct logic of converting cumsum to count.
Does this PR introduce any user-facing change?
No