[JIT] Disallow plain Tuple type annotation without arg#44585
[JIT] Disallow plain Tuple type annotation without arg#44585SplitInfinity wants to merge 1 commit intogh/splitinfinity/45/basefrom
Conversation
**Summary** This commit disallows plain `Tuple` type annotations without any contained types both in type comments and in-line as Python3-style type annotations. **Test Plan** This commit adds a unit test for these two situations. [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 18bafaa (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis 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. This comment has been revised 1 time. |
Codecov Report
@@ Coverage Diff @@
## gh/splitinfinity/45/base #44585 +/- ##
=========================================================
Coverage 67.98% 67.98%
=========================================================
Files 384 384
Lines 49587 49589 +2
=========================================================
+ Hits 33711 33713 +2
Misses 15876 15876
Continue to review full report at Codecov.
|
eellison
left a comment
There was a problem hiding this comment.
Looks good - one question about when we raise vs when we throw.
Do you mind also handling the other case where we generate a bad jit type - here we don't validate the the contained types don't return None before we instantiate Tuple type https://github.com/pytorch/pytorch/blob/master/torch/jit/annotations.py#L286
|
|
||
|
|
||
| def is_tuple(ann): | ||
| if ann is Tuple: |
There was a problem hiding this comment.
This is called in https://github.com/pytorch/pytorch/blob/master/torch/jit/annotations.py#L285 in try_ann_to_type - try_ann_to_type tries to resolve the type if it can, and otherwise returns None. The behavior in the function is getting inconsistent as to whether we throw or return None if it's a bad/unresolvable type. E.g. we're now erroring in https://github.com/pytorch/pytorch/blob/master/torch/jit/annotations.py#L302.
I think we should maybe return None instead of erroring. What do you think?
There was a problem hiding this comment.
It's true that the error msg is better now - in the future we could add a why_not parameter return value to improve but that should probably be its own PR...
There was a problem hiding this comment.
Sorry I deleted my previous comment because I didn't understand what you were saying at first. It is possible to return None instead of erroring here, but it will require more code due to small differences in the typing module between python3.6 and python3.8. You can see a concrete example of doing this in one of my previous diffs for Dict. The red diff is what I would have to add back. Take a look and if that extra code is acceptable, I can do it for Tuple and List too.
|
@SplitInfinity merged this pull request in ffe127e. |
Summary: Pull Request resolved: #44585 **Summary** This commit disallows plain `Tuple` type annotations without any contained types both in type comments and in-line as Python3-style type annotations. **Test Plan** This commit adds a unit test for these two situations. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23721515 Pulled By: SplitInfinity fbshipit-source-id: e11c77a4fac0b81cd535c37a31b9f4129c276592
Summary: Pull Request resolved: pytorch#44585 **Summary** This commit disallows plain `Tuple` type annotations without any contained types both in type comments and in-line as Python3-style type annotations. **Test Plan** This commit adds a unit test for these two situations. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23721515 Pulled By: SplitInfinity fbshipit-source-id: e11c77a4fac0b81cd535c37a31b9f4129c276592
Stack from ghstack:
Summary
This commit disallows plain
Tupletype annotations without anycontained types both in type comments and in-line as Python3-style
type annotations.
Test Plan
This commit adds a unit test for these two situations.
Differential Revision: D23721515