Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add support for relative import to %run -m (fixes #2727) #3066

Merged
merged 19 commits into from

5 participants

@tkf

%run is refactored a little bit by side effect :)

@tkf

This is the test failure:
https://travis-ci.org/ipython/ipython/jobs/5746287#L2399

It is from ChangedPyFileTest.test_changing_py_file. In this test, import foo is executed. In my test import <temporary-package>.foo is executed. I guess this failure is due to Python 2.x mixing absolute and relative imports sometime. In Python 3.x, there is no failure.

IPython/core/magics/execution.py
@@ -187,7 +187,9 @@ def prun(self, parameter_s='', cell=None, user_mode=True,
namespace = self.shell.user_ns
if cell is not None:
arg_str += '\n' + cell
- else: # called to run a program by %run -p
+ elif namespace is not None: # called to run a program by %run -p
+ arg_str = cell
+ else:
@takluyver Owner

Can this line have a comment to explain when it happens? The comments on the if/elif may also need to be updated.

@takluyver Owner

On second thoughts, maybe we should refactor this. Currently prun is used both as a magic command, and as a function to be called by other code, which makes it somewhat confusing. Maybe the core should be pulled out into another function, so that prun is just for the magic command. Going by your other changes, the new function could be called _run_with_profiler.

@tkf
tkf added a note

I thought it would be better to not break backward compatibility. As it is a user facing function (i.e., magic function), someone might be using it for writing their own magic function. But I would refactor and add _run_with_profiler if you think it is OK. I think we can get rid of the else clause as I think it is not used in IPython.

FYI, you can see the parameter spec in the documentation:
http://ipython.org/ipython-doc/dev/api/generated/IPython.core.magics.execution.html#IPython.core.magics.execution.ExecutionMagics.prun

@takluyver Owner

Backwards compatibility would already have been broken when we refactored the magics into separate classes for 0.13, and I don't remember hearing about any problems, so I don't think it's an issue. We can add a note to the "what's new" document in case anyone is using it.

The API docs unfortunately default to documenting every class and function, without us ever having to consider what constitutes our public API. I've added an @undoc decorator to explicitly mark things as private, but I'm reluctant to apply it to magic classes, because at present the API docs are the only fully comprehensive documentation of the magic commands we offer.

@tkf
tkf added a note

If I can change it that's good. I just played on safer side. The function is clear now, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/core/tests/test_run.py
((24 lines not shown))
+ sys.path.insert(0, self.tempdir.name)
+
+ self.writefile(os.path.join(package, '__init__.py'), '')
+ self.writefile(os.path.join(package, 'sub.py'), """
+ x = {0!r}
+ """.format(self.value))
+ self.writefile(os.path.join(package, 'relative.py'), """
+ from .sub import x
+ """)
+ self.writefile(os.path.join(package, 'absolute.py'), """
+ from {0}.sub import x
+ """.format(package))
+
+ def tearDown(self):
+ os.chdir(self.__orig_cwd)
+ sys.path[:] = filter(lambda x: x != self.tempdir.name, sys.path)
@takluyver Owner

Not sure about this - filter returns an iterator in Python 3. If it needs a lambda, the list comprehension is cleaner anyway:

[p for p in path if p != self.tempdir.name]

@tkf
tkf added a note

I completely agree that the list comrehension is beter than filter+lamda. I usually do that and I wonder why I didn't.

The code worked probably because list.__setitem__ turns an iterator to a list.

@takluyver Owner

Ah, I wondered if there was some magic in setting a slice that would achieve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver takluyver commented on the diff
IPython/core/tests/test_run.py
@@ -337,3 +343,74 @@ def test_run_formatting(self):
self.mktmp(src)
_ip.magic('run -t -N 1 %s' % self.fname)
_ip.magic('run -t -N 10 %s' % self.fname)
+
+
+class TestMagicRunWithPackage(unittest.TestCase):
+
+ def writefile(self, name, content):
+ path = os.path.join(self.tempdir.name, name)
+ d = os.path.dirname(path)
+ if not os.path.isdir(d):
+ os.makedirs(d)
+ with open(path, 'w') as f:
+ f.write(textwrap.dedent(content))
+
+ def setUp(self):
+ self.package = package = 'tmp{0}'.format(repr(random.random())[2:])
@takluyver Owner

I don't think we really need to randomly generate the package name, when we're already in a single-use temporary directory. We can just keep things simple and call it something like testpkg.

@tkf
tkf added a note

That won't work if you use testpkg somewhere else. As IPython has a huge code base, it is hard to avoid used names for a temporary module. See the fix I did for 2975de6. But if you still want to use testpkg, I'd change that.

@takluyver Owner

Oh, sorry, I'd forgotten that it needs to be unique for sys.modules as well as the file path. We could remove it from sys.modules in setUp/tearDown, but that's no simpler than just giving it a unique name, so never mind.

@tkf
tkf added a note

I tried this but "unloading" modules this way did not work so I will stick to the current version.

diff --git a/IPython/core/tests/test_run.py b/IPython/core/tests/test_run.py
index dac65ed..71402a2 100644
--- a/IPython/core/tests/test_run.py
+++ b/IPython/core/tests/test_run.py
@@ -356,9 +356,7 @@ def writefile(self, name, content):
             f.write(textwrap.dedent(content))

     def setUp(self):
-        self.package = package = 'tmp{0}'.format(repr(random.random())[2:])
-        """Temporary valid python package name."""
-
+        self.package = package = 'testpkg'
         self.value = int(random.random() * 10000)

         self.tempdir = TemporaryDirectory()
@@ -381,6 +379,11 @@ def tearDown(self):
         sys.path[:] = [p for p in sys.path if p != self.tempdir.name]
         self.tempdir.cleanup()

+        for name in [(n.startswith(self.package) or
+                      n.startswith(self.package + '.'))
+                     for n in sys.modules]:
+            del sys.modules[name]
+
     def check_run_submodule(self, submodule, opts=''):
         _ip.magic('run {2} -m {0}.{1}'.format(self.package, submodule, opts))
         self.assertEqual(_ip.user_ns['x'], self.value,
@takluyver Owner

Sticking with the current version sounds fine.

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

Definitely +1 on breaking run up into some smaller pieces like this.

IPython/core/magics/execution.py
((47 lines not shown))
- opts.merge(opts_def)
+ opts.merge(Struct(D=[''], l=[], s=['time'], T=['']))
@takluyver Owner

Can this have a comment explaining that it's filling in the default options for any that aren't specified?

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

Thanks, I think this is looking good.

@tkf tkf commented on the diff
IPython/core/magics/execution.py
((12 lines not shown))
+ bdb.Breakpoint.bpbynumber = [None]
+ # Set an initial breakpoint to stop execution
+ maxtries = 10
+ bp_file, bp_line = parse_breakpoint(break_point, filename)
+ checkline = deb.checkline(bp_file, bp_line)
+ if not checkline:
+ for bp in range(bp_line + 1, bp_line + maxtries + 1):
+ if deb.checkline(bp_file, bp):
+ break
+ else:
+ msg = ("\nI failed to find a valid line to set "
+ "a breakpoint\n"
+ "after trying up to line: %s.\n"
+ "Please set a valid breakpoint manually "
+ "with the -b option." % bp)
+ raise UsageError(msg)
@tkf
tkf added a note

@takluyver This was error(msg) and return. But as _run_with_debugge is a separated function now, return does not finish run. Do you think this is OK? I am not 100% sure that I can replace error(msg) and return with raise UsageError(msg).

@takluyver Owner

I think magics are supposed to raise a UsageError now, instead of error()ing and returning. It does mean that this code should only be called by a magic function, but since it's a private method in a Magics class, I think that's reasonable.

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

I haven't dug into the details here, but I certainly agree that %run is long overdue for a re-factoring!

@takluyver
Owner

This is OK by me now, but as it touches some core and rather fragile stuff, I'd like to have a second set of eyes look over it before we merge it. I think @fperez said he'd have more time soon, so maybe he'll have a look.

@bfroehle

We should also update the main %run docstring to add the -m option:

%run [-n -i -t [-N<N>] -d [-b<N>] -p [profile options]] ( -m mod | file ) [args]
@tkf

I fixed the usage line. -e was also not mentioned.

@ellisonbg
Owner

Looks like this PR has gotten some good review and updates. There was a mention of having @fperez looking at it because he knows this code well. Do you still feel like that is desired? Other than that, is this ready to go?

@takluyver
Owner

I'd ideally like a second pair of eyes on it, because the %run stuff is quite delicate. I think @fperez knows it best. But we have got reasonable test coverage for it now, so if Fernando's not going to get to it soon, we could push the green button and deal with any fallout that might occur.

IPython/core/magics/execution.py
@@ -676,7 +628,79 @@ def run(self, parameter_s='', runner=None,
del sys.modules[main_mod_name]
return stats
-
+
+ def _run_with_debugger(self, code, code_ns, break_point, filename):
+ deb = debugger.Pdb(self.shell.colors)
@fperez Owner
fperez added a note

Even though it's private, I'd want to see a docstring here... Functions without docstrings are just painful for whomever comes later, as it's very hard to make out the intent of the arguments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/core/magics/execution.py
((37 lines not shown))
+ print "%s prompt to start your script." % deb.prompt
+ try:
+ #save filename so it can be used by methods on the deb object
+ deb._exec_filename = filename
+ deb.run(code, code_ns)
+
+ except:
+ etype, value, tb = sys.exc_info()
+ # Skip three frames in the traceback: the %run one,
+ # one inside bdb.py, and the command-line typed by the
+ # user (run by exec in pdb itself).
+ self.shell.InteractiveTB(etype, value, tb, tb_offset=3)
+
+ @staticmethod
+ def _run_with_timing(run, nruns):
+ twall0 = time.time()
@fperez Owner
fperez added a note

Same here, let's add a docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/core/tests/test_run.py
@@ -16,6 +16,10 @@
import os
import sys
import tempfile
+import unittest
+import textwrap
+import random
+import functools
@fperez Owner
fperez added a note

Let's keep the imports alphabetically ordered, it's easier to find later on what's there.

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

Great job, @tkf! Very minor cosmetic issues there, otherwise it looks solid. Honestly, %run should be refactored even deeper, so that the actual code in magics is absolutely minimal and more of it is a more stateless interface (would be easier to test). But such is life, at least this disentangles the beast somewhat and is a good first step.

Once the small issues are ironed out, I think we can go ahead and merge. Thanks everyone for the earlier review, and sorry for my slowness!

@tkf
tkf commented

@fperez I just fixed the points you mentioned.

@bfroehle bfroehle merged commit 9087bb9 into ipython:master
@tkf tkf deleted the tkf:run-module branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 23, 2013
  1. @tkf

    Test for #2727 (%run -m doesn't support relative imports)

    tkf authored
    test_run_submodule_with_relative_import fails.
  2. @tkf

    Refactor part for "%run -t"

    tkf authored
  3. @tkf
  4. @tkf
  5. @tkf
  6. @tkf
  7. @tkf
  8. @tkf

    Refactor part for "%run -d"

    tkf authored
  9. @tkf
Commits on Mar 24, 2013
  1. @tkf
  2. @tkf
  3. @tkf
  4. @tkf
  5. @tkf
  6. @tkf
  7. @tkf
Commits on Mar 26, 2013
  1. @tkf
Commits on May 3, 2013
  1. @tkf
  2. @tkf

    Sort imports in test_run.py

    tkf authored
This page is out of date. Refresh to see the latest.
Showing with 244 additions and 108 deletions.
  1. +167 −108 IPython/core/magics/execution.py
  2. +77 −0 IPython/core/tests/test_run.py
View
275 IPython/core/magics/execution.py
@@ -77,8 +77,7 @@ def profile_missing_notice(self, *args, **kwargs):
@skip_doctest
@line_cell_magic
- def prun(self, parameter_s='', cell=None, user_mode=True,
- opts=None,arg_lst=None,prog_ns=None):
+ def prun(self, parameter_s='', cell=None):
"""Run a statement through the python code profiler.
@@ -178,38 +177,33 @@ def prun(self, parameter_s='', cell=None, user_mode=True,
In [1]: import profile; profile.help()
"""
+ opts, arg_str = self.parse_options(parameter_s, 'D:l:rs:T:q',
+ list_all=True, posix=False)
+ if cell is not None:
+ arg_str += '\n' + cell
+ return self._run_with_profiler(arg_str, opts, self.shell.user_ns)
- opts_def = Struct(D=[''],l=[],s=['time'],T=[''])
+ def _run_with_profiler(self, code, opts, namespace):
+ """
+ Run `code` with profiler. Used by ``%prun`` and ``%run -p``.
- if user_mode: # regular user call
- opts,arg_str = self.parse_options(parameter_s,'D:l:rs:T:q',
- list_all=True, posix=False)
- namespace = self.shell.user_ns
- if cell is not None:
- arg_str += '\n' + cell
- else: # called to run a program by %run -p
- try:
- filename = get_py_filename(arg_lst[0])
- except IOError as e:
- try:
- msg = str(e)
- except UnicodeError:
- msg = e.message
- error(msg)
- return
+ Parameters
+ ----------
+ code : str
+ Code to be executed.
+ opts : Struct
+ Options parsed by `self.parse_options`.
+ namespace : dict
+ A dictionary for Python namespace (e.g., `self.shell.user_ns`).
- arg_str = 'execfile(filename,prog_ns)'
- namespace = {
- 'execfile': self.shell.safe_execfile,
- 'prog_ns': prog_ns,
- 'filename': filename
- }
+ """
- opts.merge(opts_def)
+ # Fill default values for unspecified options:
+ opts.merge(Struct(D=[''], l=[], s=['time'], T=['']))
prof = profile.Profile()
try:
- prof = prof.runctx(arg_str,namespace,namespace)
+ prof = prof.runctx(code, namespace, namespace)
sys_exit = ''
except SystemExit:
sys_exit = """*** SystemExit exception caught in code being profiled."""
@@ -327,8 +321,10 @@ def run(self, parameter_s='', runner=None,
file_finder=get_py_filename):
"""Run the named file inside IPython as a program.
- Usage:\\
- %run [-n -i -t [-N<N>] -d [-b<N>] -p [profile options] -G] file [args]
+ Usage:
+ %run [-n -i -e -G]
+ [( -t [-N<N>] | -d [-b<N>] | -p [profile options] )]
+ ( -m mod | file ) [args]
Parameters after the filename are passed as command-line arguments to
the program (put in sys.argv). Then, control returns to IPython's
@@ -541,62 +537,45 @@ def run(self, parameter_s='', runner=None,
# every single object ever created.
sys.modules[main_mod_name] = main_mod
+ if 'p' in opts or 'd' in opts:
+ if 'm' in opts:
+ code = 'run_module(modulename, prog_ns)'
+ code_ns = {
+ 'run_module': self.shell.safe_run_module,
+ 'prog_ns': prog_ns,
+ 'modulename': modulename,
+ }
+ else:
+ code = 'execfile(filename, prog_ns)'
+ code_ns = {
+ 'execfile': self.shell.safe_execfile,
+ 'prog_ns': prog_ns,
+ 'filename': get_py_filename(filename),
+ }
+
try:
stats = None
with self.shell.readline_no_record:
if 'p' in opts:
- stats = self.prun('', None, False, opts, arg_lst, prog_ns)
+ stats = self._run_with_profiler(code, opts, code_ns)
else:
if 'd' in opts:
- deb = debugger.Pdb(self.shell.colors)
- # reset Breakpoint state, which is moronically kept
- # in a class
- bdb.Breakpoint.next = 1
- bdb.Breakpoint.bplist = {}
- bdb.Breakpoint.bpbynumber = [None]
- # Set an initial breakpoint to stop execution
- maxtries = 10
- bp_file, bp_line = parse_breakpoint(opts.get('b', ['1'])[0], filename)
- checkline = deb.checkline(bp_file, bp_line)
- if not checkline:
- for bp in range(bp_line + 1, bp_line + maxtries + 1):
- if deb.checkline(bp_file, bp):
- break
- else:
- msg = ("\nI failed to find a valid line to set "
- "a breakpoint\n"
- "after trying up to line: %s.\n"
- "Please set a valid breakpoint manually "
- "with the -b option." % bp)
- error(msg)
- return
- # if we find a good linenumber, set the breakpoint
- deb.do_break('%s:%s' % (bp_file, bp_line))
-
- # Mimic Pdb._runscript(...)
- deb._wait_for_mainpyfile = True
- deb.mainpyfile = deb.canonic(filename)
-
- # Start file run
- print "NOTE: Enter 'c' at the",
- print "%s prompt to start your script." % deb.prompt
- ns = {'execfile': py3compat.execfile, 'prog_ns': prog_ns}
- try:
- #save filename so it can be used by methods on the deb object
- deb._exec_filename = filename
- deb.run('execfile("%s", prog_ns)' % filename, ns)
-
- except:
- etype, value, tb = sys.exc_info()
- # Skip three frames in the traceback: the %run one,
- # one inside bdb.py, and the command-line typed by the
- # user (run by exec in pdb itself).
- self.shell.InteractiveTB(etype, value, tb, tb_offset=3)
+ self._run_with_debugger(
+ code, code_ns, opts.get('b', ['1'])[0], filename)
else:
- if runner is None:
- runner = self.default_runner
- if runner is None:
- runner = self.shell.safe_execfile
+ if 'm' in opts:
+ def run():
+ self.shell.safe_run_module(modulename, prog_ns)
+ else:
+ if runner is None:
+ runner = self.default_runner
+ if runner is None:
+ runner = self.shell.safe_execfile
+
+ def run():
+ runner(filename, prog_ns, prog_ns,
+ exit_ignore=exit_ignore)
+
if 't' in opts:
# timed execution
try:
@@ -606,37 +585,10 @@ def run(self, parameter_s='', runner=None,
return
except (KeyError):
nruns = 1
- twall0 = time.time()
- if nruns == 1:
- t0 = clock2()
- runner(filename, prog_ns, prog_ns,
- exit_ignore=exit_ignore)
- t1 = clock2()
- t_usr = t1[0] - t0[0]
- t_sys = t1[1] - t0[1]
- print "\nIPython CPU timings (estimated):"
- print " User : %10.2f s." % t_usr
- print " System : %10.2f s." % t_sys
- else:
- runs = range(nruns)
- t0 = clock2()
- for nr in runs:
- runner(filename, prog_ns, prog_ns,
- exit_ignore=exit_ignore)
- t1 = clock2()
- t_usr = t1[0] - t0[0]
- t_sys = t1[1] - t0[1]
- print "\nIPython CPU timings (estimated):"
- print "Total runs performed:", nruns
- print " Times : %10s %10s" % ('Total', 'Per run')
- print " User : %10.2f s, %10.2f s." % (t_usr, t_usr / nruns)
- print " System : %10.2f s, %10.2f s." % (t_sys, t_sys / nruns)
- twall1 = time.time()
- print "Wall time: %10.2f s." % (twall1 - twall0)
-
+ self._run_with_timing(run, nruns)
else:
# regular execution
- runner(filename, prog_ns, prog_ns, exit_ignore=exit_ignore)
+ run()
if 'i' in opts:
self.shell.user_ns['__name__'] = __name__save
@@ -676,7 +628,114 @@ def run(self, parameter_s='', runner=None,
del sys.modules[main_mod_name]
return stats
-
+
+ def _run_with_debugger(self, code, code_ns, break_point, filename):
+ """
+ Run `code` in debugger with a break point.
+
+ Parameters
+ ----------
+ code : str
+ Code to execute.
+ code_ns : dict
+ A namespace in which `code` is executed.
+ break_point : str
+ Line number in the file specified by `filename` argument
+ or a string in the format ``file:line``. In the latter
+ case, `filename` is ignored.
+ See also :func:`.parse_breakpoint`.
+ filename : str
+ Path to the file in which break point is specified.
+
+ Raises
+ ------
+ UsageError
+ If no meaningful break point is given by `break_point` and
+ `filename`.
+
+ """
+ deb = debugger.Pdb(self.shell.colors)
+ # reset Breakpoint state, which is moronically kept
+ # in a class
+ bdb.Breakpoint.next = 1
+ bdb.Breakpoint.bplist = {}
+ bdb.Breakpoint.bpbynumber = [None]
+ # Set an initial breakpoint to stop execution
+ maxtries = 10
+ bp_file, bp_line = parse_breakpoint(break_point, filename)
+ checkline = deb.checkline(bp_file, bp_line)
+ if not checkline:
+ for bp in range(bp_line + 1, bp_line + maxtries + 1):
+ if deb.checkline(bp_file, bp):
+ break
+ else:
+ msg = ("\nI failed to find a valid line to set "
+ "a breakpoint\n"
+ "after trying up to line: %s.\n"
+ "Please set a valid breakpoint manually "
+ "with the -b option." % bp)
+ raise UsageError(msg)
@tkf
tkf added a note

@takluyver This was error(msg) and return. But as _run_with_debugge is a separated function now, return does not finish run. Do you think this is OK? I am not 100% sure that I can replace error(msg) and return with raise UsageError(msg).

@takluyver Owner

I think magics are supposed to raise a UsageError now, instead of error()ing and returning. It does mean that this code should only be called by a magic function, but since it's a private method in a Magics class, I think that's reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # if we find a good linenumber, set the breakpoint
+ deb.do_break('%s:%s' % (bp_file, bp_line))
+
+ # Mimic Pdb._runscript(...)
+ deb._wait_for_mainpyfile = True
+ deb.mainpyfile = deb.canonic(filename)
+
+ # Start file run
+ print "NOTE: Enter 'c' at the",
+ print "%s prompt to start your script." % deb.prompt
+ try:
+ #save filename so it can be used by methods on the deb object
+ deb._exec_filename = filename
+ deb.run(code, code_ns)
+
+ except:
+ etype, value, tb = sys.exc_info()
+ # Skip three frames in the traceback: the %run one,
+ # one inside bdb.py, and the command-line typed by the
+ # user (run by exec in pdb itself).
+ self.shell.InteractiveTB(etype, value, tb, tb_offset=3)
+
+ @staticmethod
+ def _run_with_timing(run, nruns):
+ """
+ Run function `run` and print timing information.
+
+ Parameters
+ ----------
+ run : callable
+ Any callable object which takes no argument.
+ nruns : int
+ Number of times to execute `run`.
+
+ """
+ twall0 = time.time()
+ if nruns == 1:
+ t0 = clock2()
+ run()
+ t1 = clock2()
+ t_usr = t1[0] - t0[0]
+ t_sys = t1[1] - t0[1]
+ print "\nIPython CPU timings (estimated):"
+ print " User : %10.2f s." % t_usr
+ print " System : %10.2f s." % t_sys
+ else:
+ runs = range(nruns)
+ t0 = clock2()
+ for nr in runs:
+ run()
+ t1 = clock2()
+ t_usr = t1[0] - t0[0]
+ t_sys = t1[1] - t0[1]
+ print "\nIPython CPU timings (estimated):"
+ print "Total runs performed:", nruns
+ print " Times : %10s %10s" % ('Total', 'Per run')
+ print " User : %10.2f s, %10.2f s." % (t_usr, t_usr / nruns)
+ print " System : %10.2f s, %10.2f s." % (t_sys, t_sys / nruns)
+ twall1 = time.time()
+ print "Wall time: %10.2f s." % (twall1 - twall0)
+
@skip_doctest
@line_cell_magic
def timeit(self, line='', cell=None):
View
77 IPython/core/tests/test_run.py
@@ -13,9 +13,13 @@
# Imports
#-----------------------------------------------------------------------------
+import functools
import os
+import random
import sys
import tempfile
+import textwrap
+import unittest
import nose.tools as nt
from nose import SkipTest
@@ -23,6 +27,8 @@
from IPython.testing import decorators as dec
from IPython.testing import tools as tt
from IPython.utils import py3compat
+from IPython.utils.tempdir import TemporaryDirectory
+from IPython.core import debugger
#-----------------------------------------------------------------------------
# Test functions begin
@@ -337,3 +343,74 @@ def test_run_formatting(self):
self.mktmp(src)
_ip.magic('run -t -N 1 %s' % self.fname)
_ip.magic('run -t -N 10 %s' % self.fname)
+
+
+class TestMagicRunWithPackage(unittest.TestCase):
+
+ def writefile(self, name, content):
+ path = os.path.join(self.tempdir.name, name)
+ d = os.path.dirname(path)
+ if not os.path.isdir(d):
+ os.makedirs(d)
+ with open(path, 'w') as f:
+ f.write(textwrap.dedent(content))
+
+ def setUp(self):
+ self.package = package = 'tmp{0}'.format(repr(random.random())[2:])
@takluyver Owner

I don't think we really need to randomly generate the package name, when we're already in a single-use temporary directory. We can just keep things simple and call it something like testpkg.

@tkf
tkf added a note

That won't work if you use testpkg somewhere else. As IPython has a huge code base, it is hard to avoid used names for a temporary module. See the fix I did for 2975de6. But if you still want to use testpkg, I'd change that.

@takluyver Owner

Oh, sorry, I'd forgotten that it needs to be unique for sys.modules as well as the file path. We could remove it from sys.modules in setUp/tearDown, but that's no simpler than just giving it a unique name, so never mind.

@tkf
tkf added a note

I tried this but "unloading" modules this way did not work so I will stick to the current version.

diff --git a/IPython/core/tests/test_run.py b/IPython/core/tests/test_run.py
index dac65ed..71402a2 100644
--- a/IPython/core/tests/test_run.py
+++ b/IPython/core/tests/test_run.py
@@ -356,9 +356,7 @@ def writefile(self, name, content):
             f.write(textwrap.dedent(content))

     def setUp(self):
-        self.package = package = 'tmp{0}'.format(repr(random.random())[2:])
-        """Temporary valid python package name."""
-
+        self.package = package = 'testpkg'
         self.value = int(random.random() * 10000)

         self.tempdir = TemporaryDirectory()
@@ -381,6 +379,11 @@ def tearDown(self):
         sys.path[:] = [p for p in sys.path if p != self.tempdir.name]
         self.tempdir.cleanup()

+        for name in [(n.startswith(self.package) or
+                      n.startswith(self.package + '.'))
+                     for n in sys.modules]:
+            del sys.modules[name]
+
     def check_run_submodule(self, submodule, opts=''):
         _ip.magic('run {2} -m {0}.{1}'.format(self.package, submodule, opts))
         self.assertEqual(_ip.user_ns['x'], self.value,
@takluyver Owner

Sticking with the current version sounds fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ """Temporary valid python package name."""
+
+ self.value = int(random.random() * 10000)
+
+ self.tempdir = TemporaryDirectory()
+ self.__orig_cwd = os.getcwdu()
+ sys.path.insert(0, self.tempdir.name)
+
+ self.writefile(os.path.join(package, '__init__.py'), '')
+ self.writefile(os.path.join(package, 'sub.py'), """
+ x = {0!r}
+ """.format(self.value))
+ self.writefile(os.path.join(package, 'relative.py'), """
+ from .sub import x
+ """)
+ self.writefile(os.path.join(package, 'absolute.py'), """
+ from {0}.sub import x
+ """.format(package))
+
+ def tearDown(self):
+ os.chdir(self.__orig_cwd)
+ sys.path[:] = [p for p in sys.path if p != self.tempdir.name]
+ self.tempdir.cleanup()
+
+ def check_run_submodule(self, submodule, opts=''):
+ _ip.magic('run {2} -m {0}.{1}'.format(self.package, submodule, opts))
+ self.assertEqual(_ip.user_ns['x'], self.value,
+ 'Variable `x` is not loaded from module `{0}`.'
+ .format(submodule))
+
+ def test_run_submodule_with_absolute_import(self):
+ self.check_run_submodule('absolute')
+
+ def test_run_submodule_with_relative_import(self):
+ """Run submodule that has a relative import statement (#2727)."""
+ self.check_run_submodule('relative')
+
+ def test_prun_submodule_with_absolute_import(self):
+ self.check_run_submodule('absolute', '-p')
+
+ def test_prun_submodule_with_relative_import(self):
+ self.check_run_submodule('relative', '-p')
+
+ def with_fake_debugger(func):
+ @functools.wraps(func)
+ def wrapper(*args, **kwds):
+ with tt.monkeypatch(debugger.Pdb, 'run', staticmethod(eval)):
+ return func(*args, **kwds)
+ return wrapper
+
+ @with_fake_debugger
+ def test_debug_run_submodule_with_absolute_import(self):
+ self.check_run_submodule('absolute', '-d')
+
+ @with_fake_debugger
+ def test_debug_run_submodule_with_relative_import(self):
+ self.check_run_submodule('relative', '-d')
Something went wrong with that request. Please try again.