Monkey patch traceback.TracebackException to support MultiError #347

nmalaguti · 2017-11-06T16:01:26Z

This monkey patches traceback.TracebackException on import. It overrides TracebackException.__init__() to extract any inner exceptions from MultiErrors and overrides TracebackException.format() to include the details of the embedded exceptions.

This deprecates trio.format_exception as it is no longer needed and is now an alias for traceback.format_exception.

Documentation is included detailing some examples where this would be useful e.g. logging.

See gh-305.

codecov · 2017-11-06T19:28:17Z

Codecov Report

Merging #347 into master will decrease coverage by <.01%.
The diff coverage is 98.48%.

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
- Coverage   99.24%   99.24%   -0.01%     
==========================================
  Files          87       87              
  Lines       10397    10436      +39     
  Branches      729      728       -1     
==========================================
+ Hits        10319    10357      +38     
  Misses         61       61              
- Partials       17       18       +1

Impacted Files	Coverage Δ
trio/_toplevel_core_reexports.py	`100% <ø> (ø)`	⬆️
trio/_core/tests/test_multierror.py	`100% <100%> (ø)`	⬆️
trio/_core/_multierror.py	`99.4% <95.83%> (-0.6%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3edfafe...dd68087. Read the comment docs.

nmalaguti · 2017-11-06T20:51:48Z

trio/_core/_multierror.py

@@ -6,6 +6,9 @@
 from contextlib import contextmanager

 import attr
+import logging


I don't think this import is used at all anymore. I should remove it.

nmalaguti · 2017-11-06T20:52:41Z

trio/_core/_multierror.py

-# format_exception's semantics for limit= are odd: they apply separately to
-# each traceback. I'm not sure how much sense this makes, but we copy it
-# anyway.
+@deprecated("0.2.0", issue=305, instead="traceback.format_exception")


I wasn't sure what issue number to use for this. I used #305 but it doesn't really address the deprecation. Should I make an issue specifically for this?

You can also use the PR number if it makes more sense - which in this case it probably does, I agree.

(This works because github doesn't fully distinguish between PRs and issues – they use the same number space, and the URL for "issue 347" redirects to the PR page if 347 is a PR.)

nmalaguti · 2017-11-06T20:54:19Z

trio/_core/_multierror.py

-            chunks += _format_exception_multi(
-                seen, type(v), v, v.__traceback__, limit=limit, chain=True
+    if isinstance(exc_value, MultiError):
+        embedded = []


There is no explicit seen behavior here anymore to log duplicate exceptions. The behavior of TracebackException is just to omit it if it has been seen.

Is that okay?

njsmith

From gitter:

how important is it that a duplicate exception (exc.__cause__ == exc) be printed as a duplicate instead of just omitted?

The current code is really careful about duplicate exceptions, because when I was first writing the MultiError code I had tons of bugs that caused weird reference loops, so I had to make the pretty-printer bulletproof just to figure out what was going on :-). This is probably less important now. It is probably still more important for us than for the stdlib version, because for them the only way you can get a duplicate is if you have something like A.__context__ is A, which is a weird bug and arbitrarily cutting off the display seems ok? For us we can have things like MultiError([A, B]) where A.__context__ is B.__context__, and I feel like here it's a little more confusing if B is just randomly missing its context without any clue that something has happened (especially if A and B are in different parts of the tree).

I guess one approach would be to have the capture code only skip loops, but not throw away "horizontal" duplicates. (Something like, when we recurse we pass through a seen set, but we make a separate copy of it for each entry in a MultiError.) And then on output we can filter more? Or we could define a special placeholder object for "duplicate" that we use when capturing? Can you give some estimate of how hard it would be to keep this?

njsmith · 2017-11-06T20:13:21Z

trio/_core/_multierror.py

+        lookup_lines=True,
+        capture_locals=False,
+        _seen=None
+):


I think we want an

if _seen is None: _seen = set()

here before we call anything else.

njsmith · 2017-11-06T20:32:14Z

trio/_core/_multierror.py

-                seen, type(v), v, v.__traceback__, limit=limit, chain=True
+    if isinstance(exc_value, MultiError):
+        embedded = []
+        for exc in exc_value.exceptions:


Do we want an if exc not in _seen here? I guess it's needed if we want to be absolutely bulletproof. But the only case where it's absolutely necessary is when you have a MultiError that (transitively) embeds itself, which is ... not something that should ever happen. MultiError.__repr__ probably crashes in this case too. So maybe we don't need to worry about it...

njsmith · 2017-11-06T20:33:46Z

trio/_core/tests/test_multierror.py

+           'use traceback.format_exception instead' in record[0].message.args[0]
+
+
+def test_logging(caplog):


Oh excellent, I was going to ask you to copy this test over from the other PR but I see you're way ahead of me :-)

nmalaguti · 2017-11-12T17:56:56Z

I guess one approach would be to have the capture code only skip loops, but not throw away "horizontal" duplicates. (Something like, when we recurse we pass through a seen set, but we make a separate copy of it for each entry in a MultiError.)

Just putting down some thoughts to make sure I understand what each part of the multierror actually represents.

MultiError traceback: This is the stack from where the MultiError was created to where it was handled.

MultiError context or cause: This is any exception that was being handled when the MultiError was created (like if there was a nursery in an except block).

By using the stdlib traceback formatting code, this will work as desired and there should be no duplicates that need to be explicitly printed.

MultiError sub-exceptions: Each want to be printed separately. If there are duplicates, especially in different parts of the "tree", we probably want to mention that instead of silently omitting them.

This matches with what you said about "horizontal" duplicates and using a copy of our _seen at that point for each. I say a copy rather than an empty seen on the off chance that somehow the context of the MultiError we are handling appears as a cause or context of the sub exception, in which case it would be omitted.

In this way, all sub-exceptions will be printed with a reasonable stack of causes, and there may be duplicates in those stacks. In order to re-use the existing TracebackException::format code, we won't be able to mention that the duplicates are duplicates, but as long as the stacks are there it should be okay.

nmalaguti · 2017-11-12T18:35:36Z

@njsmith The only open question left is whether it is important to explicitly mention that we have printed a duplicate exception or if simply printing it is sufficient.

If we want to be explicit that it is a duplicate, we'll need to copy the code from TracebackException::__init__ and print "duplicate" when an exception is in _seen instead of omitting it. I'm not a huge fan of copying the code, but it isn't a lot. I'm also not sure how important it is to know that the exception is the same one, rather than just looks the same.

This monkey patches `traceback.TracebackException` on import. It overrides `TracebackException.__init__()` to extract any inner exceptions from `MultiError`s and overrides `TracebackException.format()` to include the details of the embedded exceptions. This deprecates `trio.format_exception` as it is no longer needed and is now an alias for `traceback.format_exception`. Documentation is included detailing some examples where this would be useful e.g. logging. See python-triogh-305.

njsmith · 2017-11-28T02:59:11Z

Sorry for leaving you hanging on this! I think this is looking good, and if we decide we want to tweak it more, we can always do that later :-). Thanks!

Deprecated in 0.2.0 (see python-triogh-347)

nmalaguti force-pushed the feature/monkeypatch-traceback branch from 5462841 to f775909 Compare November 6, 2017 18:42

python-trio deleted a comment from codecov bot Nov 6, 2017

nmalaguti commented Nov 6, 2017

View reviewed changes

nmalaguti requested a review from njsmith November 6, 2017 21:38

njsmith reviewed Nov 11, 2017

View reviewed changes

nmalaguti force-pushed the feature/monkeypatch-traceback branch from f775909 to bfeca05 Compare November 12, 2017 18:28

nmalaguti force-pushed the feature/monkeypatch-traceback branch from e3e4f75 to dd68087 Compare November 14, 2017 01:38

njsmith merged commit 399b378 into python-trio:master Nov 28, 2017

njsmith mentioned this pull request Nov 28, 2017

Add a custom stdlib logging.Formatter for MultiErrors #344

Closed

njsmith added a commit to njsmith/trio that referenced this pull request Dec 25, 2017

Remove trio.format_exception

158e8dd

Deprecated in 0.2.0 (see python-triogh-347)

njsmith mentioned this pull request Dec 25, 2017

Remove everything else that was deprecated in 0.2.0 #389

Merged

njsmith added a commit to njsmith/trio that referenced this pull request Dec 25, 2017

Remove trio.format_exception

a246437

Deprecated in 0.2.0 (see python-triogh-347)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monkey patch traceback.TracebackException to support MultiError #347

Monkey patch traceback.TracebackException to support MultiError #347

nmalaguti commented Nov 6, 2017

codecov bot commented Nov 6, 2017 •

edited

Loading

nmalaguti Nov 6, 2017

njsmith Nov 11, 2017

nmalaguti Nov 6, 2017

njsmith Nov 11, 2017

njsmith Nov 11, 2017

nmalaguti Nov 6, 2017

njsmith left a comment

njsmith Nov 6, 2017

njsmith Nov 6, 2017

njsmith Nov 6, 2017

nmalaguti commented Nov 12, 2017

nmalaguti commented Nov 12, 2017

njsmith commented Nov 28, 2017

		'use traceback.format_exception instead' in record[0].message.args[0]


		def test_logging(caplog):

Monkey patch traceback.TracebackException to support MultiError #347

Monkey patch traceback.TracebackException to support MultiError #347

Conversation

nmalaguti commented Nov 6, 2017

codecov bot commented Nov 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmalaguti commented Nov 12, 2017

nmalaguti commented Nov 12, 2017

njsmith commented Nov 28, 2017

codecov bot commented Nov 6, 2017 •

edited

Loading