-
Notifications
You must be signed in to change notification settings - Fork 251
Feat: Implementation of the DeepSeek blockwise quantization for fp8 tensors #1763
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?
Feat: Implementation of the DeepSeek blockwise quantization for fp8 tensors #1763
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/1763
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 26bb079 with merge base f343336 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Thanks for running the tests. I have two questions regarding the errors:
|
Can you clarify what you mean? Are tests failing in CI due to a missing triton installation? That shouldn't be happening, please share the link/logs if so.
We just use helpers which skip tests if GPU architecture is not at least SM 89: Line 619 in f478692
You can find examples in the float8 tests (example). |
Indeed, they are. It looks like only the CPU runs are failing. I presume that bitsandbytes might not install triton when no GPU is available (I might be missing something there). Here is an instance of a failing log:
Thank you for the hint, I've locally updated the code accordingly 👍 |
W_q, W_s = fp8_blockwise_weight_quant(W, block_size, dtype) | ||
output_blockwise = blockwise_fp8_gemm(A_q, A_s, W_q, W_s) | ||
|
||
quantize_(lin, int8_dynamic_activation_int4_weight()) |
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.
why is int8_dynamic_activation_int4_weight
being used here?
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.
Thank's for noticing it. I was aiming for a static W4A8 quantization and I overlooked that it was dynamic. I will try to address this within the week.
Also @Degnel you should skip tests requiring triton if CUDA is not available. |
@Degnel thanks for your work on this, i ran the tests and it looks like your blockwise fp8 gemm test is failing due to quantization error |
Thanks for pointing that out! I had also noticed the issue, and I think I was just a bit too harsh with the threshold. I'll increase it to make it more reasonable. That said, I'll still double-check the calculations manually to ensure everything is mathematically correct. |
@danielvegamyhre I believe that everything should be alright except for the PR Label Check (I'm not sure if I have the required rights to edit this). The test-mps-ops (macos-m1-stable) failed, but I think that the merge will fix it as it seems to be a newly introduced test. |
The test-mps-ops (macos-m1-stable) failed once again. I've seen other recent PRs both succeeding and failing this test (due to the same missing package 'importlib_metadata'). I don't think this is related to the code I wrote, but I might be missing something. Please, let me know if you have any insights. |
the test mps is unrleated, re-running tests |
It seems like the new PRs are not failing anymore due to the macOS tests. Maybe we should try to rerun it here :) @danielvegamyhre @drisspg |
Sorry, could you do 1 more rebase to kick back off ci |
- fp8 triton gemm - quant, dequant and linear utils - time & precision benchmarks - basic tests
e8edea9
to
e41457c
Compare
No problem, it should be ok |
Thank you @drisspg I've made the linting |
This PR is the first step towards addressing issue #1594. It includes the following implementations:
If the code is validated, it would be great to bench it on H100.