classification
Title: traceback: add a new thin class storing a traceback without storing local variables
Type: enhancement Stage:
Components: asyncio, Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: adaptivelogic, eric.snow, gvanrossum, haypo, mahmoud, martin.panter, martius, ncoghlan, ned.deily, pitrou, python-dev, rbcollins, rhettinger, terry.reedy, xonatius, yselivanov
Priority: normal Keywords: patch

Created on 2013-05-05 21:31 by gvanrossum, last changed 2016-06-29 05:00 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
traceback.patch adaptivelogic, 2013-07-11 21:15 review
traceback2.patch adaptivelogic, 2013-07-12 15:13 Better documentation, fixed edge cases review
linecache_1.patch rbcollins, 2015-01-14 01:37
linecache_2.patch rbcollins, 2015-01-19 02:09 review
frame_1.patch rbcollins, 2015-01-19 02:09 review
tb_stats_1.patch rbcollins, 2015-01-19 02:12
issue17911-1.patch rbcollins, 2015-01-20 02:38 review
issue17911-2.patch rbcollins, 2015-01-26 03:18 review
issue17911-3.patch rbcollins, 2015-01-26 21:48 review
issue17911-4.patch rbcollins, 2015-02-03 23:28 review
qualname_in_exceptions.patch xonatius, 2015-02-07 08:23 Prints qualnames in function call exceptions. review
issue17911-5.patch rbcollins, 2015-03-03 22:24 review
issue17911-6.patch rbcollins, 2015-03-03 22:56 review
issue-19711-8.patch rbcollins, 2015-03-05 02:20 review
Messages (77)
msg188467 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-05 21:31
For Tulip I may have to extract tracebacks and store the extracted data, in order to avoid cycles created by the frames referenced in the traceback.  I'll be extracting the traceback every time an exception is raised, and in most cases I won't be using the extracted info, because usually the exception is caught (or logged) by user code.  But it's very important to ensure that if the user code doesn't catch or log it, I can log a traceback, and I won't know that this is the case until a destructor is called, which may be quite a bit later.  (Reference: http://code.google.com/p/tulip/source/browse/tulip/futures.py#38)

Unfortunately it looks like the public APIs do a lot more work than needed.  Ideally, there'd be something similar to _extract_tb_or_stack_iter() that doesn't call linecache.getline() -- it should just return triples of (filename, lineno, functionname), and enough structure to tell apart the __cause__, __context__, and __traceback__ (the first two possibly repeated).  Given this info it would be simple enough to format and log the actual traceback, and storing just this would take less space and time than computing the lines of the fully-formatted traceback.
msg188530 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-06 13:06
I think instead we may want to add a finalize() or close() method on frame objects which would clear all local variables (as well as dereference the globals dict, perhaps), after having optionally run a generator's close() method (if the frame belongs to a generator).

If I'm not mistaken, it should allow breaking reference cycles, and remove the need for complex traceback processing, which Twisted currently also does: http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py#L89

Note that generator cleanup through the frame has a patch in issue17807.
msg188548 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-06 14:39
I'm not snubbing my nose at something that breaks these types of cycles more easily, but I still think that the abstractions offered by the traceback module could be improved.
msg188554 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-06 14:46
On Mon, May 6, 2013 at 6:06 AM, Antoine Pitrou <report@bugs.python.org> wrote:
> Note that generator cleanup through the frame has a patch in issue17807.

Sadly that doesn't help with my issue. I applied path gen3.patch and
ran various versions of the code I have. There's still something
holding on to the Task or the exception.
msg188556 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-06 14:53
> On Mon, May 6, 2013 at 6:06 AM, Antoine Pitrou
> <report@bugs.python.org> wrote:
> > Note that generator cleanup through the frame has a patch in
> > issue17807.
> 
> Sadly that doesn't help with my issue.

It won't. It's just a building block for the change I've proposed here.
msg188616 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-05-07 01:42
Antoine - I like your idea, but I think it's a separate issue. I agree with Guido that exposing some lower level non-formatting APIs in the traceback module would be helpful.

I see Guido's suggestion here as similar to the change we just made in the dis module to expose a dis.get_instructions iterator. That change makes it much easier to work with the disassembler output programmatically, whereas the module was previously too focused on displaying text with a specific format.

My current thoughts: define a "TracebackSummary" with the following fields:

    exc_type
    exc_repr
    stack_summary
    cause
    context

stack_summary would be a list of (filename, lineno, functionname) triples as Guido suggested (probably a named tuple)

cause and context would be either None or a reference to an appropriate TracebackSummary object

Basically, the summary should contain all of the information needed to create the formatted traceback output, without actually doing any formatting (aside from calling repr() on the exception)
msg188620 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-05-07 03:39
That sounds great, except I'd expect the exception type and str(), since a formatted traceback uses the str() of the exception, not its repr().

Personally I don't think the tuples need to be named, but I won't hold up progress either. :-)
msg188648 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-07 13:13
You may want two pieces of stack: the piece of stack that is above and including the catch point, and the piece of stack that is below it. The former is what gets traditionally printed in tracebacks, but the latter can be useful as well (see issue1553375 for a related feature request).
msg188719 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-08 11:55
Ok, I've spinned off the frame clearing suggestion to issue17934.
msg192672 - (view) Author: Björn Sandberg Lynch (adaptivelogic) Date: 2013-07-08 17:04
I've been looking into this as an easy piece to bite off. If I understand Guido correctly, he'd like to defer or suppress the linecache call when getting the tb summary. The problem with deferring is that you need to access f_globals for the loader to work correctly when the module source is a non-file import source. If we keep a reference to f_globals for each line in the traceback, we can defer this to later (ideally we'd just keep the __loader__ value, but that would require changing the linecache interface as well).

My inclination would be to have another keyword argument to _extract_tb_or_stack_iter (terse=False or verbose=True - either style works). In terse mode, no source lines would be available, and the formatted output would be the same as if the source wasn't available at all. This would work, although the traceback module is structured so that I'd need to pass it through quite a few wrapped iterator calls.

I'm not sure how free a hand I have when it comes to refactoring the internal implementation. I'm not fond of the extractor callbacks - I'd prefer a generator-based approach on the lines of:

def _tb_iter(tb, limit):
    i = 0
    while tb is not None:
        if limit is not None and limit < i:
            break
        yield tb.tb_frame, tb.tb_lineno
        tb = tb.tb_next
        i += 1

def _extract_frame_iter(frames, terse=False):
    ...
    for frame, lineno in frames:
    ...
msg192690 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-07-08 20:07
In terms of how much freedom you have about changing the internal, I'd check how long ago they were changed.  "Internal" APIs that have been stable for many versions tend to have illicit external uses -- but internal APIs that were introduced recently (e.g. in 3.2) are usually safe to use -- nobody is going to make too much of a stink if you break their code.

As for saving f_globals, if you're going to save an extra pointer anyways, why not just save the frame pointer?
msg192728 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-07-09 08:32
> If we keep a reference to f_globals for each line in the traceback, we 
> can defer this to later (ideally we'd just keep the __loader__ value,
> but that would require changing the linecache interface as well).

Or you could add a *new* linecache interface.
Keeping __loader__ sounds much less hackish than keeping f_globals to me.
msg192909 - (view) Author: Björn Sandberg Lynch (adaptivelogic) Date: 2013-07-11 21:15
After thinking about it, I decided to defer instead of suppress line fetching. Suppressing would still give a traceback later, but not the same as the original.

extract_exception is where the meat is. There's a slight quirk in how context works - if cause is set to something else than the actual exception, context still gets set even though it won't be printed. This is to enable later analysis. However, if you explicitly 'raise Exception from None' neither of them will be set - the formatting code would have issues otherwise.

I'm not too happy about the code duplication between the format_exception method and the formatting that works on exceptions directly, but there are plenty of tests to ensure that the output is identical.

Feel free to dispute my naming choices. I'm admittedly influenced by twisted.
msg193272 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-07-18 09:42
I think ExceptionSummary could be the visible API, instead of adding an indirection through a separate global function.
Also, I don't think having the "exc_traceback" list of tuples is future-proof. What if we want to add some information to each traceback entry, or refactor it to take __loader__ into account?
Instead, ExceptionSummary could expose the desired operations (e.g. iterate over traceback lines and the associated source code lines) without being constrained by some implementation details.
msg193294 - (view) Author: Björn Sandberg Lynch (adaptivelogic) Date: 2013-07-18 13:35
I was trying to stay with the established pattern of the existing methods. There are two unrelated issues to solve here - deferring linecache access, and the extract_exception functionality.

When it comes to deferral, we could wrap each line in a different class than tuple, but this constitutes a public API change (and this is a *very* widely used library). Even changing to a namedtuple would cause a lot of grief, since we'd break lots of doctests, and subclassing tuple is a lot of effort for little benefit. If we only do it for the deferred usage, it would "only" be inconsistent. I suppose we could have a completely separate way of doing things for ExceptionSummary, but again, inconsistent, and I think if one extract_xxx method provides the functionality, users would expect it to be available in the others.

For ExceptionSummary, the same summary instance is used more than once if you have a cause set, so object creation had to be separated from the graph population. Either this is done in a constructor function or we would need some obscure tricks with the class kwargs. This way, there's a separation of concerns - the class wraps *one* exception, and the function creates a bunch and links them together as needed.
msg193296 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-07-18 13:45
> When it comes to deferral, we could wrap each line in a different
> class than tuple, but this constitutes a public API change (and this
> is a *very* widely used library). Even changing to a namedtuple
> would cause a lot of grief, since we'd break lots of doctests, and
> subclassing tuple is a lot of effort for little benefit. If we only
> do it for the deferred usage, it would "only" be inconsistent. I
> suppose we could have a completely separate way of doing things for
> ExceptionSummary, but again, inconsistent, and I think if one
> extract_xxx method provides the functionality, users would expect it
> to be available in the others.

YMMV, but I think we should go for inconsistency here. The other APIs
in the traceback module are low-level and clearly limited by the type
of objects they return.

This feature request is a good opportunity to design something a little
more future-proof. I'd love to know what other developers/contributors
think here.
msg193333 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-07-18 23:06
For dis, we introduced a new rich bytecode introspection API. Ditto for
inspect.Signature. I think it makes sense to treat this as a completely new
traceback introspection API and ignore the low level details of the legacy
API.
msg220381 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-06-12 20:11
>  I think it makes sense to treat this as a completely new
> traceback introspection API and ignore the low level details
> of the legacy API.

That would likely be the cleanest approach.
msg220390 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-06-12 21:48
While trying to fix a similar issue for the asyncio project, I wrote the following code:
https://bitbucket.org/haypo/misc/src/ce48d7b3ea1d223691e496e41aca8f5784671cd5/python/suppress_locals.py?at=default

I was not aware that a similar approach (attached traceback2.patch) was already implemented.

Asyncio issues:
https://code.google.com/p/tulip/issues/detail?id=155
https://code.google.com/p/tulip/issues/detail?id=42

See also the Python issue #20032.
msg231743 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-11-27 02:47
Belatedly looking at this again: one thing I noticed is that the summary currently loses info if __cause__ is set.

It would be better to follow the PEP 409/415 model and report the "__suppress_context__" flag as part of the summary, rather than dropping the context information.

That allows the decision on whether or not to show the context information to be deferred to display time, rather than discarding it eagerly.
msg231792 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-11-28 00:56
I'll put something together.
msg233895 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-12 22:53
w.r.t. a new linecache interface, it looks like we need two attributes from f_globals: __name__ and __loader__, so that we can eventually call __loader__.get_source(__name__).

One small change (to let me focus on traceback) would be to add another kw argument  to the existing calls that take module_globals, called e.g. get_source, which would be a simple callable (source = get_source()). For the deferred linecache situation, we'd then create partial(f_globals.__loader__.get_source, f_globals.__name__) and keep that around until we need the source. We could even abstract that out into a function in linecache to keep the knowledge in one place.

Another way to tackle this would be to add a new function to linecache that lazily seeds the cache: it would stash the get_source method and the name against the filename, and when getline[s] is called actually invoke it.

I think the second way is a bit nicer myself. What do folk think?
msg233995 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-14 01:37
Ok, here's a draft patch for linecache. Next up, poking around the new TB API.
msg234297 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-19 01:54
One thing I noticed while working on this, traceback today stats each included file once per frame per call into e.g. format_list. This might actually account for quite some of the overhead. I'll include an optimisation for this in my new API. http://paste.openstack.org/show/158714/
http://paste.openstack.org/show/158715/
msg234298 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-19 02:09
This is a WIP patch, including it to just share progress.
msg234299 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-19 02:12
And this improves the scaling of the cache stating overhead, but we may well want to fix this more fundamentally - e.g. by linking the cache ownership into the import system somehow, so that when a file is reimported, the cached source is automatically evicted, and format_stack etc have no need to stat at all.

I tested this with the following script in timeit:

import traceback

def recurse(count):
  if count> 0:
    return recurse(count - 1)
  return traceback.format_stack()

def doit():
    len(recurse(500))
msg234319 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-19 17:56
I've split out the stat question to http://bugs.python.org/issue23273 - we can optimise it slightly in this patch, but I think its scope creep here, and will be unclear, to dive after a full fix in this issue.
msg234338 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-20 02:38
Stack and Frame looking good, next update will be next Monday, when I finish off my TracebackException class.
msg234712 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 03:18
Right, and a usable API. 

I believe that this will meet Guido's use case:

tb = TracebackException(*sys.exc_info, lookup_lines=False)
....
some time later
....
if should_show_tb:
   lines = list(tb.format())

I'm not 100% sold on the public API being a generator, but rather than forcing it one way or the other, I'll let reviewers tell me what they think :)

Performance wise, this is better, with the following times:
format_stack -> 5.19ms
new API to extract and not format -> 3.06ms
new API to extract, not lookup lines and not format -> 2.32ms

Formatting is then 2-3ms per 500 line traceback actually formatted, which seems terribly slow, but its still better than the 8+ms trunk takes (see my earlier tests). I'll look at tuning the time to render an actual trace later, since I don't like paying high costs in unittest ;) - but AIUI this should be enough to help asyncio as is.

Updated test script I used to isolate times with timeit:
import traceback

def recurse(count, lookup_lines=True):
  if count> 0:
    return recurse(count - 1, lookup_lines=lookup_lines)
  if lookup_lines:
      return traceback.Stack.extract(traceback.walk_stack(None), lookup_lines=True)
  else:
      return traceback.Stack.extract(traceback.walk_stack(None), lookup_lines=False)

def doit():
    len(recurse(500))

def doit_lazy():
    len(recurse(500, False))
msg234740 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-26 13:12
I put some detailed comments in the review, but my main large scale concern is with the "traceback.Frame" name. Making "frame object" an ambiguous phrase seems like a bad idea to me, so I've suggested making it "FrameSummary" (or "FrameInfo", but on reflection I prefer the more explicit term). That would then suggest also making it "traceback.StackSummary" for consistency.

