Fix %autopx magic #349

Closed
wants to merge 4 commits into
from

Projects

None yet

3 participants

Owner
minrk commented Apr 10, 2011

run_ast_nodes previously called one big run_code for the whole cell. This broke %autopx, which hijacks execution.

Previously, there was no call to override if %autopx was called inside a single cell (e.g. %run script.ipy). This commit splits run_ast_nodes, so that each node gets a separate call to run_code, and overriding run_code successfully hijacks execution.

@fperez fperez commented on an outdated diff Apr 10, 2011
IPython/extensions/parallelmagic.py
@@ -150,6 +150,10 @@ class ParalleMagic(Plugin):
self.shell.run_cell = new.instancemethod(
self.pxrun_cell, self.shell, self.shell.__class__
)
+ self._original_run_code = self.shell.run_code
+ self.shell.run_code = new.instancemethod(
fperez
fperez Apr 10, 2011 Owner

The 'new' module is deprecated and not available in python3, so we shouldn't use new.

@fperez fperez and 2 others commented on an outdated diff Apr 10, 2011
IPython/core/interactiveshell.py
@@ -2183,15 +2183,20 @@ class InteractiveShell(Configurable, Magic):
exec_count = self.execution_count
if to_run_exec:
- mod = ast.Module(to_run_exec)
- self.code_to_run = code = self.compile(mod, cell_name, "exec")
- if self.run_code(code) == 1:
- return
-
+ for i, node in enumerate(to_run_exec):
+ mod = ast.Module([node])
+ self.code_to_run = code = self.compile(mod, cell_name+str(i), "exec")
fperez
fperez Apr 10, 2011 Owner

why do we still need this code_to_run attribute? I think I added that ages ago for the threaded shells, but I think it shouldn't be necessary in the current design.

takluyver
takluyver Apr 10, 2011 Owner

I don't think we should add the node number here (str(i)). We've put the cell in linecache as one block, and if there's an exception, it will look it up at that name. The ast nodes know their lineno within the block, and the traceback uses that to point to the correct line of the block.

fperez
fperez Apr 10, 2011 Owner

@takluyver, did you actually notice a problem? I tested things with causing exceptions interactively, because I was worried about the exact same issue, but didn't see an issue...

takluyver
takluyver Apr 10, 2011 Owner

It doesn't seem to interfere, but I also don't see the extra number appearing in the tracebacks or debugger. I suspect the way Python works at present, the filename is fixed in when we create the AST, but I think it's best practice to keep using the same name we cached it under.

fperez
fperez Apr 10, 2011 Owner

OK, that does make sense, thanks for giving this a once over.

Otherwise, this seems ok for merge to me, agreed?

takluyver
takluyver Apr 10, 2011 Owner

Apart from the other point I just noticed, it should be fine. I've not checked the parallel stuff in any detail, just the bits in this file.

fperez
fperez Apr 11, 2011 Owner

Sorry, before this is merged, I really would like to clarify if we still need that code_to_run stuff. As best I can tell, that's old, dead code. And in these logic-critical functions, dead stuff is dangerous because it confuses us...

minrk
minrk Apr 11, 2011 Owner

ack reveals that code_to_run is used nowhere else, so I'll take it out.

(The name is used in ipapp.py, but it's not the same variable)

fperez
fperez Apr 12, 2011 Owner

On Mon, Apr 11, 2011 at 10:02 AM, minrk
reply@reply.github.com
wrote:

ack reveals that code_to_run is used nowhere else, so I'll take it out.

(The name is used in ipapp.py, but it's not the same variable)

excellent, thanks. It's indeed old code from the threaded shell hacks.

@fperez fperez and 1 other commented on an outdated diff Apr 10, 2011
IPython/extensions/parallelmagic.py
@@ -224,6 +231,48 @@ class ParalleMagic(Plugin):
print '[stdout:%i]'%eid, stdout[i]
return False
+ def pxrun_code(self, ipself, code_obj, post_execute=True):
fperez
fperez Apr 10, 2011 Owner

proper docstring? I mean, explaining what the arguments are?

minrk
minrk Apr 10, 2011 Owner

Sure thing - I should note that this is actually InteractiveShell.run_code's docstring, so maybe that should be updated as well.

fperez
fperez Apr 10, 2011 Owner

On Sat, Apr 9, 2011 at 9:31 PM, minrk
reply@reply.github.com
wrote:

Sure thing - I should note that this is actually InteractiveShell.run_code's docstring, so maybe that should be updated as well

That would be great, thanks.

Owner
fperez commented Apr 10, 2011

Other than the above small comments, the rest of the PR looks good. I have no objection to the approach of making the ast run more granular, esp. since it enables the parallel magic implementation to be fairly simple.

Min, one last thing: could you add some tests for the parallel magics? It would be good to catch these regressions on the test suite in the future.

Owner
minrk commented Apr 10, 2011

Okay, I made the changes you mentioned, and I'll get started working on tests.

@takluyver takluyver and 2 others commented on an outdated diff Apr 10, 2011
IPython/core/interactiveshell.py
- - 0: successful execution.
- - 1: an error occurred.
+ Returns
+ -------
+ 0 : successful execution.
+ 1 : an error occurred.
takluyver
takluyver Apr 10, 2011 Owner

Would it be neater to use True and False (which == 1 and 0 anyway)?

minrk
minrk Apr 10, 2011 Owner

Yes, that makes sense, I was just following the existing pattern.

In fact, in the code it appears that True/False and 1/0 are used interchangeably

fperez
fperez Apr 10, 2011 Owner

Some of that goes back to python 2.1, and was also dictated by the interfaces in code.py, where ipython started.

In the past we were still subclassing that code, so we had to keep our apis matching that. But now we don't subclass from code.py, so feel free to modernize these things as necessary.

@takluyver takluyver commented on an outdated diff Apr 10, 2011
IPython/core/interactiveshell.py
@@ -2183,15 +2183,20 @@ class InteractiveShell(Configurable, Magic):
exec_count = self.execution_count
if to_run_exec:
- mod = ast.Module(to_run_exec)
- self.code_to_run = code = self.compile(mod, cell_name, "exec")
- if self.run_code(code) == 1:
- return
-
+ for i, node in enumerate(to_run_exec):
+ mod = ast.Module([node])
+ self.code_to_run = code = self.compile(mod, cell_name+str(i), "exec")
+ if self.run_code(code) == 1:
+ return 1
+
if to_run_interactive:
takluyver
takluyver Apr 10, 2011 Owner

Just noticed, if we're looping over nodes, we can lose the if checks - they were just to stop it trying to do things with an empty list.

Owner
fperez commented Apr 11, 2011

@takluyver, do you see any other issues with this PR? Assuming Min fixes the unnecessary if checks you pointed to and the other little things here, is this one good to go? Since it touches your recent ast code, I'd like to hear from you before merging.

Owner

No, other than those things, I think it's good. The good news is that this it's hard to go much wrong here without noticing, because it's being used all the time.

Owner
fperez commented Apr 11, 2011

Cool, thanks. Min, good to go then when you have a chance for the
remaining fixes...

minrk added some commits Apr 9, 2011
@minrk minrk fix %autopx in scripts by calling run_code for each ast node
Previously, there was no call to override if %autopx was called inside
a single cell (e.g. %run script.ipy). This splits run_ast_nodes, so that
each node gets a separate call to run_code.

reviewed changes:
* remove old use of 'new'
* fix interactiveshell.run_code docstring

Additional change:
automatically load parallelmagic extension on view.activate()
31c15b8
@minrk minrk add tests for parallel magics
tests revealed some changes that should be made:

%px also prints remote stdout in blocking mode,
and actually raises the RemoteError (not just display)
if there is one
8cf5bb6
@minrk minrk remove code_to_run attribute, updates per review.
review changes:

* change 1/0 to True/False in run_code
* remove interactiveshell.code_to_run
* remove superfluous if-tests checking for empty lists in run_ast_nodes

closes gh-349
b5a53ec
@minrk minrk update parallel %autopx magics to reflect recent changes in run_cell,…
… run_code
e68cf3a
@minrk minrk closed this in b5a53ec Apr 11, 2011
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk remove code_to_run attribute, updates per review.
review changes:

* change 1/0 to True/False in run_code
* remove interactiveshell.code_to_run
* remove superfluous if-tests checking for empty lists in run_ast_nodes

closes gh-349
316aefc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment