Patch review: [ 1098732 ] Enhance tracebacks and stack traces with vars
I'd like to help develop Python for fun and profit and I've heard that posting patch reviews to python-dev is a good way to contribute. So here goes: PATCH REVIEW: [ 1098732 ] Skip Montanaro has written a patch which makes it so that you can inspect variable values in tracebacks. IMHO, it is a brilliant idea and can make debugging quite alot easier. However, I'm not so fond of the way that he has implemented it, it needs work. He basically outputs all names in all stackframes all the way up to the top which makes the traceback look way to cluttered. He has also implemented it as a hook to sys.excepthook, I would ike it to be the default way in which tracebacks are printed, or atleast ctivated by a command line switch to Python. What does everyone else think? Does Skip's idea have any merit? http://sourceforge.net/tracker/index.php?func=detail&aid=1098732&group_id=5470&atid=305470 -- mvh Björn
At 08:20 PM 2/9/05 +0100, BJörn Lindqvist wrote:
Does Skip's idea have any merit?
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__). Also note that the stdlib already has a cgitb module that does this sort of display for CGI scripts, so the technique isn't new, and cgitb provides a good example for people to create their own advanced traceback formatters with. If there were another command line option added to Python for this, I'd personally prefer it be an option to enter the debugger when a terminal traceback is printed. Currently, I use 'python -i' so that I get an interpreter prompt, then use 'import pdb; pdb.pm()' to enter the debugger at the point where the error occurred. One can then print whatever local variables are desired, go up and down the stack, list code, and even perform calculations on the values on the stack. About the only place I can think of where such an extremely verbose traceback would be useful and safe, is inside of unit tests. I believe that the py.test package uses traceback introspection of this kind in order to display relevant values when an assertion fails. So, it might be useful in the context of a unit test error report to get some of that information, but even there, there is a question of how much is relevant for display.
Phillip J. Eby wrote:
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__).
Notice that this patch does not change the exception printing behaviour of Python at all. It just changes the implementation of traceback.print_exception, so it only affects code that actually uses this function. Furthermore, it only affects code that uses this function and is *changed* to supply the argument True for print_args.
Also note that the stdlib already has a cgitb module that does this sort of display for CGI scripts, so the technique isn't new, and cgitb provides a good example for people to create their own advanced traceback formatters with.
Sure. However, if this is frequently needed (outside the context of CGI), it would sure be helpful if the traceback module supported it.
If there were another command line option added to Python for this, I'd personally prefer it be an option to enter the debugger when a terminal traceback is printed. Currently, I use 'python -i' so that I get an interpreter prompt, then use 'import pdb; pdb.pm()' to enter the debugger at the point where the error occurred.
With the patch, you would have to add an explicit try/except into your code, to supply True for print_args (or set a sys.excepthook, as Skip suggests in his patch readme). Regards, Martin
At 12:21 AM 2/10/05 +0100, Martin v. Löwis wrote:
Phillip J. Eby wrote:
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__).
Notice that this patch does not change the exception printing behaviour of Python at all. It just changes the implementation of traceback.print_exception, so it only affects code that actually uses this function. Furthermore, it only affects code that uses this function and is *changed* to supply the argument True for print_args.
I was just responding to the OP, who was advocating it for Python default behavior, or behavior controlled by the command line. That's why I said, "Yes, but not as a default behavior."
Phillip J. Eby wrote:
I was just responding to the OP, who was advocating it for Python default behavior, or behavior controlled by the command line. That's why I said, "Yes, but not as a default behavior."
I wasn't sure how to interpret the message - I could not find out whether you have looked at the patch, and agreed with it, or whether you merely read the OP's summary of the patch. Regards, Martin
Phillip> I was just responding to the OP, who was advocating it for Phillip> Python default behavior, or behavior controlled by the command Phillip> line. That's why I said, "Yes, but not as a default behavior." My original intent was that it would probably not fly as default behavior. I'm not sure I would always want that behavior either. I would like it for long-running daemons that crash while unattended (places where "python -i" wouldn't really help). Skip
"Phillip J. Eby" <pje@telecommunity.com> writes:
At 08:20 PM 2/9/05 +0100, BJörn Lindqvist wrote:
Does Skip's idea have any merit?
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__).
Oh, come on. Making tracebacks less useful to protect people who accidentally spray them across the internet seems absurd. Would you like them not to show source, either? Cheers, mwh -- Many of the posts you see on Usenet are actually from moths. You can tell which posters they are by their attraction to the flames. -- Internet Oracularity #1279-06
On Feb 9, 2005, at 6:25 PM, Michael Hudson wrote:
"Phillip J. Eby" <pje@telecommunity.com> writes:
At 08:20 PM 2/9/05 +0100, BJörn Lindqvist wrote:
Does Skip's idea have any merit?
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__).
Oh, come on. Making tracebacks less useful to protect people who accidentally spray them across the internet seems absurd. Would you like them not to show source, either?
On Mac OS X the paths to the files are so long as to make the tracebacks really ugly and *less* usable. I certainly wouldn't mind if __name__ showed up instead of __file__. I have a "pywhich" script that shows me the file given a name that I use: (note that modulegraph.util.imp_find_module is like imp.find_module but it will walk the packages to find the actual module and it only returns the filename) #!/usr/bin/env python import sys, os from modulegraph.util import imp_find_module for module in sys.argv[1:]: path,oext = os.path.splitext(imp_find_module(module)[1]) for ext in ('.py', oext): if os.path.exists(path+ext): print path+ext break -bob
Oh, come on. Making tracebacks less useful to protect people who accidentally spray them across the internet seems absurd. Would you like them not to show source, either?
My response exactly.
On Mac OS X the paths to the files are so long as to make the tracebacks really ugly and *less* usable. I certainly wouldn't mind if __name__ showed up instead of __file__. I have a "pywhich" script that shows me the file given a name that I use:
Well, sorry, but not everybody is as smart as you, and having the file name rather than the module name there helps debugging important sys.python issues. It wouldn't be the first time that someone has a hacked version of a standard module tucked away in a directory that happens to land on the path, and seeing the pathname is then a lot more productive than the module name. -- --Guido van Rossum (home page: http://www.python.org/~guido/)
At 11:25 PM 2/9/05 +0000, Michael Hudson wrote:
"Phillip J. Eby" <pje@telecommunity.com> writes:
At 08:20 PM 2/9/05 +0100, BJörn Lindqvist wrote:
Does Skip's idea have any merit?
Yes, but not as a default behavior. Many people already consider the fact that tracebacks display file paths to be a potential security problem. If anything, the default traceback display should have less information, not more. (E.g., display module __name__ instead of the code's __file__).
Oh, come on. Making tracebacks less useful to protect people who accidentally spray them across the internet seems absurd. Would you like them not to show source, either?
I said that many people considered that to be the case, not that I did. ;) I'd personally prefer to read module names than filenames, so I guess I should've mentioned that. :) Of course, Guido has previously answered the filename vs. modulename question (years ago in fact), so it was moot even before I mentioned it. For some reason it slipped my mind at the time, though.
BJörn Lindqvist wrote:
I'd like to help develop Python for fun and profit and I've heard that posting patch reviews to python-dev is a good way to contribute. So here goes:
Are we actually promoting this? I am fine with people doing this when they have done five reviews and want their specific patch looked at (personally I prefer when people do it in a single email, but I can live with individual ones). But if people don't have that in mind, should we not be encouraging this? I mean it seems to be defeating the purpose of SF and having the various mailing lists that send out updates on SF posts. -Brett
Brett C. wrote:
But if people don't have that in mind, should we not be encouraging this? I mean it seems to be defeating the purpose of SF and having the various mailing lists that send out updates on SF posts.
Clearly, the comment should *also* go to SF - posting it to python-dev may mean it gets lost eventually (in particular, when somebody gets to look at the patch). Björn did post his comment to SF, and a summary to python-dev. I personally think this is a good strategy: it puts focus on things that should be worked on. Let me explain why I think that these patches should be worked on: - it might be that the analysis of the patch suggests that the patch should be rejected, as-is. If so, it has a good chance to be closed *right away* with somebody with write privileges to the tracker, if he agrees with the analysis taken. People who care can follow the link in the email message, and see that the patch was closed. People who don't care can quickly grasp this is a patch review, and delete the message. - it might be that the analysis suggests changes. Posting it to python-dev gives the submitter of the patch a chance to challenge the review. If somebody thinks the requested changes are unecessary, they will comment. People actually prefer to discuss questionable requests for changes on the mailing list, instead of discussing them in the SF tracker. - it might be that the analysis recommend acceptance. Again, it might be that this can trigger a quick action by some committer - anybody else can safely ignore the message. However, *some* committer should take *some* action on the patch - one day or the other. Having the right to commit is a privilege, but it is also an obligation. The patch needs to be eventually looked at, and decided upon. Somebody already did the majority of the work, and suggested an action. It should be easy to decide whether this action is agreeable or not (unless the review is flawed, in which case the reviewer should be told about this). To put it the other way 'round: should we only discuss changes on python-dev which *don't* have patches on SF???? I don't think so. Furthermore, this strategy exposes the reviewer. A reviewer is somebody who will potentially get write access to the tracker, and perhaps CVS write access. A reviewer who wants to contribute in this way regularly clearly needs to gain the trust of other contributors, and posting smart, valuable, objective, balanced reviews on contributed patches is an excellent way to gain such trust (likewise, posting reviews which turn out to be flawed is a way to find out that the reviewer still needs to learn things before he can be trusted). Regards, Martin P.S. These remarks are mostly of general nature - I haven't actually studied yet Björn's review (but I leave it in my inbox so I can get back to it next week).
Martin v. Löwis wrote:
Brett C. wrote:
But if people don't have that in mind, should we not be encouraging
this? I mean it seems to be defeating the purpose of SF and having the various mailing lists that send out updates on SF posts.
[SNIP]
Björn did post his comment to SF, and a summary to python-dev. I personally think this is a good strategy: it puts focus on things that should be worked on.
Let me explain why I think that these patches should be worked on: - it might be that the analysis of the patch suggests that the patch should be rejected, as-is. [SNIP] - it might be that the analysis suggests changes. [SNIP]
- it might be that the analysis recommend acceptance. [SNIP]
All valid points, but I also don't want people to suddenly start posting one-liners or bug posts. I guess it comes down to a signal-to-noise ratio and if the level of signal we are currently getting will hold. If we say it is okay for people to send in patch reviews *only* and not notifications of new patches, bug reports, or bug reviews, then I can handle it.
To put it the other way 'round: should we only discuss changes on python-dev which *don't* have patches on SF???? I don't think so.
And neither do I. I just don't want a ton of random emails on python-dev that really belong in the SF tracker instead. Reason why we don't tend to take direct bug reports in email unless there is a question over semantics.
Furthermore, this strategy exposes the reviewer. A reviewer is somebody who will potentially get write access to the tracker, and perhaps CVS write access. A reviewer who wants to contribute in this way regularly clearly needs to gain the trust of other contributors, and posting smart, valuable, objective, balanced reviews on contributed patches is an excellent way to gain such trust (likewise, posting reviews which turn out to be flawed is a way to find out that the reviewer still needs to learn things before he can be trusted).
That is a very good point. Guess I am softening on my rejection to this. =) If people in general agree to this idea of having people post patch reviews to python-dev I will update the dev intro essay to reflect all of this. I will also add a mention about the 5-1 patch review deal. [SNIP]
P.S. These remarks are mostly of general nature - I haven't actually studied yet Björn's review (but I leave it in my inbox so I can get back to it next week).
Same here. I didn't mean to single out Björn in any way. He just happened to trigger an email out of me. =) -Brett
Brett C. wrote:
All valid points, but I also don't want people to suddenly start posting one-liners or bug posts.
I agree that keeping the noise level low is desirable; I hope this will come out naturally when we start commenting on high-noise remarks. For example, I would have no problems telling somebody who says "me too" on a feature request that he should go away and come back with an implementation of the requested feature. I would still apply the "standard" conventions of python-dev: that you should be fairly knowledgable about the things you are talking about before posting.
I guess it comes down to a signal-to-noise ratio and if the level of signal we are currently getting will hold. If we say it is okay for people to send in patch reviews *only* and not notifications of new patches, bug reports, or bug reviews, then I can handle it.
People do tend to notify about patches from time to time, especially when they are committers, and want to weigh in their reputation to advance peer review of the proposed changes. Other people who notify about new patches they made will continue to get my "5 for 1" offer which actually triggered this new interest in contributing-by-reviewing. Another reason not to post patches to python-dev is message size for modem users although I'm doubtful how valid this rationale is these days, given ADSL, spam, HTML mails, and everything...
And neither do I. I just don't want a ton of random emails on python-dev that really belong in the SF tracker instead. Reason why we don't tend to take direct bug reports in email unless there is a question over semantics.
I certainly don't want to see random comments on python-dev, either (and I do see random comments come in bursts, and have to choose to ignore entire threads because of that. I don't have to write python-dev summaries, though :-) I disagree with the primary reason not to take bug reports on python-dev, however: bug reports in email get lost if not immediately processed; usage of a tracker is necessary to actually "keep track". So this kind of bug management is the primary reason for the tracker, not that we want to keep random users out of python-dev (although this is a convenient side effect). Regards, Martin
[Martin von Löwis]
I disagree with the primary reason not to take bug reports on python-dev, however: bug reports in email get lost if not immediately processed; usage of a tracker is necessary to actually "keep track".
Some developers and users appreciate bug trackers, or at least are able to stand them. Others, at least like me, just hate them. When a developer replies to one of my emails, asking me that I use the bug tracker, my email was surely not lost, since the developer is replying to it. That developer could have used the bug tracker himself, the way he sees fit, instead of inviting me to do it. In fact, a developer asking me to use the tracker of the day is trying to educate me into using it. Or maybe he knows that using the tracker is uneasy and is trying to spare himself some disgust. Or maybe he is consciously trying to turn me down :-). I do not buy the argument of the fear of emails being lost. Actually, almost all of my emails reporting bugs received a reply in one form or another, so developers do see them. If a developer wants to use a bug tracker, then nice, good for him. For one, trackers merely tell me that I should get a life and do nicer things than reporting bugs. In any case, Python has plenty of users, and others will contribute anyway. So, after all, why should I?
So this kind of bug management is the primary reason for the tracker, not that we want to keep random users out of python-dev (although this is a convenient side effect).
Hey, that's good! Trackers may act like a randomiser! :-) -- François Pinard http://pinard.progiciels-bpi.ca
On Wed, 09 Feb 2005 17:25:14 -0800, Brett C. <bac@ocf.berkeley.edu> wrote:
All valid points, but I also don't want people to suddenly start posting one-liners or bug posts.
I guess it comes down to a signal-to-noise ratio and if the level of signal we are currently getting will hold. If we say it is okay for people to send in patch reviews *only* and not notifications of new patches, bug reports, or bug reviews, then I can handle it.
Having done some reviews (admittedly for the 5-for-1 deal) I do like seeing patch reviews appear on python-dev. As they are meant to be reviews, this implies a certain level of effort expended, and quality in the response. I agree with Martin that detail comments should go in the tracker - a posting can summarise to an extent, but should be enough to let python-dev readers know if they can act on the review. It's nice to see new contributors doing good work to help Python, and I assume they like the chance to feel like they are "participating" by posting helpful contributions to python-dev. IMHO, the tracker doesn't give this same feeling of "contributing". Also, review postings encourage others to do the same - I know I did my reviews after having seen someone else post a set of reviews. It made me think "hey, I could do that!" I'm sure there are other lurkers on python-dev who could be encouraged to assist in the same way. Having said this, I'd suggest that if people intend to review multiple patches, they post a summary covering a number of patches at a time. Paul.
participants (10)
-
"Martin v. Löwis" -
BJörn Lindqvist -
Bob Ippolito -
Brett C. -
François Pinard -
Guido van Rossum -
Michael Hudson -
Paul Moore -
Phillip J. Eby -
Skip Montanaro