Allow Freezing of Module containing interface attribute #41860

bzinodev · 2020-07-22T19:00:11Z

This patch allows to freeze model that utilizes interfaces. Freezing works
under the user assumption that the interfase module dones not aliases with
any value used in the model.

To enable freezing of such modules, added an extra pramater:

torch._C._freeze_module(module, ignoreInterfaces = True)

facebook-github-bot

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

dr-ci · 2020-07-22T19:34:01Z

💊 CI failures summary and remediations

As of commit 47d6073 (more details on the Dr. CI page):

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

Extra GitHub checks: 1 failed

Failed: GitHub Actions - flake8-py3

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

facebook-github-bot

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

facebook-github-bot

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

eellison

I’m not sure if we’re necessarily doing it “safe” currently - for instance, we don’t check if the top level module has any setAttrs that reassign the interface, and we don’t check that any of the interface calls contain nester setAttrs.

Having said that, interfaces aren’t a public api and I don’t know if it’s worth it to spend too much time on it.

I think as a simple thing we could just inline all of interfaces first, and then run freezing as normal

[Zino:] Changed the patch to first inline interface calls then perform freezing. I have made changes so we don't check for setattr inside interfaces. Instead fail if freezing is unable to fold away the interface

torch/csrc/jit/ir/alias_analysis.cpp

torch/csrc/jit/passes/freeze_module.cpp

test/jit/test_module_interface.py

torch/csrc/jit/passes/freeze_module.cpp

facebook-github-bot

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

test/jit/test_module_interface.py

facebook-github-bot

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

eellison

class Mod(torch.nn.Module):
     def __init__(self):
          self.a : Interface = a
          self.b : Interface = b

     def forward(self, input):
          self.a = b
          return self.a(input)

Presently we would inline the forward of a and then invoke it with b module. I think for safety it would be good to check that there are no setAttrs of an interface type in the forward function of the top level module or interfaces we've inlined.

[Zino] I add a test to check such model. Currently freezing fails because self.a cannot be folded away. Attribute 'a' has to be preserved and we dont allow to preserve an interface attribute (for the moment).

test/jit/test_module_interface.py

This patch allows to freeze model that utilizes interfaces. Freezing works under the user assumption that the interfase module dones not aliases with any value used in the model. To enable freezing of such modules, added an extra pramater: torch._C._freeze_module(module, freezeInterfaces = True) By default freezeInterfaces is to True.

facebook-github-bot

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

eellison

LGTM. This may not be completely safe re: interfaces but it provides a good amount of safety. interfaces are not a public feature so I think this is fine; doing too much work on this is not worth our time atm. If we decide to make interfaces public we should revisit this.

eellison · 2020-08-21T18:34:39Z

test/jit/test_module_interface.py

+
+            def forward(self, x):
+                self.proxy_mod = self.sub
+                y = self.proxy_mod(x);


I'm glad that this test fails but i'm not sure why it does lol. Do you know why ?

facebook-github-bot · 2020-08-22T02:17:53Z

@bzinodev merged this pull request in abe878c.

bzinodev requested a review from apaszke as a code owner July 22, 2020 19:00

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

facebook-github-bot reviewed Jul 22, 2020

View reviewed changes

bzinodev force-pushed the freezeInterfaces branch from 42776a1 to 4c219bd Compare July 27, 2020 18:46

bzinodev force-pushed the freezeInterfaces branch from 4c219bd to 00579bb Compare August 3, 2020 22:20

bzinodev requested review from eellison and suo August 3, 2020 22:28

facebook-github-bot reviewed Aug 3, 2020

View reviewed changes

facebook-github-bot reviewed Aug 10, 2020

View reviewed changes

eellison reviewed Aug 12, 2020

View reviewed changes

bzinodev force-pushed the freezeInterfaces branch 2 times, most recently from a48380a to 322f453 Compare August 19, 2020 16:41

facebook-github-bot reviewed Aug 19, 2020

View reviewed changes

eellison reviewed Aug 19, 2020

View reviewed changes

test/jit/test_module_interface.py Show resolved Hide resolved

bzinodev force-pushed the freezeInterfaces branch 2 times, most recently from 747c948 to e1f3660 Compare August 20, 2020 02:59

facebook-github-bot reviewed Aug 20, 2020

View reviewed changes

eellison reviewed Aug 20, 2020

View reviewed changes

test/jit/test_module_interface.py Outdated Show resolved Hide resolved

test/jit/test_module_interface.py Outdated Show resolved Hide resolved

bzinodev force-pushed the freezeInterfaces branch from e1f3660 to 595f743 Compare August 21, 2020 01:14

bzinodev force-pushed the freezeInterfaces branch from 595f743 to 47d6073 Compare August 21, 2020 03:09

facebook-github-bot reviewed Aug 21, 2020

View reviewed changes

eellison approved these changes Aug 21, 2020

View reviewed changes

facebook-github-bot closed this in abe878c Aug 22, 2020

facebook-github-bot added the merged label Aug 22, 2020

mruberry added the Merged label Oct 28, 2020

facebook-github-bot deleted the freezeInterfaces branch January 27, 2021 18:26

Allow Freezing of Module containing interface attribute #41860

Allow Freezing of Module containing interface attribute #41860

Uh oh!

Conversation

bzinodev commented Jul 22, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 1 failed

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

eellison left a comment • edited by bzinodev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

eellison left a comment • edited by bzinodev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

eellison Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Aug 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dr-ci bot commented Jul 22, 2020 •

edited

Loading

eellison left a comment •

edited by bzinodev

Loading

eellison left a comment •

edited by bzinodev

Loading