optimize_for_mobile: bring packed params to root module #42041

vkuzo · 2020-07-24T22:28:50Z

Stack from ghstack:

optimize_for_mobile: bring packed params to root module #42041 optimize_for_mobile: bring packed params to root module
fix asan failure for fold_conv_bn when conv is quantized #42461 fix asan failure for fold_conv_bn when conv is quantized

Summary:

Adds a pass to hoist conv packed params to root module.
The benefit is that if there is nothing else in the conv module,
subsequent passes will delete it, which will reduce module size.

For context, freezing does not handle this because conv packed
params is a custom object.

Test Plan:

PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D22801605

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 09de2e1 Pull Request resolved: #42041

vkuzo · 2020-07-24T22:30:11Z

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

+
+    for (Node* n : b->nodes()) {
+
+      // TODO before land: also check for n->inputs()[0]->node()->kind() == {ConvNd}


would love some tips on best way to do this

please checkout: https://codebrowser.bddppq.com/pytorch/pytorch/torch/csrc/jit/passes/quantization/helper.cpp.html#_ZN5torch3jit16is_conv2d_moduleERKNS0_5MatchERKSt13unordered_mapINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEPNS0_5ValueESt4hashISA_ESt8equal_11899141

torch/csrc/jit/passes/xnnpack_rewrite.cpp

dr-ci · 2020-07-25T00:10:00Z

💊 CI failures summary and remediations

As of commit 145f9fb (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-xenial-rocm3.5.1-py3.6

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 42 times.

kimishpatel

Looks pretty good. Left one comment.

jerryzh168 · 2020-07-27T17:30:58Z

test/test_mobile_optimizer.py

+
+        # generic case
+
+        m = Parent()


nit: can you put these two test cases in a loop?

and conv module attribute is removed

jerryzh168 · 2020-07-27T17:33:37Z

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

+  std::vector<Node*> getAttrNodes;
+  Node* curNode = getConvPackedParamsNode->inputs()[0]->node();
+  while (!(curNode->outputs()[0]->type() == graph->inputs()[0]->type())) {
+    TORCH_CHECK(
+      curNode->kind() == prim::GetAttr,
+      "Attempted to add a non-prim::GetAttr node to a chain of prim::getAttr nodes.");
+    getAttrNodes.insert(getAttrNodes.begin(), curNode);
+    curNode = curNode->inputs()[0]->node();
+  }
+  TORCH_CHECK(getAttrNodes.size() > 0, "Did not find a chain of prim::getAttr nodes");


we have some helper function in quantization to do this: https://codebrowser.bddppq.com/pytorch/pytorch/torch/csrc/jit/passes/quantization/helper.cpp.html#_ZN5torch3jit16getInvokedModuleERNS0_6ModuleEPNS0_4NodeEPNS0_5ValueE, feel free to use them

oh this one: https://codebrowser.bddppq.com/pytorch/pytorch/torch/csrc/jit/passes/quantization/helper.cpp.html#_ZN5torch3jit19getInvokedModuleOptERKNS0_6ModuleEPNS0_4NodeEPNS0_5ValueE since maybe we will fail to get module

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

jerryzh168 · 2020-07-27T17:47:10Z

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

+  std::string newName = prefix + "." + suffix + "_packed_params";
+
+  // make sure the attribute does not already exist
+  TORCH_CHECK(


if the name already exists maybe we can create a new one? e.g. https://codebrowser.bddppq.com/pytorch/pytorch/torch/csrc/jit/passes/quantization/insert_observers.cpp.html#937

I would imagine if we hit a duplicate it would be more likely that this code has a bug, compared to the user having something named _jit_pass_hoist_conv_packed_params.{foo}.{bar}.{conv_foobar} before this pass.

if . can appear in attribute names then we might hit duplicate in extreme cases, although it is very unlikely
e.g. consider {foo}.{bar} is also an attribute itself

good point, thx, will update

jerryzh168 · 2020-07-27T17:51:51Z

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

+  // traverse the chain to get packed params value
+  std::string curName = getAttrNodes[0]->s(attr::name);
+  Module curConvModule = rootModule.attr(curName).toModule();
+  for (int idx = 1; idx < getAttrNodes.size(); idx++) {


test/test_mobile_optimizer.py

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 444a54a Pull Request resolved: #42041

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: c83039b Pull Request resolved: #42041

torch/csrc/jit/passes/xnnpack_rewrite.cpp

jerryzh168

LGTM, please fix the failing tests.

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 4efe840 Pull Request resolved: #42041

vkuzo · 2020-08-03T16:04:18Z

we should wait for #42462 to be resolved before landing this

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: e2aad66 Pull Request resolved: #42041

raghuramank100 · 2020-08-06T01:04:20Z

torch/csrc/jit/passes/hoist_conv_packed_params.cpp

+// %n =
+// prim::GetAttr[name="{prefix}.name1{...}.name(n-1)._packed_params"][%self]
+//
+void hoistConvPackedParams(


Can we also add a pass for linear? Is there value in doing this for linear modules too?

I don't see why not, although I'd defer to the mobile team on the priority.

raghuramank100 · 2020-08-06T01:05:33Z

test/test_mobile_optimizer.py

        model_optim = optimize_for_mobile(model)
-        self.assertFalse(hasattr(model_optim.conv1, "bias"))
-        self.assertFalse(hasattr(model_optim.child.conv2, "bias"))
+


Is the understanding that one cannot serialize/deserialize a model after optimize_for_mobile is called?

…t module" Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: fd8104b Pull Request resolved: #42041

…ms to root module" Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 7e3ba82 Pull Request resolved: #42041

kimishpatel

Looks good. Thanks.

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: Differential Revision: [D22801605](https://our.internmc.facebook.com/intern/diff/D22801605) [ghstack-poisoned]

Summary: Adds a pass to hoist conv packed params to root module. The benefit is that if there is nothing else in the conv module, subsequent passes will delete it, which will reduce module size. For context, freezing does not handle this because conv packed params is a custom object. Test Plan: ``` PYTORCH_JIT_LOG_LEVEL=">hoist_conv_packed_params.cpp" python test/test_mobile_optimizer.py TestOptimizer.test_hoist_conv_packed_params ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: b2ce0fc Pull Request resolved: #42041

vkuzo · 2020-08-07T18:42:15Z

resubmitted as #42740 due to a ghstack snafu

vkuzo requested a review from apaszke as a code owner July 24, 2020 22:28

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jul 24, 2020

vkuzo commented Jul 24, 2020

View reviewed changes

vkuzo requested review from dreiss, jerryzh168 and kimishpatel July 24, 2020 22:30

kimishpatel reviewed Jul 24, 2020

View reviewed changes

torch/csrc/jit/passes/xnnpack_rewrite.cpp Show resolved Hide resolved

kimishpatel requested changes Jul 27, 2020

View reviewed changes

jerryzh168 reviewed Jul 27, 2020

View reviewed changes

torch/csrc/jit/passes/hoist_conv_packed_params.cpp Show resolved Hide resolved

jerryzh168 reviewed Jul 27, 2020

View reviewed changes

test/test_mobile_optimizer.py Outdated Show resolved Hide resolved

vkuzo requested a review from kimishpatel July 28, 2020 20:48

vkuzo commented Jul 29, 2020

View reviewed changes

torch/csrc/jit/passes/xnnpack_rewrite.cpp Show resolved Hide resolved

jerryzh168 approved these changes Jul 29, 2020

View reviewed changes

vkuzo mentioned this pull request Aug 3, 2020

ASAN failure on module freezing #42462

Closed

vkuzo mentioned this pull request Aug 5, 2020

fix asan failure for fold_conv_bn when conv is quantized #42461

Closed

raghuramank100 reviewed Aug 6, 2020

View reviewed changes

kimishpatel approved these changes Aug 6, 2020

View reviewed changes

vkuzo closed this Aug 7, 2020

facebook-github-bot deleted the gh/vkuzo/108/head branch September 7, 2020 14:17


		for (Node* n : b->nodes()) {

		// TODO before land: also check for n->inputs()[0]->node()->kind() == {ConvNd}

optimize_for_mobile: bring packed params to root module #42041

optimize_for_mobile: bring packed params to root module #42041

Uh oh!

Conversation

vkuzo commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-ci bot commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

kimishpatel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jerryzh168 left a comment

Choose a reason for hiding this comment

Uh oh!

vkuzo commented Aug 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimishpatel left a comment

Choose a reason for hiding this comment

Uh oh!

vkuzo commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vkuzo commented Jul 24, 2020 •

edited

Loading

dr-ci bot commented Jul 25, 2020 •

edited

Loading