[JIT] Add property support to TorchScript classes #42389

SplitInfinity · 2020-07-31T22:59:57Z

Stack from ghstack:

[JIT] Add property support for ScriptModules #42390 [JIT] Add property support for ScriptModules
[JIT] Add property support to TorchScript classes #42389 [JIT] Add property support to TorchScript classes

Summary
This commit adds support for properties to TorchScript classes,
specifically for getters and setters. They are implemented essentially
as pointers to the methods that the corresponding decorators decorate,
which are treated like regular class methods. Deleters for properties
are considered to be out of scope (and probably useless for TorchScript
anyway).

Test Plan
This commit adds a unit test for a class with a property that has both
getter and setter and one that has only a getter.

python test/test_jit.py TestClassType.test_properties

Differential Revision: D22880232

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` [ghstack-poisoned]

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Differential Revision: [D22880232](https://our.internmc.facebook.com/intern/diff/D22880232) [ghstack-poisoned]

dr-ci · 2020-08-04T20:13:03Z

💊 CI failures summary and remediations

As of commit d2b03c9 (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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 22 times.

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Differential Revision: [D22880232](https://our.internmc.facebook.com/intern/diff/D22880232) [ghstack-poisoned]

eellison

Cool, this looks great ! I think this is logically good to go, but there are a couple refactorings that would be good to do before landing. Those should be pretty mechanical/easy to do though.

aten/src/ATen/core/type.cpp

eellison · 2020-08-10T16:21:56Z

torch/csrc/jit/api/compilation_unit.h

  }

+  // Version of define but with support for properties.
+  std::vector<Function*> define(


can we unify this with the constructor below and adjust the callsites or make properties / propResolvers optional ?

Done. Although there are a bunch of callsites now where we compile free functions but still have to add empty {} for properties/property resolvers. I thought of adding them to the end of the argument list so as to avoid modifying all those callsites but it didn't seem as clean.

torch/csrc/jit/api/compilation_unit.h

eellison · 2020-08-10T16:28:15Z

torch/csrc/jit/frontend/script_type_parser.cpp

      if (isTorch(select.value()) && name == "Tensor") {
        return "Tensor";
+      } else if (isTorch(select.value()) && name == "device") {
+        return "Device";


is there a reason for this change to be included ?

Internal fbcode Detectron2 tests break without it because they use -> torch.device as a return type annotation.

That should work bc of https://github.com/pytorch/pytorch/blob/master/torch/jit/annotations.py#L313. I think what's happening here is the resolver for properties is not being set properly. We should be closing over torch bc it's used in the function, but we're not.

Sometimes with python wrapped functions you have to unpack the wrapped function to do resolution correctly, as in #40468.

Could you add a test where a property getterr function calls into a free-function ?

def foo(x): return x + 1 ... @property def attr(self): return foo(self.a)

that should check whether the resolver is being properly set

short repro:

class Boxes(object): @property def device(self) -> torch.device: return 1 + hi closure = torch._jit_internal.get_closure(Boxes.device.fget) # torch is closed over print(closure)

I added a test for this but I'm not sure if I did it right...

No, the test looks good. We shouldn't need to add the torch.device thing, it means something else has gone wrong... It would be nice to fix but if resolution in general is working, then I think we can land this.

Could you remove this "torch.device" logic here, and then in the D2 test change the property to look like ?

from torch import device @property def foo() -> device: ...

This should workaround the problem without added unnecessary stuff to our code base...

torch/csrc/jit/frontend/ir_emitter.cpp

torch/csrc/jit/frontend/sugared_value.cpp

torch/csrc/jit/python/script_init.cpp

torch/csrc/jit/frontend/ir_emitter.cpp

test/jit/test_class_type.py

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Differential Revision: [D22880232](https://our.internmc.facebook.com/intern/diff/D22880232) [ghstack-poisoned]

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` ghstack-source-id: 678bf1f Pull Request resolved: #42389

SplitInfinity · 2020-08-11T02:33:24Z

Hey, I addressed all of the comments but I had to amend my commit because I didn't want ghstack to make another PR.

eellison

Sweet, looks great!! One comment about the torch.device thing.

Also, it would be maybe be nice to land this with the property support for TorchScript modules...

eellison · 2020-08-11T16:13:41Z

torch/csrc/jit/frontend/script_type_parser.cpp

      if (isTorch(select.value()) && name == "Tensor") {
        return "Tensor";
+      } else if (isTorch(select.value()) && name == "device") {
+        return "Device";


No, the test looks good. We shouldn't need to add the torch.device thing, it means something else has gone wrong... It would be nice to fix but if resolution in general is working, then I think we can land this.

Could you remove this "torch.device" logic here, and then in the D2 test change the property to look like ?

from torch import device @property def foo() -> device: ...

This should workaround the problem without added unnecessary stuff to our code base...

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Differential Revision: [D22880232](https://our.internmc.facebook.com/intern/diff/D22880232) [ghstack-poisoned]

SplitInfinity · 2020-08-11T19:16:02Z

Okay, I reverted the torch.device change so that we can re-examine the failure. I held off on addressing comments on the properties for Modules PR until all of the major changes in this PR were acceptable in order to avoid spending too much time on rebasing. But now that this PR is approved, I will address comments on #42390 so that we can land them together or in quick succession.

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Differential Revision: [D22880232](https://our.internmc.facebook.com/intern/diff/D22880232) [ghstack-poisoned]

Summary: Pull Request resolved: #42389 **Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` Test Plan: Imported from OSS Reviewed By: eellison, ppwwyyxx Differential Revision: D22880232 Pulled By: SplitInfinity fbshipit-source-id: 4828640f4234cb3b0d4f3da4872a75fbf519e5b0

**Summary** This commit adds support for properties to TorchScript classes, specifically for getters and setters. They are implemented essentially as pointers to the methods that the corresponding decorators decorate, which are treated like regular class methods. Deleters for properties are considered to be out of scope (and probably useless for TorchScript anyway). **Test Plan** This commit adds a unit test for a class with a property that has both getter and setter and one that has only a getter. `python test/test_jit.py TestClassType.test_properties` ghstack-source-id: 75e42d4 Pull Request resolved: pytorch/pytorch#42389

SplitInfinity requested a review from apaszke as a code owner July 31, 2020 22:59

SplitInfinity mentioned this pull request Jul 31, 2020

[JIT] Add property support for ScriptModules #42390

Closed

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

SplitInfinity requested a review from eellison July 31, 2020 23:01

Meghan Lele added 2 commits August 5, 2020 14:13

eellison reviewed Aug 10, 2020

View reviewed changes

eellison approved these changes Aug 11, 2020

View reviewed changes

SplitInfinity requested review from eellison and removed request for eellison August 11, 2020 22:16

facebook-github-bot closed this in facebookresearch/detectron2@18b692f Aug 14, 2020

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

facebook-github-bot deleted the gh/splitinfinity/28/head branch August 18, 2020 14:17

mzr1996 mentioned this pull request Aug 4, 2021

[Bug]pytorch2torchscript对ResNeSt进行编译错误，ModuleAttributeError: 'ResNeSt' object has no attribute 'norm1' open-mmlab/mmpretrain#366

Closed

[JIT] Add property support to TorchScript classes #42389

[JIT] Add property support to TorchScript classes #42389

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

Uh oh!

eellison left a comment

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!

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!

SplitInfinity Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SplitInfinity commented Aug 11, 2020

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SplitInfinity commented Aug 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SplitInfinity commented Jul 31, 2020 •

edited

Loading

dr-ci bot commented Aug 4, 2020 •

edited

Loading

SplitInfinity Aug 11, 2020 •

edited

Loading