I think having the top level API be an iterable would be OK, but may be a little surprising. Is there a major advantage to avoiding building the whole traceback in memory?
msg234741 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-26 13:38
> tb = TracebackException(*sys.exc_info, lookup_lines=False)

You don't need an exception tuple in Python 3, the exception instance is enough.
msg234765 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 18:36
Thanks for the review - shall action it all as it seems all good improvements.

Two key notes: the use of an exception triple is useful to ease backports of this: a primary goal for me is being able to use the locals stuff in unittest for existing production code bases like OpenStack: there's a balance to be struck between new shiny features like __traceback__ and backporting. I'll consider the idea of value-or-triple; if all the consuming code is going to be using the triple interface I'm not sure the value one would, drumroll please, have value.

The locals stuff is as you note not complete or exposed yet. I wanted it in the design to be able to vet that it will actually be feasible without a later API break. I'm not -quite- there but I do also agree that it doesn't need to be in the output layer yet (in fact there's a different ticket for that anyway).
msg234769 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-26 18:59
Le 26/01/2015 19:36, Robert Collins a écrit :
> 
> Two key notes: the use of an exception triple is useful to ease
backports of this

Then I'd recommend making it exception-or-tuple (rather than a
*exc_info), so that both situations are satisfied.
msg234777 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 20:44
The generator thing: the code was refactoring not so long ago to be generator based internally with lists as a UI shim. I don't think there is a major advantage either way. Less buffering on one hand. Less convenient in some cases on the other. Simple use like ''.join(foo.format()) is going to be unaffected.
msg234781 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 21:48
And for profit, review changes applied (minus the small number I disagreed with). I've clarified in the code why the exc_info tuple break out is still used (compat with the legacy API is the strongest argument).

I haven't fleshed out the locals thing properly yet, I'm going to do one more iteration on that then ask for a final review before committing. (Unless we're in freeze ATM ?)
msg234782 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 22:04
actually, strike that, I'm happy with this pending a final +1 from another reviewer. Finishing the locals stuff is a separate patch, and will look like a single new parameter to StackSummary.extract, similarly on TracebackException.__init__ and then change the format logic to show it.

@Antoine - I'm not super happy with inspecting the first arg to do type-or-value stuff, if you feel its a super issue we can do it, but I'd rather wait and add it once we've got some feedback.
msg234788 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-26 22:46
On the triple-or-value front, the main case I see for considering the "you don't need exception state triples any more" argument already lost is the fact exc_info still returns a triple, and __exit__ methods still accept them as unpacked arguments. The C level exception handling APIs are also all still exception state triple based. In this view, __traceback__ is primarily about accessing *chained* tracebacks, while the top-level APIs still focus on exception state triples.

This perspective also has the virtue of confining use of the Python 3 only exception traceback attribute to code that is taking advantage of a Python 3 only feature (i.e. exception chaining). For chaining independent exception state handling, working with exception state triples then remains the "one obvious way to do it", a way which is conveniently also source compatible between Python 2 and 3.
msg234795 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-26 23:22
> In this view, __traceback__ is primarily about accessing *chained*
> tracebacks

Please let's not make up sophistic arguments when the only motivation for triples is backwards compatibility of *existing* APIs.

Exception triples are an unnecessary bother in Python 3. This new API should accept exception instances primarily. Accepting triples as well is ok but it shouldn't be made the only possibility.
msg234796 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-26 23:30
So its fairly simple IMO: it will be more code, not less, to support the non-triple API, *and* it can be added later, unless we're proposing not to support the triple API at all (which hasn't been proposed AFAICT).

To me thats a fairly strong argument for adding non-triple support later.
msg234797 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-26 23:34
Le 27/01/2015 00:30, Robert Collins a écrit :
> 
> Robert Collins added the comment:
> 
> So its fairly simple IMO: it will be more code, not less, to support
the non-triple API, *and* it can be added later, unless we're proposing
not to support the triple API at all (which hasn't been proposed AFAICT).

I am proposing it. Passing a single exception instance is the natural
way of expressing such an API. Crippling Python 3 APIs for the purpose
of backwards compatibility is not ok.

Nobody wants to write some_func(exc.__class__, exc, exc.__traceback__)
when they could simply write some_func(exc).
msg234804 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-01-27 01:05
Why do you consider it crippling?
msg234812 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-27 08:33
It's tedious and complicated compared to the natural way.
msg234820 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-27 13:23
Most of the time when I'm working heavily with exceptions it's something related to contextlib, so I'm likely getting my exception details from either sys.exc_info() or the arguments to __exit__. That means I start out with an exception triple, and the only time I need to look at type(exc) or exc.__traceback__ is when I'm following exception chains.

However, Antoine's right that if you got your exception from an *except clause* rather than one of the more indirect APIs, then you're just going to have the exception, rather than an exception triple.

So I think that's where the argument for accepting "exception-or-triple" comes from: handling an exception directly is for use with except clauses, while handling triples is convenient for use with sys.exc_info(), __exit__ methods and for general backwards compatibility.
msg235365 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-03 23:28
This iteration provides two constructors for TracebackException, one for exception objects, one for exc_info tuples. So it should be easy to use.

The __init__ takes the exc_info tuple because thats less code (much easier to destructure rather than restructure), but in the docs and docstrings I point to the classmethods, which includes from_exception - that takes a single exception object.
msg235428 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-05 10:49
+1 for having the base __init__ API be the exception triple and then a TracebackException.from_exception() class method as an alternate constructor.

However, the redundancy between TracebackException.__init__() and TracebackException.from_exc_tuple() feels very strange to me. It leaves the __init__ working-but-undocumented and isn't a pattern I can recall seeing anywhere else in the standard library.

Would it be possible to just add the from_exception() class method and leave the documented (and only) API for building from a triple being the normal constructor call?
msg235444 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-05 18:45
I can certainly do that; I was aiming to make it fair I guess, given Antoines strong feelings on this matter. As long as I'm not piggy-in-the-middle, I don't have a care in this regard :)
msg235446 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-02-05 19:09
I just posted some review comments.

I also realized that it posted some older comments that I hadn't submitted before... some of them may be obsolete, sorry :-/
msg235455 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-05 22:18
I think in this case, the fact that it's easier to decompose an exception
into the corresponding triple rather than vice-versa, together with the
fact that other exception state manipulation APIs like exc_info() and
__exit__() methods work with triples, means it makes sense to accept the
triple as the base constuctor API, and then have the convenience method to
destructure a caught exception.
msg235456 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-02-05 22:19
The solution of having two constructor classmethods actually seems nice
and symmetric to me.
msg235472 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-06 08:34
It's genuinely user hostile, as the "from_exc_tuple" version is entirely
redundant, and folks using pydoc to discover the API won't know they
"shouldn't" use the constructor directly.

That means both forms will get used, but only one will be documented. When
people ask why a second spelling for the lower level API was added, "to
make it look like it exists at the same level as the higher level
convenience API" isn't going to be a satisfying explanation.
msg235480 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-02-06 12:43
Le 06/02/2015 09:34, Nick Coghlan a écrit :
> 
> It's genuinely user hostile, as the "from_exc_tuple" version is entirely
> redundant, and folks using pydoc to discover the API won't know they
> "shouldn't" use the constructor directly.

Using pydoc isn't generally a successful way of discovering an API. It
shows you too many details that don't matter. Besides our docstrings are
much terser and less informative than the Sphinx doc.

pydoc is useful mostly as a Python-specific "man" command: you know
something exists, just want to remember the details.
msg235483 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-06 13:31
dir() will get me TracebackException as a name, help(traceback.TracebackException) will get me its signature. IDEs with autocomplete and signature tooltips will do the same.

There is nothing in those usage sequences to say "don't use __init__, use this redundant class method with the same signature instead".

I agree providing a "directly from an exception" constructor is essential for cases where you're working with a caught exception at the Python level. I don't agree with the idea of duplicating the required low level API under a different name so it doesn't *look* like a lower level API in the documentation.
msg235517 - (view) Author: Daniil Bondarev (xonatius) * Date: 2015-02-07 08:23
As suggested adding patch from http://bugs.python.org/issue2786 which prints qualnames in function call exceptions.

e.g:

>>> class A:
...     def __init__(self):
...         pass
>>> A(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: A.__init__() takes 1 positional argument but 2 were given
msg235569 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-08 21:59
Sorry xonatius I wasn't clear: AFAICT your patch is going to require changes to the traceback tests, and this issue is changing the implementation substantially: I was suggesting that you make sure your patch applies on top of this issue, not that you merge them :)
msg235570 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-08 22:01
Nick, Antoine - I'm now stuck between you. Options going forward:

 - I can JFDI realising you won't both be happy :)
 - you two can reach consensus!

I could cripple __init__ by switching to __new__, but I think thats massive overcomplication and not needed.

I can of course add more text to the pydoc prose saying 'do not use __init__' if that would address Nicks concern.
msg235574 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-08 22:42
Escalating to python-dev for API design feedback is the usual path forward
if we reach an impasse in the tracker comments.

I'll make one more attempt at persuading Antoine here though: the fact that
we're being tempted to add "do not use this API the way you would normally
expect to use a Python API, even though it works exactly as you might
expect" to the docs is a big red flag for me :)

There's a split between the "low level API that exposes implementation
details" (the exception state triple) and "high level API that hides them"
in the current patch, and Antoine is entirely correct that we previously
omitted the latter. It doesn't follow for me that we should also hide the
fact that the higher level convenience API is implemented in terms of the
lower level more implementation oriented one.
msg235578 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-02-09 00:11
You can also switch back to a single constructor, taking either an
exception instance or an exception triple. That way everyone is happy -
except perhaps if you are ideologically opposed to polymorphic
signatures :-)

Switching to python-dev would probably be overkill for this.
msg235658 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-10 02:08
I'm idealogically opposed to polymorphic interpretation of args :)

Antoine, will you be ok with one __init__ and one classmethod?
msg235720 - (view) Author: Mahmoud Hashemi (mahmoud) * Date: 2015-02-10 23:08
Hey all, great to see this being worked on so diligently for so long. Having worked in this area for a while (at home and at PayPal), we've got a few learnings to share:

1) linecache is textbook not-threadsafe. For example, https://hg.python.org/cpython/file/default/Lib/linecache.py#l38

For a lightweight traceback wrapper to be concurrency-friendly, we've had to catch KeyErrors, like so: https://github.com/mahmoud/boltons/blob/master/boltons/tbutils.py#L115

It's kind of a blanket approach, but maybe we could make a separate issue and help out with a linecache refresh?

2) We use something like (filename, lineno) in our DeferredLine class, but for very lightweight areas (e.g., greenlet creation) we just save a reference to the code object, as the additional attribute accesses do end up showing up in the profiles.

3) Generally we've found the APIs in TracebackInfo here to be pretty sufficient/functional: 

https://github.com/mahmoud/boltons/blob/master/boltons/tbutils.py#L134

Let me know if you've got any questions on that, and keep up the good work!
msg235872 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-02-13 03:07
@Mahmoud thanks! I had a quick look and the structural approach we've taken is a bit different. What do you think of the current patch here (issue17911-4.patch) ?
msg237166 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-03 22:24
Ok, all changes applied, lets see how this looks to folk.
msg237217 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-04 23:08
New changeset 73afda5a4e4c by Robert Collins in branch 'default':
Issue #17911: traceback module overhaul
https://hg.python.org/cpython/rev/73afda5a4e4c
msg237220 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-04 23:26
New changeset 7cea10917f40 by Robert Collins in branch 'default':
Fix brownbag in issue 17911 commit
https://hg.python.org/cpython/rev/7cea10917f40
msg237224 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-03-05 01:32
With 7cea10917f40 applied, multiple tests are now failing on most buildbots.  For example, on OS X 10.10, test_cgitb test_code_module test_decimal:

test_blanks (test.test_cgitb.TestCgitb) ... ok
test_fonts (test.test_cgitb.TestCgitb) ... ok
test_html (test.test_cgitb.TestCgitb) ... ERROR
test_syshook_no_logdir_default_format (test.test_cgitb.TestCgitb) ... ok
test_syshook_no_logdir_text_format (test.test_cgitb.TestCgitb) ... ok
test_text (test.test_cgitb.TestCgitb) ... ERROR

test_banner (test.test_code_module.TestInteractiveConsole) ... ok
test_cause_tb (test.test_code_module.TestInteractiveConsole) ... ERROR
test_console_stderr (test.test_code_module.TestInteractiveConsole) ... ok
test_context_tb (test.test_code_module.TestInteractiveConsole) ... ERROR
test_ps1 (test.test_code_module.TestInteractiveConsole) ... ok
test_ps2 (test.test_code_module.TestInteractiveConsole) ... ok
test_syntax_error (test.test_code_module.TestInteractiveConsole) ... ERROR
test_sysexcepthook (test.test_code_module.TestInteractiveConsole) ... ERROR

**********************************************************************
1 items had failures:
   1 of   4 in decimal.Context.create_decimal_from_float

See also, for example, the Debian buildbot where, in addition, test_fractions failed:

http://buildbot.python.org/all/builders/AMD64%20Debian%20PGO%203.x/builds/140/steps/test/logs/stdio
msg237225 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 01:36
Thats really strange, I did a ./python -m test run before committing - the brown bag was due to me running with the patch for 22936 also applied. Looking into the failures reported now.
msg237228 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 01:53
Ok, cgitb - its passing a string instead of an exception type into format_exception - something that was never supported - it only works by accident AFAICT, because the old format code was ignoring the etype - it was deriving the type from the value. Thats easy to accomodate in the compat code.
msg237229 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 01:54
Also apologies - ned told me on IRC that python -m test -uall is needed, not just -m test. Doh.
msg237230 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-05 01:54
There are failures on buildbot.
http://buildbot.python.org/all/builders/AMD64%20Ubuntu%20LTS%203.x/builds/5651/steps/test/logs/stdio
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5783/steps/test/logs/stdio
http://buildbot.python.org/all/builders/PPC64%20PowerLinux%203.x/builds/3109/steps/test/logs/stdio

Examples:

======================================================================
ERROR: test_cause_tb (test.test_code_module.TestInteractiveConsole)
----------------------------------------------------------------------
Traceback (most recent call last):
AttributeError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 91, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
ValueError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/python/3.x.langa-ubuntu/build/Lib/test/test_code_module.py", line 85, in test_cause_tb
    self.console.interact()
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 245, in interact
    more = self.push(line)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 267, in push
    more = self.runsource(source, self.filename)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 75, in runsource
    self.runcode(code)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 95, in runcode
    self.showtraceback()
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 144, in showtraceback
    for value, tb in traceback._iter_chain(*ei[1:]):
