Repository navigation
Check quantized embedding qparam dtypes against how the kernels read them - #23912
Open
Shakhtar-Sankur wants to merge 1 commit into
Open
Shakhtar-Sankur wants to merge 1 commit into
Shakhtar-Sankur wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23912
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 460b8b9 with merge base d1f7f1f ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Contributor
Author
|
@pytorchbot label "release notes: ops & kernels" |
Contributor
|
@Shakhtar-Sankur could you rebase and ping for review? Thanks |
…them The reference ops (exir/passes/_quant_patterns_and_replacements.py) require weight_zero_points to have the same dtype as weight_scales, and the variants without a dtype argument return the scales' dtype. The kernels instead read the zero points as the scales' type but checked them against the output dtype, and the .out variants read the scales as the output type without checking it. As a result, embedding_byte/2bit/4bit.dtype_out aborted on fp16 scales and zero points with a fp32 output, which the reference accepts, and accepted fp32 zero points with fp16 scales, reading them as fp16. The .out variants with fp16 scales and a fp32 output read past the end of the scales. Check the zero points against the scales, and in the .out variants check the scales against the output. The new tests for the 8-bit and 4-bit ops (2-bit shares the 4-bit code) fail without the change.
Shakhtar-Sankur
force-pushed
the
quantized-embedding-qparam-dtypes
branch
from
October 21, 2026 19:17
f79810b to
460b8b9
Compare
Contributor
Author
|
@nil-is-all rebased onto main (it now sits on top of #23677), thanks. @JakeStevens @metascroy could you take a look when you have a moment? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The reference ops in exir/passes/_quant_patterns_and_replacements.py require weight_zero_points to share weight_scales' dtype, and the variants without a dtype argument return the scales' dtype. The kernels checked the zero points against the output dtype instead (while reading them as the scales' type), and the .out variants read the scales as the output type without checking it. So:
This checks the zero points against the scales, and the scales against the output in the .out variants.
Related to #23677 (index bounds check in the same kernels); the two changes are independent and merge cleanly in either order.
Test plan
Built quantized_kernels_test with CMake (Debug, -fsanitize=address).
Authored with AI assistance. I reviewed the change and ran the tests myself.
cc @larryliu0820 @manuelcandales @JakeStevens @nil-is-all