Skip to content

Optimize mobile model on cloned module instead of in-place transformation#36621

Closed
xcheng16 wants to merge 14 commits intogh/xcheng16/18/basefrom
gh/xcheng16/18/head
Closed

Optimize mobile model on cloned module instead of in-place transformation#36621
xcheng16 wants to merge 14 commits intogh/xcheng16/18/basefrom
gh/xcheng16/18/head

Conversation

@xcheng16
Copy link
Copy Markdown
Contributor

@xcheng16 xcheng16 commented Apr 14, 2020 •
edited
Loading

Stack from ghstack:

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D21028406

Optimize mobile model on cloned module instead of in-place transforma…
426dc7b 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@xcheng16 xcheng16 requested a review from apaszke as a code owner April 14, 2020 22:06
@xcheng16 xcheng16 mentioned this pull request Apr 14, 2020
Closed
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 14, 2020
kimishpatel
kimishpatel reviewed Apr 14, 2020
View reviewed changes
Comment thread torch/csrc/jit/passes/xnnpack_rewrite.cpp Outdated
insertPrePackedOps(m);
m = freeze_module(m);
FoldPrePackingOps(m);
auto moduleClone = module.clone();
Copy link
Copy Markdown
Contributor

@kimishpatel kimishpatel Apr 14, 2020

Choose a reason for hiding this comment

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

Do variable names follow camel case? From what I noticed no. If so change accordingly. Otherwise look great.

Copy link
Copy Markdown
Contributor Author

@xcheng16 xcheng16 Apr 14, 2020

Choose a reason for hiding this comment

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

good point, seems like not.

kimishpatel
kimishpatel requested changes Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

Small change.

Update on "Optimize mobile model on cloned module instead of in-place…
955a884 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented Apr 14, 2020 •
edited
Loading

💊 Build failures summary and remediations

As of commit 398a537 (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.

See how this bot performed.

This comment has been revised 49 times.

kimishpatel
kimishpatel approved these changes Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

LGTM. There are merge conflict. Plus import to phabricator.

AshkanAliabadi
AshkanAliabadi suggested changes Apr 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@AshkanAliabadi AshkanAliabadi left a comment

Choose a reason for hiding this comment

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

When it comes making modifications in place vs not, what I have seen Python quantization modules do is to take an inplace argument, which if set to true, modifies the model in place, otherwise creates a copy. This is merely a suggestion, but following the principle of least surprise and API consistency I think we could follow the same principle. Again, just a suggestion, this is also fine as well.

Comment thread torch/csrc/jit/passes/xnnpack_rewrite.cpp
insertPrePackedOps(cloned_module);
cloned_module = freeze_module(cloned_module);
FoldPrePackingOps(cloned_module);
return cloned_module;
Copy link
Copy Markdown
Contributor

@AshkanAliabadi AshkanAliabadi Apr 14, 2020

Choose a reason for hiding this comment

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

This function's return type is void, right? You are probably getting a compiler warning which PyTorch's compilation settings do not treat as error. Also if we are no longer modifying the parameter please pass it by const reference.

Copy link
Copy Markdown
Contributor

@AshkanAliabadi AshkanAliabadi Apr 15, 2020

Choose a reason for hiding this comment

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

It's still safer to pass the argument by const ref. Please consider that if there are no other complications. Otherwise we wouldn't have a way of knowing if we accidentally modify m.

Update on "Optimize mobile model on cloned module instead of in-place…
932b838 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
@kimishpatel
Copy link
Copy Markdown
Contributor

kimishpatel commented Apr 14, 2020

When it comes making modifications in place vs not, what I have seen Python quantization modules do is to take an inplace argument, which if set to true, modifies the model in place, otherwise creates a copy. This is merely a suggestion, but following the principle of least surprise and API consistency I think we could follow the same principle. Again, just a suggestion, this is also fine as well.

I agree, but I think common expectation is to not do inplace? So if inplace is wanted, you have to be explicit about it. Also, something funny was happening when it was doing inplace mutation silently. For some reason it wasn't working until the module was serialized and then deserialized. That needs to be looked at, but this change at least make it cleaner and explicit.

Update on "Optimize mobile model on cloned module instead of in-place…
568d60e 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 14, 2020
Optimize mobile model on cloned module instead of in-place transforma…
ba8a98c 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 495164d
Pull Request resolved: #36621
Update on "Optimize mobile model on cloned module instead of in-place…
a911c1e 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
Update on "Optimize mobile model on cloned module instead of in-place…
b44c72c 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 15, 2020
Optimize mobile model on cloned module instead of in-place transforma…
7a4eea2 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: eb31ad0
Pull Request resolved: #36621
Update on "Optimize mobile model on cloned module instead of in-place…
c1d2ce5 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 15, 2020
Optimize mobile model on cloned module instead of in-place transforma…
53d36d5 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: e5c292c
Pull Request resolved: #36621
Update on "Optimize mobile model on cloned module instead of in-place…
541e52f 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 15, 2020
Optimize mobile model on cloned module instead of in-place transforma…
d16477a 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: bbd7059
Pull Request resolved: #36621
Update on "Optimize mobile model on cloned module instead of in-place…
449fd71 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
Update on "Optimize mobile model on cloned module instead of in-place…
e6526a5 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
Update on "Optimize mobile model on cloned module instead of in-place…
1e1743e 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
Update on "Optimize mobile model on cloned module instead of in-place…
842af9d 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 15, 2020
Optimize mobile model on cloned module instead of in-place transforma…
c396f75 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 98a01c5
Pull Request resolved: #36621
xcheng16 added 2 commits April 15, 2020 22:10
Update on "Optimize mobile model on cloned module instead of in-place…
5647d64 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
Update on "Optimize mobile model on cloned module instead of in-place…
398a537 
… transformation"

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: [D21028406](https://our.internmc.facebook.com/intern/diff/D21028406)

[ghstack-poisoned]
xcheng16 pushed a commit that referenced this pull request Apr 16, 2020
Optimize mobile model on cloned module instead of in-place transforma…
f0d00fb 
…tion

Summary:
Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py
Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: c463e70
Pull Request resolved: #36621
AshkanAliabadi
AshkanAliabadi approved these changes Apr 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@AshkanAliabadi AshkanAliabadi left a comment

Choose a reason for hiding this comment

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

Thank you!

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Apr 17, 2020

This pull request has been merged in eccb40f.

@facebook-github-bot facebook-github-bot deleted the gh/xcheng16/18/head branch April 20, 2020 14:17
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@xcheng16 @facebook-github-bot
Optimize mobile model on cloned module instead of in-place transforma…
756fd53 
…tion (pytorch#36621)

Summary:
Pull Request resolved: pytorch#36621

Instead of doing in-place transformation inside optimizeForMobile method,
we would like to maintain the original structure for passed scriptedModule,
so before optmization starts, we use the cloned module to do subsequent optimization
process and return the optimized cloned module.

Test Plan:
unit test
python test/test_mobile_optimizer.py

Imported from OSS

Differential Revision: D21028406

fbshipit-source-id: 756172ef99b1c1df6bb7d00e5deca85a4c239a87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kimishpatel kimishpatel kimishpatel approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@dreiss dreiss Awaiting requested review from dreiss

+1 more reviewer

@AshkanAliabadi AshkanAliabadi AshkanAliabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xcheng16 @kimishpatel @facebook-github-bot @AshkanAliabadi @mruberry