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

[quant][fx] store dtype, axis as literals in the graph #54624

Closed
wants to merge 1 commit into from

Conversation

supriyar
Copy link
Contributor

@supriyar supriyar commented Mar 24, 2021

Stack from ghstack:

Summary:
previously we were creating setattr nodes for dtype and axis.
The FX convention is that primitive types are embedded as literals in args/kwargs.

With this change we won't see getattr nodes in the graph anymore for dtype/axis

Test Plan:
python test/test_quantization.py TestQuantizeFx

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D27306898

Summary:
previously we were creating setattr nodes for dtype and axis.
The FX convention is that primitive types are embedded as literals in args/kwargs.

With this change we won't see getattr nodes in the graph anymore for dtype/axis

Test Plan:
python test/test_quantization.py TestQuantizeFx

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 24, 2021

💊 CI failures summary and remediations

As of commit 574efc3 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

supriyar added a commit that referenced this pull request Mar 24, 2021
Summary:
previously we were creating setattr nodes for dtype and axis.
The FX convention is that primitive types are embedded as literals in args/kwargs.

With this change we won't see getattr nodes in the graph anymore for dtype/axis

Test Plan:
python test/test_quantization.py TestQuantizeFx

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: b557e35c1303fe624d6cdaf5011d65e702c9e509
Pull Request resolved: #54624
@jerryzh168
Copy link
Contributor

do we need "dtype" to appear in state_dict?

@supriyar
Copy link
Contributor Author

do we need "dtype" to appear in state_dict?

I don't think so. I think this change existed even before we added support for scale/zero_point to appear in the state_dict. Since we never call register_buffer for dtype, it was never part of state_dict.

@jerryzh168
Copy link
Contributor

do we need "dtype" to appear in state_dict?

I don't think so. I think this change existed even before we added support for scale/zero_point to appear in the state_dict. Since we never call register_buffer for dtype, it was never part of state_dict.

so only scale/zero_point need to be loaded from state_dict, dtype is fixed?

@supriyar
Copy link
Contributor Author

do we need "dtype" to appear in state_dict?

I don't think so. I think this change existed even before we added support for scale/zero_point to appear in the state_dict. Since we never call register_buffer for dtype, it was never part of state_dict.

so only scale/zero_point need to be loaded from state_dict, dtype is fixed?

Yes, the dtype information is retrieved from the qconfig, so it doesn't need to be stored in the state_dict. As long as we use the same config at load time, it should match.

@jerryzh168
Copy link
Contributor

do we need "dtype" to appear in state_dict?

I don't think so. I think this change existed even before we added support for scale/zero_point to appear in the state_dict. Since we never call register_buffer for dtype, it was never part of state_dict.

so only scale/zero_point need to be loaded from state_dict, dtype is fixed?

Yes, the dtype information is retrieved from the qconfig, so it doesn't need to be stored in the state_dict. As long as we use the same config at load time, it should match.

I see, sounds good.

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #54624 (574efc3) into gh/supriyar/220/base (12a61a1) will decrease coverage by 0.39%.
The diff coverage is 100.00%.

@@                   Coverage Diff                    @@
##           gh/supriyar/220/base   #54624      +/-   ##
========================================================
- Coverage                 77.47%   77.08%   -0.40%     
========================================================
  Files                      1893     1893              
  Lines                    185921   185918       -3     
========================================================
- Hits                     144043   143307     -736     
- Misses                    41878    42611     +733     

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in a0a7a2d.

@facebook-github-bot facebook-github-bot deleted the gh/supriyar/220/head branch April 1, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants