msg188467 - (view) |
Author: Guido van Rossum (gvanrossum) * |
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) * |
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) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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 (vstinner) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
Date: 2014-11-28 00:56 |
I'll put something together.
|
msg233895 - (view) |
Author: Robert Collins (rbcollins) * |
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) * |
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) * |
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) * |
Date: 2015-01-19 02:09 |
This is a WIP patch, including it to just share progress.
|
msg234299 - (view) |
Author: Robert Collins (rbcollins) * |
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) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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) * |
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) * |
Date: 2015-01-27 01:05 |
Why do you consider it crippling?
|
msg234812 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2015-01-27 08:33 |
It's tedious and complicated compared to the natural way.
|
msg234820 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
Date: 2015-02-05 22:19 |
The solution of having two constructor classmethods actually seems nice
and symmetric to me.
|
msg235472 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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: Alyssa Coghlan (ncoghlan) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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) * |
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) * |
Date: 2015-03-05 02:20 |
Fixes for buildbots.
|
msg237237 - (view) |
Author: Ned Deily (ned.deily) * |
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) * |
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) * |
Date: 2015-03-16 01:29 |
Looking at the regression now.
|
msg238248 - (view) |
Author: Robert Collins (rbcollins) * |
Date: 2015-03-16 22:17 |
Regression fixed AFAICT, please re-open if not.
|
msg269464 - (view) |
Author: Martin Panter (martin.panter) * |
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']
|