[jit] always use the closure to resolve variable names #27515
Conversation
Resoving variable names using the local activation frames does not work when using recursive scripting, but our current code tries to do it (incorrectly) anyway. The reason it works is only because the script call is in the same local frame as the definition. This will not be true in practice and makes it seem like the API works in more cases than it really does. This forces us to always use closure-based annotations, documents it, and it fixes the tests so that they still pass.
pytorchbot
added
oncall: jit
suo
left a comment
suo
left a comment
There was a problem hiding this comment.
Nice! I think, since we can't kill the stack-based rcb entirely, it's worth putting a comment in createResolutionCallback clearly explaining when it should (and should not!) be used.
| # functions will be defined at a global scope like MyGlobalClass. In cases | ||
| # where they are not, it is possible to work around issues by declaring the | ||
| # values global in the function. | ||
|
|
There was a problem hiding this comment.
Today, if the user names a type that we can't find, it'll be inferred to be a Tensor with rather confusing results. I wonder if there's an easy way to improve the error message in this case.
If there isn't…then I guess it doesn't matter because it's unlikely that anyone outside our tests will do it.
There was a problem hiding this comment.
I can't repro this issue, I always get a Unknown type name 'FooClass'
test/test_jit.py
Outdated
| cu.define(full) | ||
|
|
||
| def test_namedtuple_python(self): | ||
| global MyTuple, MyMod |
There was a problem hiding this comment.
add the same "[local resolution in python note]" as the others?
| _frames_up = _frames_up + 1 # for invoking _gen_rcb() | ||
|
|
||
| closure_rcb = _jit_internal.createResolutionCallbackFromClosure(obj) | ||
| stack_rcb = _jit_internal.createResolutionCallback(_frames_up + 1) |
There was a problem hiding this comment.
It's worth examining the remaining uses of createResolutionCallback to check if they are being used properly. For example, the one on line 1220 on this file seems suspicious if we are trying to recursively script classes (which we're not doing today, I think?).
There was a problem hiding this comment.
I went through and renamed getResolutionCallback to getResolutionCallbackFromFrame. The remaining use cases are correct and are (1) used in define, where we want to take the callers scope and resolve variables in it, and (2) for class/interface declarations which are always annotated. There is a recursive class compilation case that I noticed that David added, but it isn't applied consistently yet, so I am not going to touch it yet.
[jit] always use the closure to resolve variable names Resoving variable names using the local activation frames does not work when using recursive scripting, but our current code tries to do it (incorrectly) anyway. The reason it works is only because the script call is in the same local frame as the definition. This will not be true in practice and makes it seem like the API works in more cases than it really does. This forces us to always use closure-based annotations, documents it, and it fixes the tests so that they still pass. gh-metadata: pytorch pytorch 27515 gh/zdevito/121/head
…#27515)" I suspect the introduction of these globals is what is causing the py2 pybind errors on master. Let's see what happens with CI.
Summary: Pull Request resolved: pytorch#27515 Resoving variable names using the local activation frames does not work when using recursive scripting, but our current code tries to do it (incorrectly) anyway. The reason it works is only because the script call is in the same local frame as the definition. This will not be true in practice and makes it seem like the API works in more cases than it really does. This forces us to always use closure-based annotations, documents it, and it fixes the tests so that they still pass. Test Plan: Imported from OSS Differential Revision: D17803403 Pulled By: zdevito fbshipit-source-id: e172559c655b05f0acf96c34f5bdc849f4e09ce2
Stack from ghstack:
Resoving variable names using the local activation frames does not work
when using recursive scripting, but our current code tries to do it
(incorrectly) anyway. The reason it works is only because the script
call is in the same local frame as the definition. This will not be
true in practice and makes it seem like the API works in more cases
than it really does. This forces us to always use closure-based annotations,
documents it, and it fixes the tests so that they still pass.
Differential Revision: D17803403