Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration of stack_data #23

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

alexmojaki
Copy link
Contributor

As discussed in #22

This is still very much a WIP, but you can run the demos and see output that looks right until you inspect it more closely. I'm putting some work out early so you can see what's coming and we can start some conversations. To try it out, clone https://github.com/alexmojaki/stack_data and pip install -e <path to folder> in your interpreter where you work on stackprinter.

A couple of perks you can already observe:

  • The function name is replaced by its __qualname__ thanks to executing.
  • Source lines in classes show correctly. For example, try this script in master:
try:
    class A:
        1 / 0
except:
    stackprinter.show()

It shows something like this:

File "path.py", line 16, in A

ZeroDivisionError: division by zero

The source line 1 / 0 and its context are absent. This is now fixed.

Things I plan on doing soon which should be quite quick and easy:

  1. Connect stackprinter configuration to stack_data configuration.
  2. Select variables to show
  3. Ordering variables
  4. Hide signature for non-function scopes.

Changes we need to discuss:

  1. For displaying expressions other than variables, e.g. attributes, I want to make another package called pure_eval which accepts an AST node and returns a bunch of sub-expressions which can be safely evaluated without triggering any side effects and their corresponding values. This will include for example attributes which are simply present in __dict__ (no properties) or subscripts a[b] where a has a known type like a list or dict. I think it's dangerous to evaluate arbitrary attributes like you're doing now because this risks mutating the state of the program which could interfere with someone's debugging. better-exceptions has a similar view - they are currently trying to integrate the display of attributes but only using getattr_static. With this in mind there would no longer be such a thing as UnresolvedAttribute.
  2. The context lines included are slightly different. Files are divided into 'pieces' which are ranges of lines 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. Context is measured in numbers of pieces, e.g. the default is to include 3 pieces before and 1 piece after. This may be more than 5 lines total but the advantage is that tracebacks don't truncate statements or other groups of lines that should logically be together. However if you have a very long piece then it may be truncated in the middle to avoid arbitrarily long tracebacks.
  3. I'd like to add an indication of which node in a frame was executing where possible using the executing library. The simplest way is to add a line such as While calling: foo() to each frame. This is probably the only decent option when there's no color. With color, there's the possibility of highlighting the expression similar to what heartrate does. However given that the expression may contain multiple variables with random colors this will probably be hard to do in a way that's reliably readable.
  4. Normal bits of source code are not currently highlighted in the 'default' color of a color scheme. They're just not highlighted at all. Should I find a way to put that back?
  5. Line continuations using backslashes are not collapsed. You seemed unsure, and I don't think it was a good idea. Let people read their code the way they wrote it and are familiar with. I don't know how I would implement it in the current framework anyway.

@cknd
Copy link
Owner

cknd commented Aug 31, 2019

Hey!

I need to make time to take a proper look, but to quickly reply to your points at the end:

  1. Side effects in attribute lookups -- I completely agree, evaluating arbitrary attributes should at most be an opt-in config 💣

  2. great! yeah, the line threshold is basically a crude approximation to what one really wants ("show just enough code so it makes sense"). This will be much nicer, working from semantically meaningful pieces

  3. Ah I see, this would be more fine-grained than just noting the executed line number -- e.g. if my line is foo(bar(zap(zop()))), we can disambiguate that we were currently executing zap, etc. (Though in this example, you'd also see that by looking at the next deeper traceback entry.. do you have a better example?) I do like the highlighting idea!
    OK so if I'm not completely mistaken this would be mostly a visual convenience feature (= you can still infer this information from the rest of the traceback, if the highlights aren't available) - in that case perhaps it'd be ok if only 'fancy mode' could indicate the active node, using color. Either way, there's still lots of time to play around with this. (One thing I wouldn't like very much though would be to sprinkle extra ascii art in people's code)

  4. I kept the non-variable code monochrome deliberately (though keywords and operators are bold and slightly brighter), so that more of the color space is available to assign to variables -- I think combining syntactic and semantic colorization could easily become chaotic
    (edit: rereading what you wrote I think I misunderstood - you meant that the color schemes have no control over the 'background' source color? But isn't the source_default color applied to them? (grep for source_default and default_tpl))

  5. Oh yes, to be clear: Collapsing multiline statements wasn't an aesthetic choice, they just caused problems in certain corner cases due to my overburdened, token-based line parsing machine. In other words, this was totally just a hack. It will be a happy day when the token machine disappears due to your work & backslashed statements will be undisturbed forever after!

@alexmojaki
Copy link
Contributor Author

Awesome, I'm glad you're so happy with my proposals.

  1. ... OK so if I'm not completely mistaken this would be mostly a visual convenience feature (= you can still infer this information from the rest of the traceback, if the highlights aren't available)

