[JIT] Add test for ignored class type property #45233

SplitInfinity · 2020-09-23T21:26:45Z

Stack from ghstack:

[JIT] Add recursive scripting for class type module attributes #45234 [JIT] Add recursive scripting for class type module attributes
[JIT] Add API for ignoring arbitrary module attributes #45262 [JIT] Add API for ignoring arbitrary module attributes
[JIT] Enable @unused syntax for ignoring properties #45261 [JIT] Enable @unused syntax for ignoring properties
[JIT] Add test for ignored class type property #45233 [JIT] Add test for ignored class type property
[JIT] Update docs for recently added features #45232 [JIT] Update docs for recently added features

Summary
This commit modifies TestClassType.test_properties to check that
properties on class types can be ignored with the same syntax as
ignoring properties on Modules.

Test Plan
python test/test_jit.py TestClassType.test_properties

Differential Revision: D23971885

**Summary** This commit modifies `TestClassType.test_properties` to check that properties on class types can be ignored with the same syntax as ignoring properties on `Modules`. **Test Plan** `python test/test_jit.py TestClassType.test_properties` [ghstack-poisoned]

gmagogsfm · 2020-09-23T22:04:40Z

test/jit/test_class_type.py

@@ -1167,13 +1167,19 @@ def free_function(x: int) -> int:

        @torch.jit.script
        class Properties(object):
+            __ignored_properties__ = ["unsupported"]


OOC, is __ignored_properties__ the API we want users to use?

I think so. As @ppwwyyxx mentioned it should be easy to enable ignoring properties with a decorator, which is more intuitive because it's similar to how someone would ignore a function: D23797598.

Should we do it for this release or wait for the next?

I think it should be good to do for this release - i don't think it's particularly risky (although mabye unused is a better fit)

**Summary** This commit modifies `TestClassType.test_properties` to check that properties on class types can be ignored with the same syntax as ignoring properties on `Modules`. **Test Plan** `python test/test_jit.py TestClassType.test_properties` [ghstack-poisoned]

dr-ci · 2020-09-23T22:21:54Z

💊 CI failures summary and remediations

As of commit 9902bff (more details on the Dr. CI page):

1/2 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)
1/2 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_ios_11_2_1_x86_64_build (1/1)

Step: "Update Homebrew" (full log | diagnosis details | 🔁 rerun) ❄️

fatal: Could not read from remote repository.

Receiving objects:  98% (175/178) Receiving objects:  99% (177/178) Receiving objects: 100% (178/178) Receiving objects: 100% (178/178), 63.90 KiB | 10.65 MiB/s, done. 
Resolving deltas:  96% (89/92) Resolving deltas:  97% (90/92) Resolving deltas: 100% (92/92) Resolving deltas: 100% (92/92), completed with 85 local objects. 
From ssh://github.com/Homebrew/homebrew-cask-versions 
 + 15f6b44...844ea12 master     -> origin/master  (forced update) 
+ git reset --hard origin/master 
HEAD is now at 844ea12 Update brave-browser-dev from 86.1.15.64,115.64 to 86.1.15.65,115.65 (#9680) 
+ for path in '$(find /usr/local/Homebrew -type d -name .git)' 
+ cd /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/.git/.. 
+ git fetch --depth=1 origin 
Connection to github.com closed by remote host.  
fatal: Could not read from remote repository. 
 
Please make sure you have the correct access rights 
and the repository exists.

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.7-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 24 times.

eellison

One question that was raised by a user internally on your previous PR - should we make the public api __jit_ignored_properties__ ? or _jit_ignored_attributes ? (if we extend this to also work for ignored class attributes which we should)

Or any other name suggestions?

SplitInfinity · 2020-09-23T22:26:22Z

If we go through with D23797598, the public API will be decorator-based. Will we still need this class attribute API for ignoring properties?

I think something like __jit_ignored_attributes__ makes sense for ignoring attributes of modules in general, I can stack that diff on top.

eellison · 2020-09-23T22:35:28Z

SGTM

**Summary** This commit modifies `TestClassType.test_properties` to check that properties on class types can be ignored with the same syntax as ignoring properties on `Modules`. **Test Plan** `python test/test_jit.py TestClassType.test_properties` [ghstack-poisoned]

ydcjeff · 2020-09-25T05:44:49Z

Will we still need this class attribute API for ignoring properties?

Just a little opinion here, for other libraries based on PyTorch, instead of writing decorators everywhere, it is good to have class attribute API for ignoring properties to be able to ignore them in the respective files in one place.

**Summary** This commit modifies `TestClassType.test_properties` to check that properties on class types can be ignored with the same syntax as ignoring properties on `Modules`. **Test Plan** `python test/test_jit.py TestClassType.test_properties` [ghstack-poisoned]

eellison · 2020-09-28T20:31:16Z

Is this PR still necessary with __jit_ignored_attributes__ pr above ?

SplitInfinity · 2020-09-28T20:34:40Z

Is this PR still necessary with __jit_ignored_attributes__ pr above ?

This PR adds the test (which didn't exist at all before and is necessary IMO), and the PR above renames the class property from __ignored_attributes__ to __jit_unused_attributes__. I made this PR before I realized I was going to rename __ignored_attributes__. I think they could be merged into one but I don't think it's a big deal.

facebook-github-bot · 2020-09-29T02:20:31Z

@SplitInfinity merged this pull request in a0f0cb1.

This was referenced Sep 23, 2020

[JIT] Update docs for recently added features #45232

Closed

[JIT] Add recursive scripting for class type module attributes #45234

Closed

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

SplitInfinity requested review from eellison and gmagogsfm September 23, 2020 21:31

gmagogsfm approved these changes Sep 23, 2020

View reviewed changes

eellison reviewed Sep 23, 2020

View reviewed changes

This was referenced Sep 24, 2020

[JIT] Enable @unused syntax for ignoring properties #45261

Closed

[JIT] Add API for ignoring arbitrary module attributes #45262

Closed

Meghan Lele added 2 commits September 24, 2020 11:50

Meghan Lele added 4 commits September 25, 2020 13:26

facebook-github-bot closed this in a0f0cb1 Sep 29, 2020

facebook-github-bot added the merged label Sep 29, 2020

facebook-github-bot deleted the gh/splitinfinity/50/head branch October 2, 2020 14:17

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] Add test for ignored class type property #45233

[JIT] Add test for ignored class type property #45233

SplitInfinity commented Sep 23, 2020 •

edited

gmagogsfm Sep 23, 2020

SplitInfinity Sep 23, 2020 •

edited

eellison Sep 23, 2020

dr-ci bot commented Sep 23, 2020 •

edited

eellison left a comment

SplitInfinity commented Sep 23, 2020 •

edited

eellison commented Sep 23, 2020

ydcjeff commented Sep 25, 2020 •

edited

eellison commented Sep 28, 2020

SplitInfinity commented Sep 28, 2020

facebook-github-bot commented Sep 29, 2020

[JIT] Add test for ignored class type property #45233

[JIT] Add test for ignored class type property #45233

Conversation

SplitInfinity commented Sep 23, 2020 • edited

gmagogsfm Sep 23, 2020

Choose a reason for hiding this comment

SplitInfinity Sep 23, 2020 • edited

Choose a reason for hiding this comment

eellison Sep 23, 2020

Choose a reason for hiding this comment

dr-ci bot commented Sep 23, 2020 • edited

💊 CI failures summary and remediations

❄️ 1 failure tentatively classified as flaky

pytorch_ios_11_2_1_x86_64_build (1/1)

ci.pytorch.org: 1 failed

eellison left a comment

Choose a reason for hiding this comment

SplitInfinity commented Sep 23, 2020 • edited

eellison commented Sep 23, 2020

ydcjeff commented Sep 25, 2020 • edited

eellison commented Sep 28, 2020

SplitInfinity commented Sep 28, 2020

facebook-github-bot commented Sep 29, 2020

SplitInfinity commented Sep 23, 2020 •

edited

SplitInfinity Sep 23, 2020 •

edited

dr-ci bot commented Sep 23, 2020 •

edited

SplitInfinity commented Sep 23, 2020 •

edited

ydcjeff commented Sep 25, 2020 •

edited