-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
[Bugfix] Fix topk_ids indices_type for CUTLASS w8a8 FP8 MoE #20166
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
6d0e19a
7c57bb0
376bcde
6ca83a4
ec668c3
beeba9a
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 |
---|---|---|
|
@@ -81,7 +81,7 @@ def max_num_tokens_per_rank(self) -> Optional[int]: | |
return self.max_num_tokens | ||
|
||
def topk_indices_dtype(self) -> Optional[torch.dtype]: | ||
return torch.uint32 | ||
return torch.int32 | ||
|
||
def prepare( | ||
self, | ||
|
@@ -100,7 +100,9 @@ def prepare( | |
hidden_dim = a1.size(-1) # K | ||
|
||
assert topk_ids.size(0) == num_tokens | ||
# assert expert_map is None, "NYI" | ||
assert expert_map is None, """with expert map, -1 id is used for | ||
non-local token; this causes error when casting ids to the | ||
topk_indices_dtype() uint32""" | ||
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. Added assertion and changed the id type here. test_pplx_moe.py passes. Let me know if we should run a model e2e and how. Thx! @tylertitsworth @bnellnm cc @yeqcharlotte 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. Looking at this again, I don't think we need both the assertion and int32 change? 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. Hi @minosfuture Could we lift this assert ? I think
I believe a better solution is to make fyi @tlrmchlsmth 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. let me raise a PR for this! thanks for the catch. 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. would it be problematic to clear expert_map assigning None to it? 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 function doesn't use the expert_map - so overriding it to None should be fine as it'd prevent any incorrect use.
I actually dont see it being used in any of the implementations. I think it is okay to remove the argument altogether. 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. parameter is needed for this is an inherited function. I commented it all out. This essentially reverts the changes here. As a followup, I think we should fix where it passes an expert_map, and add the assertion back. #20714 @varun-sundar-rabindranath @tlrmchlsmth pls help approve this fix pr and auto-merge it. thx! 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.
I was suggesting we could remove the var in the base-class as I don't see any implementation using it. |
||
|
||
# Is this always going to be a1.device? | ||
device = a1.device | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -883,23 +883,27 @@ def apply( | |
num_expert_group=num_expert_group, | ||
custom_routing_function=custom_routing_function, | ||
scoring_func=scoring_func, | ||
e_score_correction_bias=e_score_correction_bias, | ||
indices_type=self.topk_indices_dtype, | ||
) | ||
e_score_correction_bias=e_score_correction_bias) | ||
|
||
a1_scale = layer.w13_input_scale | ||
a2_scale = layer.w2_input_scale | ||
per_act_token = a1_scale.numel() != 1 if a1_scale is not None else ( | ||
a2_scale.numel() != 1 if a2_scale is not None else False) | ||
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. this is a fix needed after rebase #19636. cc @bnellnm @luccafong |
||
|
||
return self.fused_experts( | ||
x, | ||
layer.w13_weight, | ||
layer.w2_weight, | ||
topk_weights, | ||
topk_ids, | ||
per_act_token=per_act_token, | ||
activation=activation, | ||
global_num_experts=global_num_experts, | ||
expert_map=None if self.disable_expert_map else expert_map, | ||
w1_scale=layer.w13_weight_scale, | ||
w2_scale=layer.w2_weight_scale, | ||
a1_scale=layer.w13_input_scale, | ||
a2_scale=layer.w2_input_scale, | ||
a1_scale=a1_scale, | ||
a2_scale=a2_scale, | ||
) | ||
|
||
|
||
|
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.
note @tlrmchlsmth @varun-sundar-rabindranath - this appears to have broken the PPLX backend as the pplx dispatch function expects uint32
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.
VLLM_ALL2ALL_BACKEND="pplx" vllm serve Qwen/Qwen3-30B-A3B-FP8 --data-parallel-size 2 --enable-expert-parallel --enforce-eager
result:
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.
@robertgshaw2-redhat that will be fixed by #20825
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.
okay cool - just a note: I was using triton+pplx in this case
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.
Sorry for the regression! I'll make sure to test it next time. We are also working on improving CI coverage and preparing a script to cover major models for easy local test.