That is often (probably mostly) the case, although there are plenty of exceptions:

  1. When there is no deeper frame.
  2. When the same function is called multiple times in a line.
  3. When the variable/attribute being called has a different name from the function, e.g. callback(x) where callback = foo. In this case you could still probably figure it out by looking at the variables, but it's getting harder.
  4. When you call a magic method via an operation, e.g. foo[bar] triggers __getitem__. For beginners this is crucial as they may not know what __getitem__ means, but apart from that there could easily be several potential __getitem__ calls in the line. In a way this is a mix of points 2 and 3.

An example of all of these is if you get an IndexError while digging into a nested list with x[i][j][k].

We can check for 'normal' cases defined as follows:

  1. There is a deeper frame.
  2. The executing node is a vanilla ast.Call node, i.e. no magic methods.
  3. The thing being called is just a variable or attribute.
  4. The name of the variable/attribute is the same as the code name of the deeper frame.
  5. The name only appears once in the statement being executed.

In those cases we can hide the extra info by default. But I'm not sure if this would be a good thing.

  1. ... you meant that the color schemes have no control over the 'background' source color?

Yes, but I wasn't clear, I was talking about within this PR. Since I'm not iterating through every token, there is no longer a place to apply default_tpl. So those bits of code are not highlighted and they use the terminal's default color, which is typically the opposite of the background and may be configured by the user independently. If that's a problem I think I can fix it by inserting e.g. </variable><default> where I currently just insert </variable>.

@alexmojaki
Copy link
Contributor Author

alexmojaki commented Sep 1, 2019

  1. How would you like to name and document the parameters currently called source_lines and source_lines_after? I'm thinking we just leave them as they are and add a note in the docs such as "Multiline statements are treated as a single line". It's not a complete explanation but I think it's close enough and users don't need to know the exact details.
  2. In particular, are you happy for source_lines=1 to actually potentially show multiple lines, especially in the summary traceback?
  3. source_lines_after is currently not passed as an argument anywhere. Is that intentional?
  4. As I said, long pieces (e.g. statements with several lines) may be truncated in the middle. For example, this:
        ranges = [
            Range(
                node.first_token.start[1],
                node.last_token.end[1],
                (variable, node),
            )
            for variable, node in self.frame_info.variables_by_lineno[self.lineno]
        ]

would become something like:

        ranges = [
            Range(
(...)
            )
            for variable, node in self.frame_info.variables_by_lineno[self.lineno]
        ]

Currently the default is to display at most 6 lines (including the (...) if needed) per piece, except for the currently executing piece which is never truncated. Would you like to let users configure this? If so give me a name and some documentation that you think fits your library and intended audience.
5. Currently I'm skipping blank lines of code between pieces to reduce the length of the traceback. I think it's OK for the code in a traceback to not be hyper-readable and the tracebacks are already very long. What do you think? It's easy to turn that off.

@alexmojaki alexmojaki changed the title Initial crude integration of stack_data Integration of stack_data Sep 1, 2019
@alexmojaki
Copy link
Contributor Author

I just pushed an update to stack_data which relies on an update to executing, so make sure you upgrade that.

@alexmojaki
Copy link
Contributor Author

OK, I've created https://github.com/alexmojaki/pure_eval, so pip install -e that as well and of course update stack_data. So this PR now has safe attribute and subscript inspection.

There's not that much left for me to implement. I'm very keen to hear your responses to the questions above, particularly the first 3 here.

@luzpaz
Copy link

luzpaz commented Nov 5, 2019

Any progress on this ?

@alexmojaki
Copy link
Contributor Author

Hey @luzpaz, the current blocker is that I've asked @cknd a bunch of questions to which I'm awaiting a response.

I'm curious, what's your interest in this PR?

@luzpaz
Copy link

luzpaz commented Nov 5, 2019

@alexmojaki i'm looking forward to the perks this PR introduces per #23 (comment)

@cknd
Copy link
Owner

cknd commented Nov 5, 2019

Hey all! I haven't forgotten about this, I just found it surprisingly difficult to find a free weekend lately. I'll need your patience a bit longer

@alexmojaki
Copy link
Contributor Author

Hi @cknd, that's fine, just remember that for now you only need to look at the conversation, not the code changes. I still have plenty more to do.

@cknd
Copy link
Owner

cknd commented Dec 25, 2019

Hey, apologies for the delays. To pick up this thread again:

  1. On naming the source_lines and source_lines_after parameters: I'd just remove the source_lines_after parameter and let source_lines control the maximum nr of lines printed for one frame (perhaps renaming it to max_lines).
  2. I think there should remain a way to enforce a limit on the nr of lines shown per frame. Where space constraints force us to choose lines, my priorities have roughly been as follows (disregarding implementation feasibility): 1) the line that contains the executed node 2) prior lines that could have affected the variables occurring in the executed one, 3) other lines nearby for visual recognition of the code, focusing mostly on prior lines. So far, I approximated desire 2) by including the function header and some fixed N lines above the executed one, I have a hunch your inspection tools could do this more intelligently.
  3. It's half intentional at best, in the sense that the parameter didn't turn out to be very important and could just as well be a constant in the code I.e. in usage, I rarely noticed the need to to see more lines after the last executed one
  4. I haven't yet played around with it enough to have a good feel for how the new logical-block-based code selection method behaves. It's probably enough for now if it's internally structured in a way where exposing the config later stays easy.
  5. Skipping blank lines: I have a fairly visually oriented memory so I always prioritized preserving the original layout of the code -- I'd keep at least the single blank lines untouched. Perhaps multiple blank lines could be folded down to one to save space.

@alexmojaki
Copy link
Contributor Author

If we still specify a number of lines, then I don't understand what's going to change and how the division into pieces (logical blocks) will be used. Can you elaborate, maybe with examples?

@alexmojaki
Copy link
Contributor Author

Anyway, in the meantime stack_data and pure_eval have been pretty fully developed. They have complete tests and documentation and are available on PyPI. Hopefully soon they will be integrated into IPython. I suggest you read through the stack_data README.

By the way, the new dependencies do not and will not support Python 3.4, which is well past its EOL, so this PR is dropping it.

@cknd
Copy link
Owner

cknd commented Dec 31, 2019

If we still specify a number of lines, then I don't understand what's going to change and how the division into pieces (logical blocks) will be used. Can you elaborate, maybe with examples?

Hm, it seems I'm still lacking intuitions how the new stack_data block snipper behaves on real code. I need to play with it some more. Roughly, my thought is: Since the blocks can have varying sizes (1-6 lines with the current default afaik), then for a given maximum nr of blocks to be printed, the resulting printout can vary in length a lot, depending on the nesting structure of the code and the size of the nested blocks. But since the main motive for printing less than the whole scope is to save screen space, any (future) config to limit printing should have a mostly predictable effect on the space that will be used.

I'll play with the new snipping mechanism and see how I can map it to this underlying motive, i.e.: Given a certain quantifiable desire to save screen space, how can we select the parts of code most likely to help with debugging. I have a hunch that logical blocks are part of the answer.

(random idea that disregards implementation effort: One way to reconcile block selection and a configurable output size might be to treat it as a bin packing problem, packing a number of logical blocks that fits the configured size of printout, prioritized by expected usefulness (starting with the executed line, then outer blocks that explain the control flow to the executed line, and previous occurrences of the variables in the lines printed so far, etc.) /random idea that disregards implementation effort)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants