[JIT] metacompile isinstance checks #23885
Conversation
eellison
changed the title
This is a series of PRs that will allow us to support adding [padding to conv](#22484) and also reduce the friction of adding method overloads that was brought up in #23266. This PR only compiles one if branch if the condition is an isinstance check. This is consistent with what mypy does; it does not report errors if a branch can be determined statically to be unreachable. ``` from typing import overload def foo(x): # type: (int) -> int if isinstance(x, str): return x["1"] return x + 1 reveal_type(foo) # no error, shows int -> int ```
|
|
||
| FileCheck().check_not("prim::PythonOp").run(cu.test.graph) | ||
|
|
||
| def test_isinstance_metacompile(self): |
There was a problem hiding this comment.
This should also check something where the value could be either type, i.e. a class attribute
class M(torch.nn.Module):
def __init__(self, x):
self.x = x
...
torch.jit.script(M(2))
torch.jit.script(M('hi'))There was a problem hiding this comment.
isinstance is supposed to throw if it can't be statically resolved (don't have runtime support); that's an existing limitation
There was a problem hiding this comment.
It is still statically resolved here, each module statically has a different type for that attribute name
| // corresponding branches | ||
| Expr cond = stmt.cond(); | ||
| bool isinstance_call = isIsInstanceCall(cond); | ||
| bool potential_none_check = !isinstance_call && isPotentialNoneCheck(cond); |
There was a problem hiding this comment.
nit: Is isIsInstanceCall(cond) ever true when isPotentialNoneCheck(cond) is true? Otherwise, seems like this conditional can just be bool potential_none_check = isPotentialNoneCheck(cond).
There was a problem hiding this comment.
This is maybe over-optimization. isPotentialNoneCheck(cond) is never true when isIsInstanceCall(cond) is true. So here I am only computing isPotentialNoneCheck when isIsInstanceCall is false.
If you think i should remove it i will
| if (isinstance_call) { | ||
| auto is_instance_result = emitSugaredExpr(cond, 1); | ||
| auto ivalue = toIValue(is_instance_result->asValue(cond.range(), method)); | ||
| TORCH_INTERNAL_ASSERT(ivalue); // no support for runtime checks |
There was a problem hiding this comment.
nit: Shouldn't this be an user facing error? My understanding is that TORCH_INTERNAL_ASSERT is reserved for things that should never happen.
There was a problem hiding this comment.
This should never happen. If you look at implementation of isinstance it always returns a constant
Chillee
left a comment
Chillee
left a comment
There was a problem hiding this comment.
Looks fairly straightforward and logic checks out.
This is a series of PRs that will allow us to support adding [padding to conv](#22484) and also reduce the friction of adding method overloads that was brought up in #23266. This PR only compiles one if branch if the condition is an isinstance check. This is consistent with what mypy does; it does not report errors if a branch can be determined statically to be unreachable. ``` def foo(x): # type: (int) -> int if isinstance(x, str): return x["1"] return x + 1 reveal_type(foo) # no error, shows int -> int ``` Differential Revision: [D16697092](https://our.internmc.facebook.com/intern/diff/D16697092)
ghstack-source-id: 141fa52 Pull Request resolved: pytorch#23885
This is a series of PRs that will allow us to support adding padding to conv and also reduce the friction of adding method overloads that was brought up in #23266.
This PR only compiles one if branch if the condition is an isinstance check. This is consistent with what mypy does; it does not report errors if a branch can be determined statically to be unreachable.
Stack from ghstack:
Differential Revision: D16697092