-
Notifications
You must be signed in to change notification settings - Fork 609
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
chore(repr): simplify Expr.__rich_console__(), move generic error handling up to _repr_mimebundle_ #9811
Conversation
…dling up to _repr_mimebundle_ First, remove the unneeded usage of rich.Text. Since we aren't applying any styles, wrapping the vanilla python str in rich.Text has no effect. Second, move the generic exception catching and printing to the _repr_mimebundle_() method. If an exception occurs during plain usage of __rich_console__, not in the context of ipython, then the error will get surfaced as normal. Only if we are in the context of ipython would the error get sliently eaten. Therefore, only deal with this edge case in _repr_mimebundle(). We now print the exception with traceback.print_exc(), this might be slightly different from console.print_exception(), but similar enough. Third, simply pass the options.max_width to pretty.to_rich(). This causes, when in jupyter, this function to be called with a max_width of 1_000_000 instead of None (which would get interpreted as infinite). In functionality, these both mean the same thing since 1_000_000 is effectively infinite, but this new method uses slightly less branching logic.
try: | ||
bundle = super()._repr_mimebundle_(*args, **kwargs) | ||
bundle["text/plain"] = bundle["text/plain"].rstrip() | ||
return bundle | ||
except Exception as e: | ||
# In IPython (but not other REPLs), exceptions inside of | ||
# _repr_mimebundle_ are swallowed to allow calling several display | ||
# functions and choosing to display the "best" based on some priority. | ||
# | ||
# This means that exceptions during interactive repr are silently caught. | ||
# We can't stop the exception from being swallowed, but at least we can print them. | ||
traceback.print_exc() | ||
raise e |
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.
try: | |
bundle = super()._repr_mimebundle_(*args, **kwargs) | |
bundle["text/plain"] = bundle["text/plain"].rstrip() | |
return bundle | |
except Exception as e: | |
# In IPython (but not other REPLs), exceptions inside of | |
# _repr_mimebundle_ are swallowed to allow calling several display | |
# functions and choosing to display the "best" based on some priority. | |
# | |
# This means that exceptions during interactive repr are silently caught. | |
# We can't stop the exception from being swallowed, but at least we can print them. | |
traceback.print_exc() | |
raise e | |
try: | |
bundle = super()._repr_mimebundle_(*args, **kwargs) | |
except Exception as e: | |
# In IPython (but not other REPLs), exceptions inside of | |
# _repr_mimebundle_ are swallowed to allow calling several display | |
# functions and choosing to display the "best" based on some priority. | |
# | |
# This means that exceptions during interactive repr are silently caught. | |
# We can't stop the exception from being swallowed, but at least we can print them. | |
traceback.print_exc() | |
raise e | |
else: | |
bundle["text/plain"] = bundle["text/plain"].rstrip() | |
return bundle |
Do not bundle the code that is catching the exception you care about with other code that can raise an exception that is unrelated to the thing you're trying to handle.
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.
If an error is raised within those last two lines (admittedly very very less likely) then Ipython still is gonna swallow the error silently. I wanted to still see that error. If there were some error handler further up the call stack that would catch the more general error, then I'd be all for restricting our try block to as small as possible. But there is no parent handler, so this is the only chance we have to see it.
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.
@cpcloud what do you think? I'm willing to change if you still think this is the right move.
Going to see whether this helps with the duplicate exception rendering. |
It doesn't. |
Actually, I think the printing and special handling are entirely unnecessary. The problem of not being able to catch exceptions in a However, the traceback seems to show up nowadays in both the notebook and the IPython console, and our special handling of things is causing really awful docs pages that repeat the exception three times. I'm going to try to remove all of our special handling. |
There's some weird issue with implementing |
Closing this PR, will open another. |
First, remove the unneeded usage of rich.Text. Since we aren't applying any styles, wrapping the vanilla python str in rich.Text has no effect.
Second, move the generic exception catching and printing to the repr_mimebundle() method.
If an exception occurs during plain usage of rich_console, not in the context of ipython, then the error will get surfaced as normal. Only if we are in the context of ipython would the error get sliently eaten. Therefore, only deal with this edge case in _repr_mimebundle().
We now print the exception with traceback.print_exc(), this might be slightly different from console.print_exception(), but similar enough.
Third, simply pass the options.max_width to pretty.to_rich(). This causes, when in jupyter, this function to be called with a max_width of 1_000_000 instead of None (which would get interpreted as infinite). In functionality, these both mean the same thing since 1_000_000 is effectively infinite, but this new method uses slightly less branching logic.
Fourth, now the try/catch for the
TranslationError
is much more tightly scoped to where we expect it to happen, during the actual expression evaluation in to_rich(). If we somehow got this during _noninteractive_repr(), we would want this to actually error and not get caught, that is an indication we have a bug somewhere.