Add int1 to int7 dtypes#136301
Conversation
Summary: Similar to #117208, we want to add int1 to int7 for edge use cases for weight quantization Test Plan: python test/test_quantization.py -k test_uint4_int4_dtype Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/136301
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit c42b2de with merge base 86631ec ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Summary: Similar to #117208, we want to add int1 to int7 for edge use cases for weight quantization Test Plan: python test/test_quantization.py -k test_uint4_int4_dtype Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
|
Can you explain in more detail why the signed types are desirable? |
|
@ezyang it's used in edge low bit kernels to represent weight: https://www.internalfb.com/diff/D62464487 TBH, I think it we can extend torch.dtype out of core, it's probably better, but my impression is that we can't do that right now |
Summary: Similar to #117208, we want to add int1 to int7 for edge use cases for weight quantization (https://www.internalfb.com/diff/D62464487) Test Plan: python test/test_quantization.py -k test_uint4_int4_dtype Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
|
actually we can delay this PR I think, since we have been uint8 right now for uintx as well: https://github.com/pytorch/ao/blob/53b6b787bb035f1abc3c4804dd22c09ee41e7a0d/torchao/dtypes/uintx/uintx.py#L66, we just need to add some placeholder dtypes in torchao in this case, although it is more convenient to be able to do |
|
It's coherent to support this type but it's pretty unlikely for, e.g., signed int1 to be a useful concept, you would only have the elements -1 and 0. So I would like at least like a little more concrete evidence you can't use the uint types instead |
|
@ezyang you mean just for int1 or other intx dtypes as well? I agree int1 is probably not very useful. but cc @metascroy and also @kimishpatel to see if there are more context here for the need intx dtypes (especially int1). |
|
For the larger int types, it seems to me that the biggest benefit you get over unsigned is access to signed operations. But this also isn't obviously that helpful; for example, in two's complement, you can implemented signed addition... as regular unsigned addition. Obviously, exposing signed integers as a user facing abstraction is a legitimate thing to do, but my understanding of AO's use case is they would wrapped up in a tensor subclass anyway... |
| case ScalarType::UInt5: | ||
| case ScalarType::UInt6: | ||
| case ScalarType::UInt7: | ||
| return true; |
There was a problem hiding this comment.
This seems like a bug before this pr?
| assert quantized_tensor.int_repr().min().item() == q8_min | ||
|
|
||
| def test_uint1_7_dtype(self): | ||
| def test_uint4_int4_dtype(self): |
There was a problem hiding this comment.
what happened to the original test case?
There was a problem hiding this comment.
naming was a bit off, original test is testing uint4, and I added int4 in the PR
|
@ezyang so in general it is not clear why unsigned is treated different than signed? We have had this issue with uint16. So just from UX perspective it seems like an aberration that we have uintx types but not intx.
I think this is fair and I am not sure for the kernels we actually need intx/uintx types at all if kernels are tied with quantization API. Meaning if we did not use AO API, and relied purely on exported IR to derive if some tensors are uintx/intx, then I think having this natively available in PyTorch makes sense. But in general @ezyang what would be the objection given uintx are already present. |
yes all the operations are implemented with tensor subclass (currently just pack/unpack to/from uint8),
main benefit right now is that we are using torch.uintx/torch.intx as user facing API so people don't need to maybe use ( we are thinking of implementing tensor subclasses a bit later, since we need a "standard"/"default" packing format, and we want to have an efficient kernel for these before we decide on that |
Summary: Similar to #117208, we want to add int1 to int7 for edge use cases for weight quantization (https://www.internalfb.com/diff/D62464487) Test Plan: python test/test_quantization.py -k test_uint4_int4_dtype Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]
|
@ezyang any ideas on the CI failure (https://github.com/pytorch/pytorch/actions/runs/11026420870/job/30727936288?pr=136301)? I can't repro locally |
|
looks like master problem |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
@pytorchbot revert -m "causing internal failures" -c "ghfirst" |
|
@pytorchbot successfully started a revert job. Check the current status here. |
|
@jerryzh168 your PR has been successfully reverted. |
This reverts commit bfa16a1. Reverted #136301 on behalf of https://github.com/PaliC due to causing internal failures ([comment](#136301 (comment)))
This reverts commit bfa16a1. Reverted pytorch#136301 on behalf of https://github.com/PaliC due to causing internal failures ([comment](pytorch#136301 (comment)))
|
landed in #137928 |
Stack from ghstack (oldest at bottom):
Summary:
Similar to #117208, we want to add int1 to int7 for edge use cases
for weight quantization (https://www.internalfb.com/diff/D62464487)
Test Plan:
python test/test_quantization.py -k test_uint4_int4_dtype
Reviewers:
Subscribers:
Tasks:
Tags: