New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Union[NoneType, T] as input type #51606
Conversation
💊 CI failures summary and remediationsAs of commit 7226a54 (more details on the Dr. CI page):
🕵️ 4 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages: pytorch_linux_bionic_py3_8_gcc9_coverage_test2 (1/4)Step: "Run tests" (full log | diagnosis details | 🔁 rerun)
|
Job | Step | Action |
---|---|---|
pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test2 | 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 to the (internal) Dr. CI Users group.
ghstack-source-id: 6cde5155c57cde7645c230b7fce2e37b2d1ddd85 Pull Request resolved: #51605
808315a
to
7226a54
Compare
} else if (!first_none && second_none) { | ||
return OptionalType::create(first_type); | ||
} else { | ||
throw ErrorReport(subscript.range()) << err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, Union[None, None]
would trigger this line and error out, is this intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That should be handled during the regular Union.
return u | ||
else: | ||
return 0 | ||
scripted = torch.jit.script(test1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional:
Use self.checkScript
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was told self.checkScript
is the old way in one of my earlier PR's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, didn't know about that. What's wrong with it and what's the alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikithamalgifb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@nikithamalgifb merged this pull request in ecf8166. |
ghstack-source-id: 32db9661ce0f9441ef7061285bc24967c2808ea6
Pull Request resolved: #51605
Fixes #51582
Summary:
In Python 3.9+ Union[T, NoneType] and Union[NoneType, T] as OptionalType.
Test:
python test/test_jit.py -v TestJit.test_union_to_optional