AttributeError: module 'traceback' has no attribute '_iter_chain'



======================================================================
ERROR: test_html (test.test_cgitb.TestCgitb)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/test/test_cgitb.py", line 23, in test_html
    raise ValueError("Hello World")
ValueError: Hello World

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/test/test_cgitb.py", line 27, in test_html
    html = cgitb.html(sys.exc_info())
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/cgitb.py", line 189, in html
    ''.join(traceback.format_exception(etype, evalue, etb)))
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/traceback.py", line 109, in format_exception
    etype, value, tb, limit=limit).format(chain=chain))
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/traceback.py", line 433, in __init__
    if exc_type and issubclass(exc_type, SyntaxError):
TypeError: issubclass() arg 1 must be a class
msg237231 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 02:01
code module is using a _function from within the traceback module to filter out a frame - we can do that with the new interface easily (it filters the first tem).
msg237232 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 02:04
The decimal failure is due to _format_final_exc_line now filtering out 'None' as well, because the string captured values of objects leads to None -> 'None' before we do rendering.

I think in this case its reasonable to change the behaviour (since None itself was already filtered out, its only the special case of a value of 'None' that worked previously).
msg237233 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-05 02:20
Fixes for buildbots.
msg237237 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2015-03-05 03:11
Much better, thanks!
msg237339 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-06 10:18
New changeset 648b35f22b91 by Berker Peksag in branch 'default':
Issue #17911: Tweak traceback documentation.
https://hg.python.org/cpython/rev/648b35f22b91
msg237842 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2015-03-11 04:11
(correct new issue #) Idle is partially disabled in a2 (#23631), with symptoms similar to that of the first example in msg237230.  Trivial test: open Idle Shell, enter '1/0'.  If this is not fixed before .a3, I think it should be reverted until it is.
msg238169 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-16 01:29
Looking at the regression now.
msg238248 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-03-16 22:17
Regression fixed AFAICT, please re-open if not.
msg269464 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-29 05:00
Robert: in issue17911-2.patch (and the eventual commit) you added a check for value == 'None' in _format_final_exc_line(). Why was this necessary? I think it is the cause of my Issue 27348 regression:

>>> traceback.format_exception(Exception, Exception("One"), None)
['Exception: One\n']
>>> traceback.format_exception(Exception, Exception("None"), None)
['Exception\n']
History
Date User Action Args
2016-06-29 05:00:26martin.pantersetnosy: + martin.panter
messages: + msg269464
2015-08-17 15:50:12haypolinkissue23003 superseder
2015-03-16 22:17:38rbcollinssetstatus: open -> closed
resolution: fixed
messages: + msg238248
2015-03-16 01:29:06rbcollinssetmessages: + msg238169
2015-03-11 04:11:37terry.reedysetmessages: + msg237842
2015-03-11 04:10:45terry.reedysetmessages: - msg237840
2015-03-11 03:57:20terry.reedysetnosy: + terry.reedy
messages: + msg237840
2015-03-06 10:18:14python-devsetmessages: + msg237339
2015-03-06 02:15:06ncoghlanlinkissue22936 dependencies
2015-03-05 03:11:31ned.deilysetpriority: critical -> normal
nosy: gvanrossum, rhettinger, ncoghlan, pitrou, haypo, rbcollins, ned.deily, python-dev, eric.snow, mahmoud, yselivanov, adaptivelogic, martius, xonatius
messages: + msg237237
2015-03-05 02:20:55rbcollinssetfiles: + issue-19711-8.patch

messages: + msg237233
2015-03-05 02:04:39rbcollinssetmessages: + msg237232
2015-03-05 02:01:11rbcollinssetmessages: + msg237231
2015-03-05 01:54:51hayposetmessages: + msg237230
2015-03-05 01:54:30rbcollinssetmessages: + msg237229
2015-03-05 01:53:04rbcollinssetmessages: + msg237228
2015-03-05 01:36:48rbcollinssetmessages: + msg237225
2015-03-05 01:32:46ned.deilysetpriority: normal -> critical
nosy: + ned.deily
messages: + msg237224

2015-03-04 23:26:26python-devsetmessages: + msg237220
2015-03-04 23:08:28python-devsetnosy: + python-dev
messages: + msg237217
2015-03-03 22:56:40rbcollinssetfiles: + issue17911-6.patch
2015-03-03 22:24:15rbcollinssetfiles: + issue17911-5.patch

messages: + msg237166
2015-02-13 03:07:49rbcollinssetmessages: + msg235872
2015-02-10 23:08:24mahmoudsetnosy: + mahmoud
messages: + msg235720
2015-02-10 02:08:01rbcollinssetmessages: + msg235658
2015-02-09 00:11:10pitrousetmessages: + msg235578
2015-02-08 22:42:43ncoghlansetmessages: + msg235574
2015-02-08 22:01:35rbcollinssetmessages: + msg235570
2015-02-08 21:59:50rbcollinssetmessages: + msg235569
2015-02-07 08:23:30xonatiussetfiles: + qualname_in_exceptions.patch
nosy: + xonatius
messages: + msg235517

2015-02-06 13:31:41ncoghlansetmessages: + msg235483
2015-02-06 12:43:39pitrousetmessages: + msg235480
2015-02-06 08:34:55ncoghlansetmessages: + msg235472
2015-02-05 22:19:29pitrousetmessages: + msg235456
2015-02-05 22:18:10ncoghlansetmessages: + msg235455
2015-02-05 19:09:23pitrousetmessages: + msg235446
2015-02-05 18:45:49rbcollinssetmessages: + msg235444
2015-02-05 10:49:27ncoghlansetmessages: + msg235428
2015-02-03 23:28:49rbcollinssetfiles: + issue17911-4.patch

messages: + msg235365
2015-01-27 13:23:04ncoghlansetmessages: + msg234820
2015-01-27 08:33:03pitrousetmessages: + msg234812
2015-01-27 01:05:46rbcollinssetmessages: + msg234804
2015-01-26 23:34:01pitrousetmessages: + msg234797
2015-01-26 23:30:57rbcollinssetmessages: + msg234796
2015-01-26 23:22:14pitrousetmessages: + msg234795
2015-01-26 22:46:09ncoghlansetmessages: + msg234788
2015-01-26 22:04:09rbcollinssetmessages: + msg234782
2015-01-26 21:48:41rbcollinssetfiles: + issue17911-3.patch

messages: + msg234781
2015-01-26 20:44:35rbcollinssetmessages: + msg234777
2015-01-26 18:59:32pitrousetmessages: + msg234769
2015-01-26 18:36:18rbcollinssetmessages: + msg234765
2015-01-26 16:30:05martiussetnosy: + martius
2015-01-26 13:38:35pitrousetmessages: + msg234741
2015-01-26 13:12:20ncoghlansetmessages: + msg234740
2015-01-26 03:18:47rbcollinssetfiles: + issue17911-2.patch

messages: + msg234712
2015-01-20 02:38:02rbcollinssetfiles: + issue17911-1.patch

messages: + msg234338
2015-01-19 17:56:27rbcollinssetmessages: + msg234319
2015-01-19 02:12:38rbcollinssetfiles: + tb_stats_1.patch

messages: + msg234299
2015-01-19 02:09:59rbcollinssetfiles: + frame_1.patch

messages: + msg234298
2015-01-19 02:09:29rbcollinssetfiles: + linecache_2.patch
2015-01-19 01:54:50rbcollinssetmessages: + msg234297
2015-01-14 01:37:32rbcollinssetfiles: + linecache_1.patch

messages: + msg233995
2015-01-12 22:53:15rbcollinssetmessages: + msg233895
2014-11-28 00:56:20rbcollinssetnosy: + rbcollins
messages: + msg231792
2014-11-27 02:47:50ncoghlansetmessages: + msg231743
2014-06-25 23:22:14hayposettitle: Extracting tracebacks does too much work -> traceback: add a new thin class storing a traceback without storing local variables
2014-06-12 21:48:46hayposetmessages: + msg220390
2014-06-12 20:11:03rhettingersetnosy: + rhettinger
messages: + msg220381
2014-06-12 12:47:21hayposetnosy: + haypo
components: + asyncio
2014-03-05 22:17:48eric.snowsetnosy: + eric.snow
2014-01-31 23:45:11yselivanovsetkeywords: - easy
nosy: + yselivanov

versions: + Python 3.5, - Python 3.4
2013-07-18 23:06:16ncoghlansetmessages: + msg193333
2013-07-18 13:45:23pitrousetmessages: + msg193296
2013-07-18 13:35:10adaptivelogicsetmessages: + msg193294
2013-07-18 09:42:42pitrousetmessages: + msg193272
2013-07-12 15:13:52adaptivelogicsetfiles: + traceback2.patch
2013-07-11 21:15:37adaptivelogicsetfiles: + traceback.patch
keywords: + patch
messages: + msg192909
2013-07-09 08:32:07pitrousetmessages: + msg192728
2013-07-08 20:07:58gvanrossumsetmessages: + msg192690
2013-07-08 17:04:34adaptivelogicsetnosy: + adaptivelogic
messages: + msg192672
2013-05-08 11:55:47pitrousetmessages: + msg188719
2013-05-07 13:13:44pitrousetmessages: + msg188648
2013-05-07 03:40:03gvanrossumsetkeywords: + easy
2013-05-07 03:39:46gvanrossumsetmessages: + msg188620
2013-05-07 01:42:05ncoghlansetmessages: + msg188616
2013-05-06 14:53:05pitrousetmessages: + msg188556
2013-05-06 14:46:52gvanrossumsetmessages: + msg188554
2013-05-06 14:39:37gvanrossumsetmessages: + msg188548
2013-05-06 13:06:02pitrousetnosy: + ncoghlan, pitrou
messages: + msg188530
2013-05-05 21:31:26gvanrossumcreate