Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix %autopx magic #349

Closed
wants to merge 4 commits into from

3 participants

@minrk
Owner

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.

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 Owner
fperez added a note

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

@fperez Owner
fperez added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Owner
fperez added a note

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 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 Owner
fperez added a note

@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 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 Owner
fperez added a note
@takluyver 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 Owner
fperez added a note

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 Owner
minrk added a note

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 Owner
fperez added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Owner
fperez added a note

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

@minrk Owner
minrk added a note

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

@fperez Owner
fperez added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez
Owner

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.

@minrk
Owner

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

IPython/core/interactiveshell.py
((12 lines not shown))
- - 0: successful execution.
- - 1: an error occurred.
+ Returns
+ -------
+ 0 : successful execution.
+ 1 : an error occurred.
@takluyver Owner

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

@minrk Owner
minrk added a note

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 Owner
fperez added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fperez
Owner

@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.

@takluyver
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.

@fperez
Owner
minrk added some commits
@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 pull request from a commit
@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 closed this in b5a53ec
@ellisonbg ellisonbg referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@mattvonrocketstein mattvonrocketstein referenced this pull request from a commit in mattvonrocketstein/ipython
@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
Commits on Apr 11, 2011
  1. @minrk

    fix %autopx in scripts by calling run_code for each ast node

    minrk authored
    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()
  2. @minrk

    add tests for parallel magics

    minrk authored
    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
  3. @minrk

    remove code_to_run attribute, updates per review.

    minrk authored
    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
  4. @minrk
