AST splitter #332

Merged
merged 9 commits into from Apr 7, 2011

Projects

None yet

3 participants

@takluyver
IPython member

I'm making a pull request as much so we can see the diff as for instant inclusion - I've modified quite a bit of the core stuff, so I imagine it might need some checking and polishing.

So, in short, I've replaced our old technique of splitting input into blocks with a version that works with AST (abstract syntax tree) nodes. These are representations of pieces of Python code, and a node can be anything from an integer literal to an if-elif-else to a class definition. For our purposes, we work only with the top level of nodes we get when we parse some code; we don't consider the many child nodes of these.

Specifically, run_cell will now:

  • With single line input, run it all in interactive mode; so 2*4; 3*3 produces two sets of output.
  • With multiline input, run only the last node in interactive mode. So if 2*4; 3*3 is the last line of a multiline cell, only the second result (9) is displayed.

Along the way, I had to modify the CachingCompiler a bit, as it assumed that it would cache and compile source in one operation. It now has a cache method, which returns the cell name, (e.g. ""), which is passed back to the compilation step. The compilation step now has a call signature matching the built in compile function.

@takluyver
IPython member

I should add that (at this point), the test suite passes, and IPython.core.inputsplitter still has 100% test coverage (although I've removed split_blocks and the tests that cover it).

@ellisonbg
IPython member
@takluyver
IPython member

It shouldn't change, as far as I'm aware. We call prefilter_lines on the cell before trying to run it, which should turn everything into valid Python syntax, and knows about the difference between multi-line and single line cells. The only difference is that I think prefilter_lines will do the dynamic transformations on the first line of a multi-line block. That's easily changed if we want.

@ellisonbg
IPython member
@takluyver
IPython member

Don't worry about asking questions; apart from anything else, trying to explain what's going on often helps me understand it.

The frontends call two methods of the inputsplitter: push is designed to operate line by line, and returns whether or not the block of input is complete (as determined by codeop.CommandCompiler from the stdlib). push_accepts_more relies on the state from previous calls to push to determine whether to carry on entering code or to execute it now. I haven't changed this, except that push_accepts_more used to call split_blocks.

Static parts of the IPython syntax, which can be processed without the user namespace (like explicit %magic calls) are transformed by the push method into valid Python syntax for the frontend. Dynamic parts, like automagics (run foo) and macros, if they are not coincicdentally Python syntax as well, appear to the frontend as a SyntaxError, in which case it sends it to the kernel (which tries the dynamic transformations before rejecting it). So yes, there is a degree of duplication between inputsplitter and prefilter, which I've not tried to address.

Our split_blocks implementation already depended on getting valid Python code: if it wasn't, an except clause simply returned the code as a single block. While I've removed split_blocks, I've left an except block in push_accepts_more so that that method behaves the same way with invalid Python syntax (that is, it will indicate that it should be sent for attempted execution).

@ellisonbg
IPython member
@takluyver
IPython member

Thanks, Brian. Let's tag Fernando (@fperez) so he knows you've suggested that he look at it (although I'm sure he will anyway before long).

@ellisonbg
IPython member
@fperez
IPython member

Hey Thomas, sorry for the silence, a bit swamped today with local life stuff. I'll try to catch up as soon as I can, many thanks for this work!

@takluyver
IPython member

Not to worry, Fernando, there's no rush.

@fperez
IPython member

I'll try to work on this later today, just wanted to note that we need to remember to close #306 when this goes in.

@takluyver
IPython member

Will do. And remove the 'known failure' decorator on its test. I branched just before the test for 306 got merged, but I could rebase, then tweak its test and mark the commit as closing the issue.

@fperez
IPython member

I'll start reviewing as-is, but feel free to do the rebase/fix so that gets closed automatically. We'll make sure to wait for your rebase before actually merging.

@takluyver
IPython member

Rebased, removed the known_failure decorator , and run the test suite run to check that it passes (which it does). The last commit should auto-close #306 as and when it's merged.

@fperez fperez and 1 other commented on an outdated diff Apr 7, 2011
IPython/core/interactiveshell.py
self.history_manager.store_output(self.execution_count)
# Each cell is a *single* input, regardless of how many lines it has
self.execution_count += 1
-
+
+ def run_ast_nodes(self, nodelist, cell_name, interactivity=1):
+ """Run a sequence of AST nodes. The execution mode depends on the
+ interactivity parameter.
+
+ Parameters
+ ----------
+ nodelist : list
+ A sequence of AST nodes to run.
+ interactivity : int
+ At 0, all nodes are run in 'exec' mode. At '1', the last node alone
@fperez
fperez Apr 7, 2011

What do you think of making this interactivity parameter instead take False/'last'/True? I just don't like magic integer flags very much, they are not very self-explanatory. I'm thinking 0->False, 1->'last', 2->True. what do you think?

@takluyver
takluyver Apr 7, 2011

Can do if you want. Although an argument that can be a string or a boolean doesn't feel that neat either. What about string options, as used by compile? "all", "last", "none"?

@fperez
IPython member

Other than a very small comment I made inline, this is great work! I ran the test suite, and have been playing with it for a while, and it looks very good to me. Note: you should add your name to the authors list in those files, you've made so many changes. It's partly giving you proper credit (though that's mostly ensured by the git log), partly keeping a quick reference of the main hands that have gone into a file, in case somebody has a question later.

I can't see any reason to hold this further: all the semantics we needed are preserved, the tests pass, and it's a major simplification of that ugly double-pass hack.

Stellar job!

@takluyver takluyver merged commit beafaea into ipython:master Apr 7, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment