-
Notifications
You must be signed in to change notification settings - Fork 609
【fix】ops gatingtopk fix nightly ci error #4340
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
base: main
Are you sure you want to change the base?
Conversation
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 fixes a potential TypeError in _select_experts_with_fusion_ops by providing default values for topk_group and num_expert_group. My review focuses on a subtle change in behavior this introduces. While the fix is correct for the bug, it alters how softmax scoring handles expert grouping. I've left a comment with a suggestion to preserve the original behavior if the change was unintentional.
| topk_group = topk_group if topk_group is not None else 1 | ||
| num_expert_group = num_expert_group if num_expert_group is not None else 1 | ||
| norm_type = 0 if scoring_func == "softmax" else 1 |
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 change correctly provides default values for topk_group and num_expert_group, fixing a potential TypeError. However, it also alters the behavior for scoring_func="softmax". The original code always disabled grouping for softmax by forcing topk_group=1 and num_expert_group=1. This change enables grouping for softmax if grouping parameters are provided.
If this was unintentional, the suggested change below preserves the original behavior for softmax while still fixing the bug for other cases. If enabling grouping for softmax was intended, feel free to disregard this suggestion.
if scoring_func == "softmax":
norm_type = 0
topk_group = 1
num_expert_group = 1
else:
norm_type = 1
topk_group = topk_group if topk_group is not None else 1
num_expert_group = num_expert_group if num_expert_group is not None else 1|
👋 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. |
0f52a8d to
98515c1
Compare
5657b4d to
6616fb4
Compare
d48da64 to
f08bb5f
Compare
Signed-off-by: 1092626063 <1092626063@qq.com>
f08bb5f to
e4bdda5
Compare
What this PR does / why we need it?
ops gatingtopk fix nightly ci error
Does this PR introduce any user-facing change?
How was this patch tested?