[JIT] Improve class type annotation inference #45940

SplitInfinity · 2020-10-07T01:35:25Z

Stack from ghstack:

[JIT] Improve class type annotation inference #45940 [JIT] Improve class type annotation inference

Summary
In try_ann_to_type, if an annotation has an attribute named
__torch_script_class__, it is assumed to be a TorchScript class that
has already been scripted. However, if it is a class that extends
another class, this code path causes a crash because it looks up the
JIT type for the class by name in the compilation unit. This JIT type
obviously cannot exist because inheritance is not supported.

This commit fixes this by looking up the qualified name of a class
in torch.jit._state._script_class in order to ascertain whether it has
already been scripted (instead of looking for a __torch_script_class__
attribute on the class object.

Test Plan
This commit adds a unit test consisting of the code sample from the
issue that reported this problem.

Fixes
This commit fixes #45860.

Differential Revision: D24310027

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. [ghstack-poisoned]

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. ghstack-source-id: d90df83 Pull Request resolved: #45940

eellison · 2020-10-07T18:34:51Z

torch/jit/annotations.py

        return EnumType(_qualified_name(ann), get_enum_value_type(ann, loc), list(ann))
    if inspect.isclass(ann):
-        if hasattr(ann, "__torch_script_class__"):
+        if hasattr(ann, "__torch_script_class__") and len(ann.mro()) <= 2:


Won't this break torch.no_grad ? I think it would be more intuitive to switch to storing map of {python class pointer, jit class} instead of annotating the class

We have a map from qualified_name -> cls (Python class, not JIT type) in _state.py for all classes that have truly been scripted; what if we changed this to

Suggested change

if hasattr(ann, "__torch_script_class__") and len(ann.mro()) <= 2:

if torch.jit._state._get_script_class(_qualified_name(ann)):

In the example that motivated this fix, A would be in _state._script_classes, but B would not.

Although I realize now that inheritance is prohibited if the class is scripted directly, but allowed if it is scripted implicitly (e.g. through try_ann_to_type) and doesn't use any features of inheritance (like super()). This seems inconsistent and awkward, but as you pointed out, no_grad relies on this. 😞

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. This commit fixes this by looking up the qualified name of a class in torch.jit._state._script_class in order to ascertain whether it has already been scripted (instead of looking for a `__torch_script_class__` attribute on the class object. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. [ghstack-poisoned]

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. This commit fixes this by looking up the qualified name of a class in torch.jit._state._script_class in order to ascertain whether it has already been scripted (instead of looking for a `__torch_script_class__` attribute on the class object. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. ghstack-source-id: 90cfb48 Pull Request resolved: #45940

codecov · 2020-10-07T23:35:31Z

Codecov Report

Merging #45940 into gh/splitinfinity/58/base will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##           gh/splitinfinity/58/base   #45940      +/-   ##
============================================================
+ Coverage                     68.25%   68.27%   +0.02%     
============================================================
  Files                           410      410              
  Lines                         53611    53248     -363     
============================================================
- Hits                          36593    36357     -236     
+ Misses                        17018    16891     -127

Impacted Files	Coverage Δ
torch/jit/annotations.py	`92.23% <100.00%> (+0.45%)`	⬆️
torch/testing/_internal/jit_utils.py	`89.71% <100.00%> (+<0.01%)`	⬆️
torch/testing/_internal/te_utils.py	`0.00% <0.00%> (-82.15%)`	⬇️
torch/nn/modules/channelshuffle.py	`0.00% <0.00%> (-63.64%)`	⬇️
torch/multiprocessing/spawn.py	`79.26% <0.00%> (-6.60%)`	⬇️
torch/fx/experimental/GraphManipulation.py	`94.28% <0.00%> (-5.72%)`	⬇️
torch/utils/data/_utils/worker.py	`21.49% <0.00%> (-2.83%)`	⬇️
torch/nn/quantized/modules/activation.py	`88.88% <0.00%> (-1.66%)`	⬇️
torch/autograd/__init__.py	`84.28% <0.00%> (-1.43%)`	⬇️
torch/distributions/binomial.py	`94.93% <0.00%> (-1.01%)`	⬇️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc846db...f401180. Read the comment docs.

eellison

Looks good, two other invocations we need to update i think

torch/jit/annotations.py

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. This commit fixes this by looking up the qualified name of a class in torch.jit._state._script_class in order to ascertain whether it has already been scripted (instead of looking for a `__torch_script_class__` attribute on the class object. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. [ghstack-poisoned]

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. This commit fixes this by looking up the qualified name of a class in torch.jit._state._script_class in order to ascertain whether it has already been scripted (instead of looking for a `__torch_script_class__` attribute on the class object. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. ghstack-source-id: 6fe19a4 Pull Request resolved: #45940

dr-ci · 2020-10-14T07:26:08Z

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR---

1 failure not recognized by patterns:

Job	Step	Action
^{pytorch_linux_xenial_py3_6_gcc5_4_test}	^{Run tests}	🔁 rerun

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

SplitInfinity · 2020-10-14T07:27:37Z

The test failure is a mypy type check failure in FX.

eellison

LGTM

facebook-github-bot · 2020-10-15T10:11:23Z

@SplitInfinity merged this pull request in 75bf5f2.

facebook-github-bot · 2020-10-15T10:11:32Z

@SplitInfinity merged this pull request in 75bf5f2.

**Summary** In `try_ann_to_type`, if an annotation has an attribute named `__torch_script_class__`, it is assumed to be a TorchScript class that has already been scripted. However, if it is a class that extends another class, this code path causes a crash because it looks up the JIT type for the class by name in the compilation unit. This JIT type obviously cannot exist because inheritance is not supported. This commit fixes this by looking up the qualified name of a class in torch.jit._state._script_class in order to ascertain whether it has already been scripted (instead of looking for a `__torch_script_class__` attribute on the class object. **Test Plan** This commit adds a unit test consisting of the code sample from the issue that reported this problem. **Fixes** This commit fixes #45860. ghstack-source-id: 6fe19a4 Pull Request resolved: #45940

SplitInfinity requested a review from apaszke as a code owner October 7, 2020 01:35

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

SplitInfinity requested review from eellison, gmagogsfm and lobstergrindset October 7, 2020 01:36

eellison reviewed Oct 7, 2020

View reviewed changes

SplitInfinity requested a review from eellison October 13, 2020 22:26

SplitInfinity mentioned this pull request Oct 13, 2020

torch.jit.script segfault #45860

Closed

eellison reviewed Oct 14, 2020

View reviewed changes

torch/jit/annotations.py Show resolved Hide resolved

SplitInfinity requested a review from eellison October 14, 2020 07:26

eellison approved these changes Oct 14, 2020

View reviewed changes

facebook-github-bot closed this in 75bf5f2 Oct 15, 2020

facebook-github-bot added the Merged label Oct 15, 2020

SplitInfinity mentioned this pull request Oct 15, 2020

[v1.7.0] Release Tracker #45592

Closed

facebook-github-bot deleted the gh/splitinfinity/58/head branch October 18, 2020 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[JIT] Improve class type annotation inference #45940

[JIT] Improve class type annotation inference #45940

Uh oh!

SplitInfinity commented Oct 7, 2020 •

edited

Loading

Uh oh!

eellison Oct 7, 2020

Uh oh!

SplitInfinity Oct 7, 2020 •

edited

Loading

Uh oh!

eellison Oct 7, 2020

Uh oh!

SplitInfinity Oct 7, 2020 •

edited

Loading

Uh oh!

codecov bot commented Oct 7, 2020 •

edited

Loading

Uh oh!

eellison left a comment

Uh oh!

Uh oh!

dr-ci bot commented Oct 14, 2020 •

edited by facebook-github-bot

Loading

Uh oh!

SplitInfinity commented Oct 14, 2020

Uh oh!

eellison left a comment

Uh oh!

facebook-github-bot commented Oct 15, 2020

Uh oh!

facebook-github-bot commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	if hasattr(ann, "__torch_script_class__") and len(ann.mro()) <= 2:
	if torch.jit._state._get_script_class(_qualified_name(ann)):

[JIT] Improve class type annotation inference #45940

[JIT] Improve class type annotation inference #45940

Uh oh!

Conversation

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

Uh oh!

Uh oh!

eellison Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Oct 7, 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!

eellison Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity Oct 7, 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!

codecov bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-ci bot commented Oct 14, 2020 • edited by facebook-github-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

1 failure not recognized by patterns:

Uh oh!

SplitInfinity commented Oct 14, 2020

Uh oh!

eellison left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 15, 2020

Uh oh!

facebook-github-bot commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SplitInfinity commented Oct 7, 2020 •

edited

Loading

SplitInfinity Oct 7, 2020 •

edited

Loading

SplitInfinity Oct 7, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading

dr-ci bot commented Oct 14, 2020 •

edited by facebook-github-bot

Loading