Skip to content
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

Consolidate ZeroPointDomain.NONE & None zero point domains #1556

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sanchitintel
Copy link
Contributor

@sanchitintel sanchitintel commented Jan 13, 2025

Summary

  • Both ZeroPointDomain.NONE & None zero point domains were being used. The latter was being used for float8. This PR consolidates both & retains ZeroPointDomain.NONE
  • Using ZeroPointDomain.NONE would now produce None for zero-point
  • int8_dynamic_activation_int8_weight now uses ZeroPointDomain.NONE as weight zero point domain (as weight is quantized symmetrically to int8).

Some of the older changes in this PR (such as supporting torch.compile with optional zero_point) were rendered redundant by more recent changes in the main branch, so I removed them & modified the description accordingly. Thanks!

BC-breaking note:

A None value for zero_point_domain would no longer be accepted. Please use ZeroPointDomain.NONE instead.
A None zero-point would be generated if ZeroPointDomain.NONE would be used.

Before this PR:

None value of zero_point_domain was being used for quantization to float8 dtypes to indicate the absence of a zero-point.
ZeroPointDomain.NONE was being used for quantization to non-float8 dtypes to indicate the absence of a zero-point.
choose_qparams_affine returned a non-None zero-point for symmetric quantization.

After this PR lands:

ZeroPointDomain.NONE value for zero_point_domain should be used to denote the absence of a zero-point.
zero point in such cases would be None.

APIs affected:

For the following user-facing APIs, a None zero_point_domain can no longer be used with the following APIs (ZeroPointDomain.NONE should be used instead of None) -

torchao.quantization.quant_api.int4_weight_only

zero_point_domain is no longer optional for the following developer-facing APIs -

torchao.quantization.quant_primitives.choose_qparams_affine
torchao.quantization.quant_primitives.choose_qparams_affine_with_min_max
torchao.quantization.quant_primitives.fake_quantize_affine_cachemask
torchao.quantization.quant_primitives.quantize_affine
torchao.quantization.observer.AffineQuantizedObserverBase
torchao.dtypes.uintx.marlin_qqq_tensor.MarlinQQQTensor.from_hp_to_intx
torchao.dtypes.AffineQuantizedTensor.from_hp_to_intx
torchao.dtypes.AffineQuantizedTensor.from_hp_to_intx_static

For the following developer-facing APIs, a None zero_point_domain can no longer be used with the following APIs (ZeroPointDomain.NONE should be used instead of None) -

torchao.quantization.quant_primitives.fake_quantize_affine
torchao.quantization.quant_primitives.dequantize_affine
torchao.quantization.qat.FakeQuantizeConfig

For the following internal APIs (whose names begin with an underscore character), zero_point_domain can no longer be None (ZeroPointDomain.NONE should be used instead of None):

torchao.quantization.qat._ToAffineFakeQuantized

Copy link

pytorch-bot bot commented Jan 13, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/1556

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit cf90998 with merge base 47f96f1 (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 13, 2025
@sanchitintel sanchitintel force-pushed the zero_point_domain_none branch from 473c42f to c53a9d5 Compare January 13, 2025 23:26
Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sanchitintel
Copy link
Contributor Author

Thanks for reviewing, @jerryzh168! I'll fix the lint error once other CI checks would complete.

@sanchitintel

This comment was marked as outdated.

@sanchitintel
Copy link
Contributor Author

The UTs that failed are failing because of some Inductor codegen issues that have since been fixed, but the CI jobs used PyTorch v2.4. I'll skip some of those UTs, so that they'd run with PyTorch v2.5 & beyond instead.

Thanks!

@jerryzh168
Copy link
Contributor

for https://github.com/pytorch/ao/actions/runs/12760810579/job/35567540271?pr=1556 you can install pre-commit

pip install pre-commit

and run it:

pre-commit run

then it will run the formatting before every git commit

@jerryzh168 jerryzh168 added topic: bug fix Use this tag for PRs that fix bugs topic: improvement Use this tag if this PR is an improvement (doesn't fit into any of the other categories) and removed topic: bug fix Use this tag for PRs that fix bugs labels Jan 15, 2025
@sanchitintel sanchitintel force-pushed the zero_point_domain_none branch from 837ee22 to c44df1f Compare January 15, 2025 06:09
@sanchitintel sanchitintel marked this pull request as ready for review January 15, 2025 06:26
@sanchitintel sanchitintel force-pushed the zero_point_domain_none branch from c83f470 to da2e9e0 Compare January 15, 2025 06:48
@sanchitintel sanchitintel changed the title Fix ZeroPointDomain.NONE support & make it default for da8w8 weights Consolidate ZeroPointDomain.NONE & None zero point domains Jan 15, 2025
test/integration/test_integration.py Show resolved Hide resolved
test/integration/test_integration.py Outdated Show resolved Hide resolved
torchao/quantization/quant_primitives.py Outdated Show resolved Hide resolved
@@ -199,7 +200,6 @@ def test_linear_observer_tensor(self, observe_weight: bool):
input_scale.item(),
max_val / max_fp8,
)
self.assertIsNotNone(input_zero_point)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a change of behavior when you change zero_point_domain for None to ZeroPointDomain.NONE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, input_zero_point would now be None. So, instead of removing that line, I now added self.assertIsNone(input_zero_point). Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so what is the meaning of zero_point_domain == None before?

Copy link
Contributor Author

@sanchitintel sanchitintel Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some APIs/implementations were creating/expecting a None zero_point when zero_point_domain ZeroPointDomain.NONE or None was used, while choose_qparams_affine was not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jerryzh168, is it possible that some torchao users' code may be expecting a non-None zero_point with zero_point_domain ZeroPointDomain.NONE/None, making this change BC-breaking for them? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that most usages of this function are internal to torchao so that its okay to BC break, you can add the label just to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your advice, @drisspg!
Could you please help add such a label as GitHub isn't displaying an option to me for adding it? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bc-breaking label, please also write a bc-breaking note similar to #1049

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, @jerryzh168! I added a note & rebased the PR.

Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please rebase and land

@jerryzh168 jerryzh168 mentioned this pull request Jan 26, 2025
3 tasks
@jerryzh168 jerryzh168 added the topic: bc-breaking Use this tag if this PR breaks backward compatibility label Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: bc-breaking Use this tag if this PR breaks backward compatibility topic: improvement Use this tag if this PR is an improvement (doesn't fit into any of the other categories)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants