-
Notifications
You must be signed in to change notification settings - Fork 81
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
Make exceptions from user code more readable #1460
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
Thank you @apaz-cli , the general idea seems OK, but we would
- need a flag that gives the full traceback (register
DebugOptions.interpreter_traceback
), - need a test for both full traceback and reduced..
Could you add these please?
Also the code comments.
return co.co_filename.endswith("thunder" + os.sep + "core" + os.sep + "interpreter.py") and ( | ||
co.co_name in ("fn_", "fn_2") | ||
) |
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.
- Why just fn_ and fn_2 and not all of the methods in the interpreter?
- Please don't use the filename/code object here. Instead check whether that is in f_globals of the frame has
__name__ == 'thunder.core.interpreter'
.
If we drop all interpreter methods, we likely want to keep internal frames at the top of the stacktrace.
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.
Excluding everything in thunder.core.interpreter
sounds good. I was trying to be conservative by default.
But using f_globals
to do that instead of co_name
doesn't work, because not every function has a f_globals
with a __name__
in it. Certainly it would not work with some uses of the exec()
lookaside. It also depends on how exactly the module is imported. There are a bunch of reasons why it doesn't work.
As for the functions filtered out by code object, those live outside of interpreter.py
, in files that I don't want to exclude because some of their functions could be called by user code. And using the code objects is very convenient because it exactly matches the functions to exclude, and we wouldn't want to exclude all of thunder.__init__
.
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.
Which functions we want to filter does not have an f_globals with __name__
?
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.
thunder_general_jit()
does not. When I add an assertion that f_globals
has a __name__
, it fails.
⚡ ap-readable_exceptions ~/lightning-thunder python exctest.py
Traceback (most recent call last):
File "/teamspace/studios/this_studio/lightning-thunder/exctest.py", line 14, in <module>
jfn()
File "/teamspace/studios/this_studio/lightning-thunder/thunder/__init__.py", line 836, in _thunder_unwrap_inner_exception
assert hasattr(tb.tb_frame.f_globals, "__name__"), tb.tb_frame.f_code.co_name
AssertionError: thunder_general_jit
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.
That's because f_globals is a mapping, so it's members, not attributes.
@@ -814,7 +814,50 @@ def maybe_call_epilogue(cache_entry, result, pro_to_epi): | |||
|
|||
return result | |||
|
|||
def unwrap_inner_exception(c: Callable) -> Callable: |
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 wonder if this could live in the interpreter instead.
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.
The function can. The unwrapping actually can't, tried that already. Traceback frames are pushed as the stack unwinds, not when the exception is thrown.
@t-vi I can't think of any situation where I would want to see the full traceback. This change is only triggered in situations where the exception comes from user code inside the interpreter. If there's an interpreter bug where the path is relevant, this is not triggered. If there's a bug, then the bug is in their code, and not the interpreter, so the path through the interpreter is known to be irrelevant. |
Can you please add the flag to get the full traceback? Thank you. |
Code:
After:
Before: