-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Bugfix][Mamba] - Fix Conv State Kernel FP32 Support #24883
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
Signed-off-by: asafg <39553475+Josephasafg@users.noreply.github.com>
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 addresses a Triton compilation error for Mamba's FP32 convolution state during prefill by casting the input tensor to match the state's data type. The fix in causal_conv1d_fn
is appropriate. However, the same bug likely exists in the causal_conv1d_update
function, which handles the decode path, and this has not been addressed. This could lead to failures during token generation after the prefill phase. I've added a critical comment highlighting this omission. The test enhancements are good, improving coverage for different cache dtype parameters.
Signed-off-by: asafg <39553475+Josephasafg@users.noreply.github.com>
This pull request has merge conflicts that must be resolved before it can be |
fe6756e
to
bfdaf6f
Compare
@heheda12345 @tdoublep Can I please get a review? |
original_x_dtype = x.dtype | ||
x = x.to(conv_state.dtype) |
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.
Do we definitely want to cast x
to the conv_state dtype
, rather than casting conv_state
to the x_dtype
?
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.
Its a good question - Since the user picks fp32 for the cache type, Im afraid that by downcasting it to fp16 and then back to fp32, we could lose accuracy doing it. I had also figured that by choosing fp32, we want the computations to be done in that type dont we?
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.
How does it work for the SSM state? I guess we want it to be consistent.
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.
we cast to float in the kernel
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.
LGTM
Purpose
There was a bug in the FP32 support recently added to mamba conv state where if you pass this arg
--mamba-cache-dtype "float32"
you'd get this error:The kernel needed to cast the input type to be the same as conv state.
Test Plan
Added two tests to test_hybrid.py as they were missing. Tests pass
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.