[JIT] Disallow plain Dict type annotation without arg by SplitInfinity · Pull Request #44334 · pytorch/pytorch

SplitInfinity · 2020-09-08T21:52:42Z

Stack from ghstack:

[JIT] Disallow plain Optional type annotation without arg #44586 [JIT] Disallow plain Optional type annotation without arg
[JIT] Disallow plain Tuple type annotation without arg #44585 [JIT] Disallow plain Tuple type annotation without arg
[JIT] Disallow plain List type annotation without arg #44584 [JIT] Disallow plain List type annotation without arg
[JIT] Disallow plain Dict type annotation without arg #44334 [JIT] Disallow plain Dict type annotation without arg

Summary
This commit detects and prohibits the case in which typing.Dict is
used as an annotation without type arguments (i.e. typing.Dict[K, V]).
At present, typing.Dict is always assumed to have two arguments, and
when it is used without them, typing.Dict.__args__ is nonempty and
contains some typing.TypeVar instances, which have no JIT type equivalent.
Consequently, trying to convert typing.Dict to a JIT type results in
a c10::DictType with nullptr for its key and value types, which can cause
a segmentation fault.

This is fixed by returning a DictType from
jit.annotations.try_ann_to_type only if the key and value types are converted
successfully to a JIT type and returning None otherwise.

Test Plan
This commit adds a unit test to TestDict that tests the plain Dict
annotations throw an error.

Fixes
This commit closes #43530.

Differential Revision: D23610766

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. [ghstack-poisoned]

dr-ci · 2020-09-08T22:31:22Z

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_macos_10_13_py3_test (1/1)

Step: "Test" (full log | diagnosis details | 🔁 rerun)

Sep 11 21:49:40 [E request_callback_no_python.cpp:618] Received error while processing request type 2: RuntimeError: Can not pickle torch.futures.Future

Sep 11 21:49:40 At: 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(93): serialize 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(145): serialize 
Sep 11 21:49:40  
Sep 11 21:49:40 [E request_callback_no_python.cpp:618] Received error while processing request type 2: RuntimeError: Can not pickle torch.futures.Future 
Sep 11 21:49:40  
Sep 11 21:49:40 At: 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(93): serialize 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(145): serialize 
Sep 11 21:49:40  
Sep 11 21:49:40 [E request_callback_no_python.cpp:618] Received error while processing request type 2: RuntimeError: Can not pickle torch.futures.Future 
Sep 11 21:49:40  
Sep 11 21:49:40 At: 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(93): serialize 
Sep 11 21:49:40   /Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/distributed/rpc/internal.py(145): serialize 
Sep 11 21:49:40  
Sep 11 21:49:40 [W tensorpipe_agent.cpp:576] RPC agent for worker1 encountered error when reading incoming request from worker0: EOF: end of file (this is expected to happen during shutdown) 
Sep 11 21:49:40 [W tensorpipe_agent.cpp:576] RPC agent for worker2 encountered error when reading incoming request from worker0: EOF: end of file (this is expected to happen during shutdown) 
Sep 11 21:49:40 [W tensorpipe_agent.cpp:576] RPC agent for worker2 encountered error when reading incoming request from worker1: EOF: end of file (this is expected to happen during shutdown) 
Sep 11 21:49:40 ok (1.568s) 
Sep 11 21:49:42   test_return_future_remote (__main__.TensorPipeRpcTestWithSpawn) ... [W tensorpipe_agent.cpp:576] RPC agent for worker3 encountered error when reading incoming request from worker0: EOF: end of file (this is expected to happen during shutdown)

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.

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. [ghstack-poisoned]

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. ghstack-source-id: 1601d24 Pull Request resolved: #44334

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. [ghstack-poisoned]

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. Differential Revision: [D23610766](https://our.internmc.facebook.com/intern/diff/D23610766) [ghstack-poisoned]

eellison · 2020-09-09T23:05:23Z

Another type container i missed: Tuple - could you check that this works for that too?

EDIT: also Future, RRef,

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. Differential Revision: [D23610766](https://our.internmc.facebook.com/intern/diff/D23610766) [ghstack-poisoned]

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. ghstack-source-id: 8fb136a Pull Request resolved: #44334

SplitInfinity · 2020-09-10T01:22:41Z

I'll stack them on top. I'm currently fighting some test failures due to small differences in typing module internals between python3.8 and python3.6 that impact our implementation of is_dict. Once I figure that out, I think those changes should transfer over to is_list, is_tuple, etc.

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. Differential Revision: [D23610766](https://our.internmc.facebook.com/intern/diff/D23610766) [ghstack-poisoned]

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. ghstack-source-id: c75b7d0 Pull Request resolved: #44334

SplitInfinity · 2020-09-10T06:17:22Z

Re-requesting review because I had to make non-trivial changes to get python3.6 tests to pass. Let me know what you think, and once these changes look good, I'll stack similar improvements to try_ann_to_type for List, Tuple, Future, RRef on top.

codecov · 2020-09-10T07:47:52Z

Codecov Report

❗ No coverage uploaded for pull request base (gh/splitinfinity/41/base@8a574c7). Click here to learn what that means.
The diff coverage is n/a.

@@                     Coverage Diff                     @@
##             gh/splitinfinity/41/base   #44334   +/-   ##
===========================================================
  Coverage                            ?   68.00%           
===========================================================
  Files                               ?      382           
  Lines                               ?    49403           
  Branches                            ?        0           
===========================================================
  Hits                                ?    33597           
  Misses                              ?    15806           
  Partials                            ?        0

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 8a574c7...fa5bf34. Read the comment docs.

**Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. Differential Revision: [D23610766](https://our.internmc.facebook.com/intern/diff/D23610766) [ghstack-poisoned]

SplitInfinity · 2020-09-12T04:19:07Z

Okay, I stacked similar PRs for List, Tuple, and Optional on top. RRef and Future are already handled.

facebook-github-bot · 2020-09-16T22:22:20Z

@SplitInfinity merged this pull request in cb3b8a3.

facebook-github-bot · 2020-09-16T22:22:30Z

@SplitInfinity merged this pull request in cb3b8a3.

Summary: Pull Request resolved: #44334 **Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes #43530. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23610766 Pulled By: SplitInfinity fbshipit-source-id: 036b10eff6e3206e0da3131cfb4997d8189c4fec

Summary: Pull Request resolved: pytorch#44334 **Summary** This commit detects and prohibits the case in which `typing.Dict` is used as an annotation without type arguments (i.e. `typing.Dict[K, V]`). At present, `typing.Dict` is always assumed to have two arguments, and when it is used without them, `typing.Dict.__args__` is nonempty and contains some `typing.TypeVar` instances, which have no JIT type equivalent. Consequently, trying to convert `typing.Dict` to a JIT type results in a `c10::DictType` with `nullptr` for its key and value types, which can cause a segmentation fault. This is fixed by returning a `DictType` from `jit.annotations.try_ann_to_type` only if the key and value types are converted successfully to a JIT type and returning `None` otherwise. **Test Plan** This commit adds a unit test to `TestDict` that tests the plain `Dict` annotations throw an error. **Fixes** This commit closes pytorch#43530. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23610766 Pulled By: SplitInfinity fbshipit-source-id: 036b10eff6e3206e0da3131cfb4997d8189c4fec

SplitInfinity requested a review from apaszke as a code owner September 8, 2020 21:52

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

SplitInfinity requested review from eellison, gmagogsfm, jamesr66a and suo September 8, 2020 21:53

gmagogsfm reviewed Sep 8, 2020

View reviewed changes

Comment thread test/jit/test_list_dict.py Outdated

Comment thread torch/jit/annotations.py Outdated

gmagogsfm approved these changes Sep 9, 2020

View reviewed changes

SplitInfinity requested a review from gmagogsfm September 10, 2020 06:16

gmagogsfm approved these changes Sep 10, 2020

View reviewed changes

Comment thread torch/jit/annotations.py Outdated

This was referenced Sep 12, 2020

[JIT] Disallow plain List type annotation without arg #44584

Closed

[JIT] Disallow plain Tuple type annotation without arg #44585

Closed

[JIT] Disallow plain Optional type annotation without arg #44586

Closed

suo removed their request for review September 15, 2020 16:57

facebook-github-bot closed this in cb3b8a3 Sep 16, 2020

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

facebook-github-bot deleted the gh/splitinfinity/41/head branch September 20, 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] Disallow plain Dict type annotation without arg#44334

[JIT] Disallow plain Dict type annotation without arg#44334
SplitInfinity wants to merge 8 commits intogh/splitinfinity/41/basefrom
gh/splitinfinity/41/head

SplitInfinity commented Sep 8, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

dr-ci Bot commented Sep 8, 2020 •

edited

Loading

Uh oh!

eellison commented Sep 9, 2020 •

edited

Loading

Uh oh!

SplitInfinity commented Sep 10, 2020

Uh oh!

SplitInfinity commented Sep 10, 2020

Uh oh!

codecov Bot commented Sep 10, 2020 •

edited

Loading

Uh oh!

Uh oh!

SplitInfinity commented Sep 12, 2020

Uh oh!

facebook-github-bot commented Sep 16, 2020

Uh oh!

facebook-github-bot commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dr-ci Bot commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_macos_10_13_py3_test (1/1)

Uh oh!

eellison commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SplitInfinity commented Sep 10, 2020

Uh oh!

SplitInfinity commented Sep 10, 2020

Uh oh!

codecov Bot commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

SplitInfinity commented Sep 12, 2020

Uh oh!

facebook-github-bot commented Sep 16, 2020

Uh oh!

facebook-github-bot commented Sep 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SplitInfinity commented Sep 8, 2020 •

edited

Loading

dr-ci Bot commented Sep 8, 2020 •

edited

Loading

eellison commented Sep 9, 2020 •

edited

Loading

codecov Bot commented Sep 10, 2020 •

edited

Loading