[jit] Report errors through call stack #22280
Conversation
pytorchbot
added
caffe2
oncall: jit
driazati
changed the title
pytorchbot
added
the
module: pybind
zdevito
left a comment
zdevito
left a comment
There was a problem hiding this comment.
Looks pretty good. There is one change in compiler that should be make otherwise I think a lot of potential errors are missed, but that change is pretty easy to make.
torch/csrc/jit/script/compiler.cpp
Outdated
| return sv->attr(select.range(), method, select.selector().name()); | ||
| } | ||
| case TK_APPLY: { | ||
| // Push the source range of a call in case compiling this function |
There was a problem hiding this comment.
TK_APPLY is not the only call that can trigger recursive compilation. There are a ton of them: magic method invocations, method invocations (they won't go through TK_APPLY), various sugared value things that invoke toIValue type things. It is probably not sustainable to be able to find all these places. Can we instead do something like record the current source Range of the Node being emitted for every call to emitStmt and emitSugaredExpr, and then if we end up in a recursive compilation scenario (i.e. there is a call to push_function that occurs later), we can add this source range to the stack.
There was a problem hiding this comment.
The way it is setup this would be something like set_source_for_current_function, with push_function and pop_function being the only things that adjust the depth of the stack.
test/test_jit.py
Outdated
| def forward(self, x): | ||
| return b(x) | ||
|
|
||
|
|
There was a problem hiding this comment.
We probably want one test for something that gets compiled because of a magic method.
…ent function and update it as necessary
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@driazati is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
The error for `test_error_stack_module`:
```
Traceback (most recent call last):
File "../test.py", line 35, in <module>
scripted = torch.jit.script(M())
File "/home/davidriazati/other/pytorch/torch/jit/__init__.py", line 1119, in script
return _convert_to_script_module(obj)
File "/home/davidriazati/other/pytorch/torch/jit/__init__.py", line 1825, in _convert_to_script_module
raise e
RuntimeError:
d(int x) -> int:
Expected a value of type 'int' for argument 'x' but instead found type 'str'.
:
at ../test.py:11:12
def c(x):
return d("hello") + d(x)
~ <--- HERE
'c' is being compiled since it was called from 'b'
at ../test.py:14:12
def b(x):
return c(x)
~~~ <--- HERE
'b' is being compiled since it was called from 'forward'
at ../test.py:22:16
def forward(self, x):
return b(x)
~~~ <--- HERE
'forward' is being compiled since it was called from 'forward'
at ../test.py:31:20
def forward(self, x):
return x + self.submodule(x)
~~~~~~~~~~~~~~~~ <--- HERE
```
This also unifies our error reporting in the front end with `ErrorReport`
TODO
* Include module names in message, #22207 should make this easy
](https://our.intern.facebook.com/intern/diff/16060781/)
Pull Request resolved: pytorch/pytorch#22280
Pulled By: driazati
Differential Revision: D16060781
fbshipit-source-id: c42968b53aaddb774ac69d5abbf7e60c23df8eed
Following on to #22280, this adds module names so they're included in the call stacks of an error message (e.g. so it appears as `M.forward` instead of `forward`)
Summary: Following on to #22280, this adds module names so they're included in the call stacks of an error message (e.g. so it appears as `M.forward` instead of `forward`) ](https://our.intern.facebook.com/intern/diff/16287925/) Pull Request resolved: #22921 Pulled By: driazati Differential Revision: D16287925 fbshipit-source-id: 6f31d72caa87ba2dc527805d36f7d62eb94c0808
The error for
test_error_stack_module:This also unifies our error reporting in the front end with
ErrorReportTODO
Differential Revision: D16060781