Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.5k
Integration of stack_data#11886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Hey@Carreau, I know this is a long post and you're probably busy but I thought you'd be excited about this. Are you happy with the changes I'm introducing so far? Should I implement the suggestions I made in points 3 on both examples? |
Hey, apologies for the late reply; Yes I'm excited about having something better ; I just barely had time to do open Source since August, and I am just starting to catch up on a few thousand notifications ! But I will read that in more details ! |
pure_eval is interesting, I'm wondering if it could be of use (or folded in) jedi/parso. You may want to setpython_requires in setup.py, and if those packages are Pure Python I would attempt to package withflit.
That makes a lot of sens and similar in how pdb is better in Python 3.
Up to you , ultratb code is quite old, from the time Python did not have boolean, so a complete rewrite is welcome. We can also change the layout and how things are printed. I'm not too worried about backward incompatibility. I think all of this is great; and if we have temporary feature regression but a sane framework to wonr on it would be nice. I'm tempted to think that@gforsyth and@scopatz might be interested in this from the xonsh side ? I guess the next step is try to push/document/publish pure_eval and stack_data I'm happy to have an opt-in flag in IPython to use those if installed and see how it feels as a user for a couple of releases; and then switch to default once deemed good enough. Thanks again for starting these. |
Some extra thoughts; I'd love at some point to have an html repr for tracebacks; which would mean potentially rendering things to html with the value visible on hover, or within I'd would also love to offload the coloring to Pygments. Not sure how worth it is; but that would allow to delegate theming to pygments. I'm also thinking about whether |
Yep! This would be cool to have in xonsh! |
Awesome! It's great that you're so happy with my proposals, I will continue working on this and I will trust myself more to make these kinds of judgement calls. There is still a lot more to be done in the two new repos and this PR, so it's going to be a while. Regarding coloring: eventually I want to do#11827. That requires inserting markers (e.g. ANSI codes) whose positions are known in the source code. Doing that is significantly harder if the code has been syntax highlighted. If there's a way for these two systems to not get in each other's way that would be great. I don't know if that's possible at all, butmaybe it would be harder with Pygments. Otherwise I might just parse the highlighted text to recalculate the position. |
I actually think it would be easier with Pygments. The way pygments works (handwavy explanation) is you yield tuples of So if you wan to change the colors you basically do: defretokenize(tokens_stream):fortype,tokenintoken_stream:ifin_range(token):yieldnew_type,tokenelseyieldtype,token There might be some tricks to do here and there; but I'm guessing we should be able to interleave. What we are doing with pygments if possible. In general I thing we should avoid as much as possible to insert ansi code until as far as possible to avoid any headache. The other thing IIRC is that prompt_toolkit should be able to render directly a pygments token stream if my memory is correct, and that a token stream can be rendered either to ANSI or HTML directly. Anyway just thoughts. |
6cfe61c tob975973CompareHere's an update:
|
OK, I've discovered iptest which I didn't know about before. I've fixed most of the tests now. One failure which is not making sense to me is |
Success! The only thing that's failing on travis is something about brew and python 3.6 on OSX, which I can't fix myself. |
Merry Christmas! (almost) 🎄 The libraries are documented and ready. This PR is ready aside from the issue of making it optional, which I've tried to address in#12054 but it's tricky. While we think about that, this can still be reviewed. I'm looking into your suggestion regarding pygments. The problem is that The other problem is that pygments seems to only handle one token type at a time. I can't seem to mark a token as its usual type so that it gets syntax highlighted as well as my special type so that it also gets highlighted (e.g. via the background) as part of the executing node. That means giving up syntax highlighting inside the node. Maybe that's OK, but it makes me sad. |
Sorry about that; I've started to move most of the test to by pytest compatible but didn't had the chance to fix everything; in particular we still have a few things only with doctests. OS X usually I can test / fix if necessary. I've been travelling and just arrived in europe and fighting jetlag. I'll trust you if you think pygments does not work. It was mostly a thought, but you've spent more time than me. I'll try to have a look at#12054 my main concern next will be to get pure_eval, stack_data and executing in conda-forge as most of our installs are from there. I'm thinking of releasing 7.11 on Fri, and then make a 7.x branch then have some fun on master for 2020 and celebrate Python 2 EOL, and start work toward 8.0 |
I actually seem to have the pygments thing sorted out 😄 You mentioned packaging the libraries with flit...is there any particular reason for that? I don't understand the appeal of these tools (same with poetry) over a regular |
conda forge packages are on their way! Do these packages need to support pypy? Does IPython currently support pypy? |
The 3 packages are now successfully in conda-forge. |
Making stack_data optional is now in the newalexmojaki#2 which will preserve the git history. |
BTW travis is only failing for unrelated reasons: |
Hey@Carreau, I see that in the fog of all my comments I've obscured the fact that this PR is ready for review. I bring it up because I noticed that some commits appeared in ultratb.py in master so I had to resolve conflicts. |
Thanks, yes it is on my TODO list, and I've seen some of your emails on Python Ideas about traceback. I'm trying to find a way to free up some of my time and dive into all of that. On the good side I'm done with some immigration paperwork, on the bad side my works still has not hired someone to replace the third member of my team, so we're currently still pulling 150% load... but getting there. |
No problem, I understand that this is a big task and you have your own life to live :) I just wanted to make sure we were on the same page. |
(I've at least restarted the CI). |
| returntest_function(*args,**kwargs) | ||
| finally: | ||
| sys.setrecursionlimit(rl) | ||
| ultratb._FRAME_RECURSION_LIMIT=_orig_rec_limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I believe those were here to speedup the test suite by a lot (and/or failure on CI). Is that handled by stack_data ?
I'm happy if it needs to be removed for other reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The speedup is provided bysys.setrecursionlimit.ultratb._FRAME_RECURSION_LIMIT was used for detecting recursion, although I don't know why it didn't usesys.getrecursionlimit.
stack_data doesn't detect recursion specifically, it detects any string of repeating frames, similar to the Python traceback.
CI seem to fail for unrelated but weird reasons. |
Ok, you know what, let's merge that in master, and create a 7.x branch in case we need to backport fixes. |
👏 🎊 Go nuts now, and feel free to change the format of stack traces. |
Awesome! This is very exciting. So are you fine with just making this the default and requiring the new dependencies? |
Yes, I'll keep maintaining a 7.x separate branch with critical bugfix; but that mean that we have a couple of month where we can play around and worse case revert/redo if something is wrong. |
Uh oh!
There was an error while loading.Please reload this page.
As requested in#11827 and also discussed inhttps://github.com/Qix-/better-exceptions/issues/10 andhttps://github.com/Qix-/better-exceptions/issues/92, I've started work on a libraryhttps://github.com/alexmojaki/stack_data which extracts data from tracebacks for other libraries to format as they please. In this PR I'd like to integrate it and figure out details about the desired behaviour and API of
stack_datawhich I can change along the way. For now I don't plan on addressing#11827 directly, that can be in a future PR.In the process I've also createdhttps://github.com/alexmojaki/pure_eval which finds expressions that can be safely evaluated without side effects.
stack_datauses it directly so those values are shown in tracebacks.Neither of these libraries are officially released yet, so if you want to try out this fork you need to clone them and
pip install -eeach one. Alsopip install -U executingin case you've ever installed it before.This PR introduces several changes. I think some are unambiguously good, while others need to be considered and discussed. I'll demonstrate them with a couple of examples.
Consider this script:
Here's the traceback from master:
And here's the new traceback:
Probably the most critical change and feature of
stack_datais that context is not simply measured in individual lines, but 'pieces', which are ranges of lines which logically belong together, representing either a single simple statement or a part of a compound statement (loops, if, try/except, etc) that doesn't contain any other statements. Most pieces are a single line, but a multi-line statement or if condition is a single piece. In this example, lines 6, 7, and 15 are each a single whole piece, while the lines 8-12 form one piece because they contain one multiline statement. So in the second frame, the new traceback includes the main piece [8-12] and two pieces of earlier context [6] and [7]. By contrast the same frame in the first traceback only shows two lines of earlier context which is not enough to show the start of the current statement and the assignment ofdct.Blank lines are excluded in most situations. The idea is to make tracebacks shorter while only slightly sacrificing code readability which is not critical when viewing a traceback. In the first traceback there are three blank lines which aren't adding anything.
A consequence of the above points is that the first frame in the new traceback awkwardly includes lines from inside the definition of
foowhich are not really relevant in that frame at the<module>level. I'm thinking I should generally collapse function definitions contained inside the current scope to look like this:and consider that one piece of context. What do you think?
There are several differences in the lists of values:
pure_evaldoesn't list functions, modules and classes with a__name__equal to the name of the variable or attribute, because it's generally redundant. For example, it doesn't listfoo = <function foo at 0x1053666a8>as the first traceback does.pure_evalis able to evaluate more complex expressions such aslst[i] = 0when it's safe to do so. For now it can't do very much yet but it has plenty of room to grow.ratioas the name of a variable when it's really the name of a keyword argument and no such variable ever comes close to existing, henceglobal ratio = undefined.pure_evalandstack_datawork with the AST so this can't happen.Another example:
Output from master:
New output:
Important differences:
__qualname__of the function being executed, e.g.Foo.bar(...)instead of justbar(...). This is provided byexecuting.self.xandother.x. Evaluating these prints outhitwo extra times.pure_evaldoesn't evaluate dynamic attributes such as properties to avoid unwanted side effects.other.ybut not the underlying values such asother. Maybe this is intentional, but I think it's potentially missing valuable information. They are now included. However because IPython also shows the values of argumentsselfis redundant in this case. Shall I exclude arguments from the list of values?fooin the first traceback shows the linedef barwhich is completely out of scope, as well as some blank lines.global Foo.foo = <function Foo.foo at 0x1128161e0>even though that expression doesn't appear in that line, apparently as a weird side effect of how the token based implementation works. This isn't just weird, it has the potential to be misleading. Consider this script:Which includes a line in the traceback:
which is technically correct but may be confusing when the reader needs to know that
Foo().bar == 3.I think that's more than enough to digest and discuss for now. I'll just quickly mention two things that you shouldn't expect to work yet and we'll discuss them later: