Skip to content

Quantization: preserving pre and post forward hooks #37233

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.

Sign up for GitHub

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

Jump to bottom
Closed
zetyquickly wants to merge 1 commit into from
Closed

Quantization: preserving pre and post forward hooks #37233

zetyquickly wants to merge 1 commit into from

Conversation

@zetyquickly
Copy link
Contributor

@zetyquickly zetyquickly commented Apr 24, 2020 •
edited
Loading

  1. While do convert() preserve module's pre and post forward hooks
  2. While do fusion preserve only module's pre forward hooks (because after fusion output no longer the same)

@dr-ci
Copy link

dr-ci bot commented Apr 24, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit ce7aae3 (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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 54 times.

@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch from 305339e to 93540f8 Compare April 24, 2020 14:45
@zetyquickly zetyquickly mentioned this pull request Apr 24, 2020
Closed
@mrshenli mrshenli added the oncall: quantization Quantization support in PyTorch label Apr 27, 2020
@mrshenli mrshenli requested a review from raghuramank100 April 27, 2020 19:36
@ngimel ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 5, 2020
@jerryzh168
Copy link
Contributor

jerryzh168 commented May 20, 2020

Could you add some tests in quantization/tests_quantize.py?

@zetyquickly
Copy link
Contributor Author

zetyquickly commented May 20, 2020

@jerryzh168
Yes, I thought about that. Will do in a while

@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch 4 times, most recently from 205fdda to ef1fdb7 Compare May 22, 2020 17:28
@zetyquickly
Copy link
Contributor Author

zetyquickly commented May 22, 2020

@jerryzh168
Added three test:

  • On fusion. To check whether base models forward pre hooks move to parent which is fused module. (Base means first in fused list Conv in [Conv, BN, ReLU] for example)
  • Two test on preserving hooks while QAT and post-training statis is done

@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch 2 times, most recently from 25190b2 to 6e535ba Compare May 22, 2020 17:38
jerryzh168
jerryzh168 reviewed Jun 9, 2020
View reviewed changes
@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch from 6e535ba to 0d4987a Compare June 10, 2020 01:25
jerryzh168
jerryzh168 reviewed Jun 10, 2020
View reviewed changes
jerryzh168
jerryzh168 reviewed Jun 10, 2020
View reviewed changes
jerryzh168
jerryzh168 reviewed Jun 10, 2020
View reviewed changes
jerryzh168
jerryzh168 reviewed Jun 10, 2020
View reviewed changes
jerryzh168
jerryzh168 reviewed Jun 10, 2020
View reviewed changes
@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch 2 times, most recently from 519cf47 to 5e9ad51 Compare June 10, 2020 20:24
@zetyquickly zetyquickly requested a review from jerryzh168 June 10, 2020 20:31
@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch from d1d028a to 493fac9 Compare June 10, 2020 22:24
jerryzh168
jerryzh168 approved these changes Jun 10, 2020
View reviewed changes
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.

Looks good, thanks!

raghuramank100
raghuramank100 reviewed Jun 11, 2020
View reviewed changes
test/quantization/test_quantize.py Outdated
Copy link
Contributor

@raghuramank100 raghuramank100 Jun 11, 2020

Choose a reason for hiding this comment

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

Can you also add a test for quantize_dynamic() API?

Copy link
Contributor Author

@zetyquickly zetyquickly Jun 11, 2020

Choose a reason for hiding this comment

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

yes, sure. done

Copy link
Contributor Author

@zetyquickly zetyquickly Jun 17, 2020

Choose a reason for hiding this comment

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

@raghuramank100 ping :)

raghuramank100
raghuramank100 suggested changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@raghuramank100 raghuramank100 left a comment

Choose a reason for hiding this comment

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

Request additional tests, thanks for the PR!

@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch 3 times, most recently from efa6f70 to 4063e8b Compare June 11, 2020 09:14
@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch from 4063e8b to c8de10d Compare June 21, 2020 16:38
@zetyquickly zetyquickly requested a review from jerryzh168 June 26, 2020 10:34
jerryzh168
jerryzh168 approved these changes Jul 8, 2020
View reviewed changes
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.

lg

facebook-github-bot
facebook-github-bot reviewed Jul 8, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jerryzh168 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jerryzh168
Copy link
Contributor

jerryzh168 commented Jul 10, 2020

got this error: class TestPostTrainingStatic(QuantizationTestCase):
File "/mnt/xarfuse/uid-30041/e9a1e41e-ns-4026533223/quantization/test_quantize.py", line 448, in TestPostTrainingStatic
@override_qengines
NameError: name 'override_qengines' is not defined, could you import override_qengines from common_quantization.py?

@zetyquickly
Quantization: preserving pre and post forward hooks
ce7aae3 
1. While do convert() preserve module's pre and post forward hooks
2. While do fusion preserve the first module's pre forward hooks and the last modules's post forward hooks
@zetyquickly zetyquickly force-pushed the zetyquickly/preserve-hooks branch from c8de10d to ce7aae3 Compare July 12, 2020 21:54
@zetyquickly
Copy link
Contributor Author

zetyquickly commented Jul 13, 2020

got this error: class TestPostTrainingStatic(QuantizationTestCase):
File "/mnt/xarfuse/uid-30041/e9a1e41e-ns-4026533223/quantization/test_quantize.py", line 448, in TestPostTrainingStatic
@override_qengines
NameError: name 'override_qengines' is not defined, could you import override_qengines from common_quantization.py?

@jerryzh168 Fixed that. I've lost this during rebase somehow

facebook-github-bot
facebook-github-bot reviewed Jul 13, 2020
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@jerryzh168 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 13, 2020

@jerryzh168 merged this pull request in 0c77bd7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@jerryzh168 jerryzh168 jerryzh168 approved these changes

@raghuramank100 raghuramank100 Awaiting requested review from raghuramank100

Assignees

No one assigned

Labels

Merged oncall: quantization Quantization support in PyTorch open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@zetyquickly @jerryzh168 @facebook-github-bot @raghuramank100 @ngimel @mrshenli @pytorchbot @mruberry