[quant][graphmode] Get scalar type from observer module #26425
Closed
Conversation
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
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
This was referenced Sep 18, 2019
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
ZolotukhinM
suggested changes
Sep 19, 2019
ZolotukhinM
left a comment
ZolotukhinM
left a comment
There was a problem hiding this comment.
Could you please add a test for this change? Ideally, the test should fail before this change and pass after (I also suspect that such test would catch that you're not actually passing the weight set by reference). Otherwise the code looks good!
| script::Module& module, | ||
| const QConfig& qconfig) { | ||
| const QConfig& qconfig, | ||
| const std::unordered_set<Value*>& weight_values) { |
There was a problem hiding this comment.
Might be worth to refactor all that into a helper class to avoid passing these sets in every function. Later we'll also add a similar set for 'bias' values.
Contributor
Author
There was a problem hiding this comment.
OK will do refactor in a separate PR
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
ZolotukhinM
approved these changes
Sep 20, 2019
ZolotukhinM
left a comment
ZolotukhinM
left a comment
There was a problem hiding this comment.
Overall looks good! Please fix a couple of remarks inline before landing.
| // the child module. | ||
| auto module_instance = v->node()->inputs()[0]; | ||
| auto module_method_name = v->node()->s(attr::name); | ||
| // TODO: looks like this block is not related to v? maybe we should |
There was a problem hiding this comment.
The code below uses v-node(). I'm not sure this TODO comment is correct.
Contributor
Author
There was a problem hiding this comment.
I mean we don't need v, we just need node
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: [ghstack-poisoned]
Contributor
Author
|
OK. will change them
Sent from my iPhone
On Sep 20, 2019, at 12:19, Mikhail Zolotukhin <[email protected]> wrote:
@ZolotukhinM commented on this pull request.
________________________________
In test/test_jit.py<#26425 (comment)>:
+ return F.relu(self.conv(x))
+
+ def get_forward(m):
+ return m._c._get_method("forward")
+
+ m = torch.jit.script(M())
+ observer = torch.jit.script(Observer())
+ weight_observer = torch.jit.script(WeightObserver())
+ qconfig_dict = {
+ '':
+ QConfig(
+ activation=observer._c,
+ weight=weight_observer._c)
+ }
+ torch._C._jit_pass_insert_observers(m._c, "forward", qconfig_dict, True)
+ assert m._c._get_module('conv')._get_module('observer_for_input.1')._get_attribute('dtype') == 13 # torch.quint8
I think my main concern is that they are internal details, and thus we shouldn't rely on them in our tests.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#26425?email_source=notifications&email_token=ABF2R2KV6YHGZBJACLXY623QKUOYLA5CNFSM4IYDO2L2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFO2TSQ#discussion_r326768590>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABF2R2NLMSWAAORE5OGJZP3QKUOYLANCNFSM4IYDO2LQ>.
|
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: Differential Revision: [D17504459](https://our.internmc.facebook.com/intern/diff/D17504459) [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: Differential Revision: [D17504459](https://our.internmc.facebook.com/intern/diff/D17504459) [ghstack-poisoned]
Summary: Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Reviewers: mvz Subscribers: Tasks: Tags: Differential Revision: [D17504459](https://our.internmc.facebook.com/intern/diff/D17504459) [ghstack-poisoned]
Contributor
|
This pull request has been merged in 1bec8d7. |
mingbowan
pushed a commit
to mingbowan/pytorch
that referenced
this pull request
Sep 23, 2019
Summary: Pull Request resolved: pytorch#26425 Currently the scalar type is hardcoded for weight and normal tensor but what we want is to get it from corresponding observer module Test Plan: there are some known issues right now, will test e2e later when all the issues are fixed Imported from OSS Differential Revision: D17504459 fbshipit-source-id: f5a21789c2ebeb60bff4acc777db80170063c9f8
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.
Stack from ghstack:
Summary:
Currently the scalar type is hardcoded for weight and normal tensor
but what we want is to get it from corresponding observer module
Test Plan:
there are some known issues right now,
will test e2e later when all the issues are fixed
Reviewers:
mvz
Subscribers:
Tasks:
Tags:
Differential Revision: D17504459