-
Notifications
You must be signed in to change notification settings - Fork 645
Revert "[cherry-pick][refactor]support gatingtopk operator generalization (#4050)" #4352
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
Conversation
…tion (vllm-project#4050)" This reverts commit c87a77e. Signed-off-by: wangxiyuan <[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. |
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 reverts a previous change that generalized the gatingtopk operator, which was found to break end-to-end tests. The revert restores the previous, more specific implementation across vllm_ascend/ops/moe/experts_selector.py and related tests. The changes appear to correctly revert the problematic commit. I have one suggestion on the restored code in experts_selector.py to improve its robustness and prevent a potential bug where logic blocks could unintentionally overwrite each other.
| # y2_flag=False, # old api; should the third output be output | ||
| routed_scaling_factor=1, | ||
| eps=float(1e-20)) | ||
| if not use_grouped_topk and custom_routing_function is None and scoring_func == "softmax": |
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.
The if statement on this line could potentially conflict with the preceding if is_deepseek_v3_r1: block. If conditions for both blocks are met (e.g., for a deepseek_v3_r1 model with scoring_func="softmax"), the results from the first block will be overwritten. This could lead to incorrect expert selection. Using elif would make these conditions mutually exclusive and prevent this potential bug.
| if not use_grouped_topk and custom_routing_function is None and scoring_func == "softmax": | |
| elif not use_grouped_topk and custom_routing_function is None and scoring_func == "softmax": |
|
ops test should be updated |
…tion (vllm-project#4050)" (vllm-project#4352) This reverts commit c87a77e. it breaks ops e2e test Signed-off-by: wangxiyuan <[email protected]> Signed-off-by: 刘哲续 <[email protected]>
…tion (vllm-project#4050)" (vllm-project#4352) This reverts commit c87a77e. it breaks ops e2e test Signed-off-by: wangxiyuan <[email protected]>
This reverts commit c87a77e.
it breaks ops e2e test