[JIT] Add support for named_modules() #29495

eellison · 2019-11-09T00:47:59Z

Stack from ghstack:

[JIT] Add support for named_modules() #29495 [JIT] Add support for named_modules()

Differential Revision: D18412561

[ghstack-poisoned]

…quential" This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for #28998 [ghstack-poisoned]

ghstack-source-id: 5329963937c5622a691a9573bb95bac9597cc4af Pull Request resolved: #29495

driazati · 2019-11-11T19:47:08Z

torch/csrc/jit/script/python_sugared_value.h

+      iterator->addChild(loc, m, modules_);
+      return std::make_shared<ModuleDictMethod>(iterator, "items");
+    } else {
+      throw ErrorReport(loc) << "_Modules only supports keys, values, or items";


_Modules -> _modules

zdevito

Binding a private python member into TorchScript seems like the wrong approach. Shouldn't we implement the standard nn.Module API for this stuff? That would be the children, named_children, modules() and named_modules methods. Having _modules without named_children is going to promote the use of a private API.

…quential" This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

ghstack-source-id: 3a48515779883739cacde1f244d474884cfebc88 Pull Request resolved: #29495

…quential" This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

ghstack-source-id: 90941b7813c3f03ca0a63755cc42f6da9f32bf81 Pull Request resolved: #29495

…quential" This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

dr-ci · 2020-02-25T19:49:09Z

💊 CircleCI build failures summary and remediations

As of commit 01e28fe:

1/1 failures introduced in this PR

Detailed failure analysis

One may explore the probable reasons each build failed interactively on the Dr. CI website.

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakage:

pytorch_linux_xenial_py3_6_gcc5_4_build (1/1)

Step: "Build" (full log | pattern match details)

Automatic merge failed; fix conflicts and then commit the result.

CONFLICT (add/add): Merge conflict in .circleci/scripts/vs_install.ps1 
Auto-merging .circleci/scripts/vs_install.ps1 
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_run_in_docker.sh 
Auto-merging .circleci/scripts/binary_run_in_docker.sh 
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_populate_env.sh 
Auto-merging .circleci/scripts/binary_populate_env.sh 
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_ios_upload.sh 
Auto-merging .circleci/scripts/binary_ios_upload.sh 
CONFLICT (add/add): Merge conflict in .circleci/config.yml 
Auto-merging .circleci/config.yml 
Automatic merge failed; fix conflicts and then commit the result.

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.

This comment has been revised 19 times.

no longer exposing internal api, now exposing named_modules()

eellison · 2020-02-25T20:06:03Z

This doesn't do the checking for uniqueness, but when we recursively script we create a new module on each compilation (don't preserve aliasing), so i don't think that's actually a problem.

Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

ghstack-source-id: 0bc3f1a98042f67c41baae297d4ff70d55a07bc7 Pull Request resolved: #29495

driazati

Can you make a follow up issue or add to this PR stuff like named_children?

torch/csrc/jit/script/python_sugared_value.h

torch/csrc/jit/script/python_sugared_value.cpp

eellison · 2020-02-26T17:52:34Z

Can you make a follow up issue or add to this PR stuff like named_children?

i'm going to do the other apis as a follow up sometime this week.

Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

ghstack-source-id: 488beb0a820c3b7cd3e4f09443e849aed4f4ba39 Pull Request resolved: #29495

facebook-github-bot · 2020-02-27T03:23:36Z

@eellison merged this pull request in 057fd5e.

…29495) Summary: Pull Request resolved: #29495 This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for #28998 Test Plan: Imported from OSS Differential Revision: D18412561 Pulled By: eellison fbshipit-source-id: a8b24ebee39638fccf63b2701f65f8bb0de84faa

…ytorch#29495) Summary: Pull Request resolved: pytorch#29495 This PR adds support for `_modules`, making it so we no longer need to special case support for `nn.Sequential`. I was getting internal errors around the previous approach using `self.define()`, so i am adding this PR as part of the stack. Fix for pytorch#28998 Test Plan: Imported from OSS Differential Revision: D18412561 Pulled By: eellison fbshipit-source-id: a8b24ebee39638fccf63b2701f65f8bb0de84faa

add support for _modules, reducing special casing of nn.Sequential

41e61cf

[ghstack-poisoned]

eellison requested a review from apaszke as a code owner November 9, 2019 00:48

This was referenced Nov 9, 2019

Make nn.Sequential iterable #28987

Closed

[JIT] Cleanup special handling of Containers, allowing custom forwards #28988

Closed

eellison requested a review from suo November 9, 2019 00:48

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 9, 2019

eellison pushed a commit that referenced this pull request Nov 9, 2019

add support for _modules, reducing special casing of nn.Sequential

53d5dee

ghstack-source-id: 5329963937c5622a691a9573bb95bac9597cc4af Pull Request resolved: #29495

eellison requested review from zdevito, driazati and wanchaol and removed request for zdevito November 9, 2019 00:53

driazati approved these changes Nov 11, 2019

View reviewed changes

zdevito previously requested changes Nov 11, 2019

View reviewed changes

eellison pushed a commit that referenced this pull request Nov 12, 2019

add support for _modules, reducing special casing of nn.Sequential

c60275f

ghstack-source-id: 3a48515779883739cacde1f244d474884cfebc88 Pull Request resolved: #29495

eellison pushed a commit that referenced this pull request Nov 12, 2019

add support for _modules, reducing special casing of nn.Sequential

3a2b794

ghstack-source-id: 90941b7813c3f03ca0a63755cc42f6da9f32bf81 Pull Request resolved: #29495

eellison changed the title ~~add support for _modules, reducing special casing of nn.Sequential~~ [JIT] Add support for named_modules() Feb 25, 2020

eellison requested a review from driazati February 25, 2020 19:49

Update on "[JIT] Add support for named_modules()"

602c255

Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

eellison pushed a commit that referenced this pull request Feb 25, 2020

add support for _modules, reducing special casing of nn.Sequential

0c85582

ghstack-source-id: 0bc3f1a98042f67c41baae297d4ff70d55a07bc7 Pull Request resolved: #29495

driazati approved these changes Feb 25, 2020

View reviewed changes

torch/csrc/jit/script/python_sugared_value.h Outdated Show resolved Hide resolved

torch/csrc/jit/script/python_sugared_value.cpp Outdated Show resolved Hide resolved

Update on "[JIT] Add support for named_modules()"

01e28fe

Fix for #28998 Differential Revision: [D18412561](https://our.internmc.facebook.com/intern/diff/D18412561) [ghstack-poisoned]

eellison pushed a commit that referenced this pull request Feb 26, 2020

add support for _modules, reducing special casing of nn.Sequential

d1ed6bb

ghstack-source-id: 488beb0a820c3b7cd3e4f09443e849aed4f4ba39 Pull Request resolved: #29495

facebook-github-bot closed this in 057fd5e Feb 27, 2020

facebook-github-bot added the merged label Feb 27, 2020

facebook-github-bot deleted the gh/eellison/28/head branch March 1, 2020 15:17

chenbohua3 mentioned this pull request Apr 8, 2020

[JIT] support self.named_buffers and self.named_parameters in TorchScript #36211

Open

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] Add support for named_modules() #29495

[JIT] Add support for named_modules() #29495

eellison commented Nov 9, 2019 •

edited

driazati Nov 11, 2019

zdevito left a comment

dr-ci bot commented Feb 25, 2020 •

edited

eellison commented Feb 25, 2020

driazati left a comment

eellison commented Feb 26, 2020

facebook-github-bot commented Feb 27, 2020

[JIT] Add support for named_modules() #29495

[JIT] Add support for named_modules() #29495

Conversation

eellison commented Nov 9, 2019 • edited

driazati Nov 11, 2019

Choose a reason for hiding this comment

zdevito left a comment

Choose a reason for hiding this comment

dr-ci bot commented Feb 25, 2020 • edited

💊 CircleCI build failures summary and remediations

Detailed failure analysis

🕵️ 1 new failure recognized by patterns

pytorch_linux_xenial_py3_6_gcc5_4_build (1/1)

eellison commented Feb 25, 2020

driazati left a comment

Choose a reason for hiding this comment

eellison commented Feb 26, 2020

facebook-github-bot commented Feb 27, 2020

eellison commented Nov 9, 2019 •

edited

dr-ci bot commented Feb 25, 2020 •

edited