This page is out of date. Refresh to see the latest.
View
53 IPython/core/interactiveshell.py
@@ -443,12 +443,6 @@ def init_instance_attrs(self):
# ipython names that may develop later.
self.meta = Struct()
- # Object variable to store code object waiting execution. This is
- # used mainly by the multithreaded shells, but it can come in handy in
- # other situations. No need to use a Queue here, since it's a single
- # item which gets cleared once run.
- self.code_to_run = None
-
# Temporary files used for various purposes. Deleted at exit.
self.tempfiles = []
@@ -2210,16 +2204,20 @@ def run_ast_nodes(self, nodelist, cell_name, interactivity='last'):
raise ValueError("Interactivity was %r" % interactivity)
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
-
- if to_run_interactive:
- mod = ast.Interactive(to_run_interactive)
- self.code_to_run = code = self.compile(mod, cell_name, "single")
- return self.run_code(code)
+
+ for i, node in enumerate(to_run_exec):
+ mod = ast.Module([node])
+ code = self.compile(mod, cell_name, "exec")
+ if self.run_code(code):
+ return True
+
+ for i, node in enumerate(to_run_interactive):
+ mod = ast.Interactive([node])
+ code = self.compile(mod, cell_name, "single")
+ if self.run_code(code):
+ return True
+
+ return False
# PENDING REMOVAL: this method is slated for deletion, once our new
@@ -2316,13 +2314,8 @@ def run_source(self, source, filename=None, symbol='single'):
return True
# Case 3
- # We store the code object so that threaded shells and
- # custom exception handlers can access all this info if needed.
- # The source corresponding to this can be obtained from the
- # buffer attribute as '\n'.join(self.buffer).
- self.code_to_run = code
# now actually execute the code object
- if self.run_code(code) == 0:
+ if not self.run_code(code):
return False
else:
return None
@@ -2336,11 +2329,17 @@ def run_code(self, code_obj):
When an exception occurs, self.showtraceback() is called to display a
traceback.
- Return value: a flag indicating whether the code to be run completed
- successfully:
+ Parameters
+ ----------
+ code_obj : code object
+ A compiled code object, to be executed
+ post_execute : bool [default: True]
+ whether to call post_execute hooks after this particular execution.
- - 0: successful execution.
- - 1: an error occurred.
+ Returns
+ -------
+ False : successful execution.
+ True : an error occurred.
"""
# Set our own excepthook in case the user code tries to call it
@@ -2373,8 +2372,6 @@ def run_code(self, code_obj):
if softspace(sys.stdout, 0):
print
- # Flush out code object which has been run (and source)
- self.code_to_run = None
return outflag
# For backwards compatibility
View
118 IPython/extensions/parallelmagic.py
@@ -15,12 +15,10 @@
#-----------------------------------------------------------------------------
import ast
-import new
import re
from IPython.core.plugin import Plugin
from IPython.utils.traitlets import Bool, Any, Instance
-from IPython.utils.autoattr import auto_attr
from IPython.testing import decorators as testdec
#-----------------------------------------------------------------------------
@@ -36,7 +34,7 @@
class ParalleMagic(Plugin):
"""A component to manage the %result, %px and %autopx magics."""
- active_view = Any()
+ active_view = Instance('IPython.parallel.client.view.DirectView')
verbose = Bool(False, config=True)
shell = Instance('IPython.core.interactiveshell.InteractiveShellABC')
@@ -105,8 +103,10 @@ def magic_px(self, ipself, parameter_s=''):
print NO_ACTIVE_VIEW
return
print "Parallel execution on engines: %s" % self.active_view.targets
- result = self.active_view.execute(parameter_s)
- return result
+ result = self.active_view.execute(parameter_s, block=False)
+ if self.active_view.block:
+ result.get()
+ self._maybe_display_output(result)
@testdec.skip_doctest
def magic_autopx(self, ipself, parameter_s=''):
@@ -125,9 +125,13 @@ def magic_autopx(self, ipself, parameter_s=''):
%autopx to enabled
In [26]: a = 10
- <Results List>
- [0] In [8]: a = 10
- [1] In [8]: a = 10
+ Parallel execution on engines: [0,1,2,3]
+ In [27]: print a
+ Parallel execution on engines: [0,1,2,3]
+ [stdout:0] 10
+ [stdout:1] 10
+ [stdout:2] 10
+ [stdout:3] 10
In [27]: %autopx
@@ -146,10 +150,12 @@ def _enable_autopx(self):
print NO_ACTIVE_VIEW
return
+ # override run_cell and run_code
self._original_run_cell = self.shell.run_cell
- self.shell.run_cell = new.instancemethod(
- self.pxrun_cell, self.shell, self.shell.__class__
- )
+ self.shell.run_cell = self.pxrun_cell
+ self._original_run_code = self.shell.run_code
+ self.shell.run_code = self.pxrun_code
+
self.autopx = True
print "%autopx enabled"
@@ -158,17 +164,42 @@ def _disable_autopx(self):
"""
if self.autopx:
self.shell.run_cell = self._original_run_cell
+ self.shell.run_code = self._original_run_code
self.autopx = False
print "%autopx disabled"
- def pxrun_cell(self, ipself, cell, store_history=True):
+ def _maybe_display_output(self, result):
+ """Maybe display the output of a parallel result.
+
+ If self.active_view.block is True, wait for the result
+ and display the result. Otherwise, this is a noop.
+ """
+ targets = self.active_view.targets
+ if isinstance(targets, int):
+ targets = [targets]
+ if targets == 'all':
+ targets = self.active_view.client.ids
+ stdout = [s.rstrip() for s in result.stdout]
+ if any(stdout):
+ for i,eid in enumerate(targets):
+ print '[stdout:%i]'%eid, stdout[i]
+
+
+ def pxrun_cell(self, raw_cell, store_history=True):
"""drop-in replacement for InteractiveShell.run_cell.
This executes code remotely, instead of in the local namespace.
+
+ See InteractiveShell.run_cell for details.
"""
- raw_cell = cell
+
+ if (not raw_cell) or raw_cell.isspace():
+ return
+
+ ipself = self.shell
+
with ipself.builtin_trap:
- cell = ipself.prefilter_manager.prefilter_lines(cell)
+ cell = ipself.prefilter_manager.prefilter_lines(raw_cell)
# Store raw and processed history
if store_history:
@@ -187,6 +218,7 @@ def pxrun_cell(self, ipself, cell, store_history=True):
ipself.execution_count += 1
return None
except NameError:
+ # ignore name errors, because we don't know the remote keys
pass
if store_history:
@@ -195,7 +227,6 @@ def pxrun_cell(self, ipself, cell, store_history=True):
ipself.history_manager.store_output(ipself.execution_count)
# Each cell is a *single* input, regardless of how many lines it has
ipself.execution_count += 1
- print cell
if re.search(r'get_ipython\(\)\.magic\(u?"%?autopx', cell):
self._disable_autopx()
@@ -205,24 +236,49 @@ def pxrun_cell(self, ipself, cell, store_history=True):
result = self.active_view.execute(cell, block=False)
except:
ipself.showtraceback()
+ return True
+ else:
+ if self.active_view.block:
+ try:
+ result.get()
+ except:
+ self.shell.showtraceback()
+ return True
+ else:
+ self._maybe_display_output(result)
return False
-
- if self.active_view.block:
- try:
- result.get()
- except:
- ipself.showtraceback()
- else:
- targets = self.active_view.targets
- if isinstance(targets, int):
- targets = [targets]
- if targets == 'all':
- targets = self.active_view.client.ids
- stdout = [s.rstrip() for s in result.stdout]
- if any(stdout):
- for i,eid in enumerate(targets):
- print '[stdout:%i]'%eid, stdout[i]
+
+ def pxrun_code(self, code_obj):
+ """drop-in replacement for InteractiveShell.run_code.
+
+ This executes code remotely, instead of in the local namespace.
+
+ See InteractiveShell.run_code for details.
+ """
+ ipself = self.shell
+ # check code object for the autopx magic
+ if 'get_ipython' in code_obj.co_names and 'magic' in code_obj.co_names and \
+ any( [ isinstance(c, basestring) and 'autopx' in c for c in code_obj.co_consts ]):
+ self._disable_autopx()
return False
+ else:
+ try:
+ result = self.active_view.execute(code_obj, block=False)
+ except:
+ ipself.showtraceback()
+ return True
+ else:
+ if self.active_view.block:
+ try:
+ result.get()
+ except:
+ self.shell.showtraceback()
+ return True
+ else:
+ self._maybe_display_output(result)
+ return False
+
+
_loaded = False
View
10 IPython/parallel/client/view.py
@@ -765,11 +765,11 @@ def activate(self):
print "The IPython parallel magics (%result, %px, %autopx) only work within IPython."
else:
pmagic = ip.plugin_manager.get_plugin('parallelmagic')
- if pmagic is not None:
- pmagic.active_view = self
- else:
- print "You must first load the parallelmagic extension " \
- "by doing '%load_ext parallelmagic'"
+ if pmagic is None:
+ ip.magic_load_ext('parallelmagic')
+ pmagic = ip.plugin_manager.get_plugin('parallelmagic')
+
+ pmagic.active_view = self
@testdec.skip_doctest
View
111 IPython/parallel/tests/test_view.py
@@ -10,8 +10,10 @@
# Imports
#-------------------------------------------------------------------------------
+import sys
import time
from tempfile import mktemp
+from StringIO import StringIO
import zmq
@@ -300,3 +302,112 @@ def findall(pat, s):
self.assertEquals(view.apply_sync(findall, '\w+', 'hello world'), 'hello world'.split())
+ # parallel magic tests
+
+ def test_magic_px_blocking(self):
+ ip = get_ipython()
+ v = self.client[-1]
+ v.activate()
+ v.block=True
+
+ ip.magic_px('a=5')
+ self.assertEquals(v['a'], 5)
+ ip.magic_px('a=10')
+ self.assertEquals(v['a'], 10)
+ sio = StringIO()
+ savestdout = sys.stdout
+ sys.stdout = sio
+ ip.magic_px('print a')
+ sys.stdout = savestdout
+ sio.read()
+ self.assertTrue('[stdout:%i]'%v.targets in sio.buf)
+ self.assertRaisesRemote(ZeroDivisionError, ip.magic_px, '1/0')
+
+ def test_magic_px_nonblocking(self):
+ ip = get_ipython()
+ v = self.client[-1]
+ v.activate()
+ v.block=False
+
+ ip.magic_px('a=5')
+ self.assertEquals(v['a'], 5)
+ ip.magic_px('a=10')
+ self.assertEquals(v['a'], 10)
+ sio = StringIO()
+ savestdout = sys.stdout
+ sys.stdout = sio
+ ip.magic_px('print a')
+ sys.stdout = savestdout
+ sio.read()
+ self.assertFalse('[stdout:%i]'%v.targets in sio.buf)
+ ip.magic_px('1/0')
+ ar = v.get_result(-1)
+ self.assertRaisesRemote(ZeroDivisionError, ar.get)
+
+ def test_magic_autopx_blocking(self):
+ ip = get_ipython()
+ v = self.client[-1]
+ v.activate()
+ v.block=True
+
+ sio = StringIO()
+ savestdout = sys.stdout
+ sys.stdout = sio
+ ip.magic_autopx()
+ ip.run_cell('\n'.join(('a=5','b=10','c=0')))
+ ip.run_cell('print b')
+ ip.run_cell("b/c")
+ ip.run_code(compile('b*=2', '', 'single'))
+ ip.magic_autopx()
+ sys.stdout = savestdout
+ sio.read()
+ output = sio.buf.strip()
+ self.assertTrue(output.startswith('%autopx enabled'))
+ self.assertTrue(output.endswith('%autopx disabled'))
+ self.assertTrue('RemoteError: ZeroDivisionError' in output)
+ ar = v.get_result(-2)
+ self.assertEquals(v['a'], 5)
+ self.assertEquals(v['b'], 20)
+ self.assertRaisesRemote(ZeroDivisionError, ar.get)
+
+ def test_magic_autopx_nonblocking(self):
+ ip = get_ipython()
+ v = self.client[-1]
+ v.activate()
+ v.block=False
+
+ sio = StringIO()
+ savestdout = sys.stdout
+ sys.stdout = sio
+ ip.magic_autopx()
+ ip.run_cell('\n'.join(('a=5','b=10','c=0')))
+ ip.run_cell('print b')
+ ip.run_cell("b/c")
+ ip.run_code(compile('b*=2', '', 'single'))
+ ip.magic_autopx()
+ sys.stdout = savestdout
+ sio.read()
+ output = sio.buf.strip()
+ self.assertTrue(output.startswith('%autopx enabled'))
+ self.assertTrue(output.endswith('%autopx disabled'))
+ self.assertFalse('ZeroDivisionError' in output)
+ ar = v.get_result(-2)
+ self.assertEquals(v['a'], 5)
+ self.assertEquals(v['b'], 20)
+ self.assertRaisesRemote(ZeroDivisionError, ar.get)
+
+ def test_magic_result(self):
+ ip = get_ipython()
+ v = self.client[-1]
+ v.activate()
+ v['a'] = 111
+ ra = v['a']
+
+ ar = ip.magic_result()
+ self.assertEquals(ar.msg_ids, [v.history[-1]])
+ self.assertEquals(ar.get(), 111)
+ ar = ip.magic_result('-2')
+ self.assertEquals(ar.msg_ids, [v.history[-2]])
+
+
+
Something went wrong with that request. Please try again.