iptest: Use a temporary directory for IPYTHONDIR #1888

bfroehle · 2012-06-08T19:39:08Z

Begin working on #1880 (Add parallelism to iptest & test_pr) by using a temporary directory for IPYTHONDIR for each IPython test group.

bfroehle · 2012-06-08T19:41:48Z

IPython/testing/iptest.py

-            return os.system(' '.join(self.call_args))
+            with TemporaryDirectory() as IPYTHONDIR:
+                return os.system(' '.join(['IPYTHONDIR=%s' % IPYTHONDIR] +
+                                          self.call_args))


I have NOT tested this in Windows, so this may, or may not, actually work.

Wouldn't using an env dict as we do on other platforms work more reliably? We should be able to use subprocess.call in place of os.system.

Here's the full comment, which you can't see in this diff view:

# On Windows, use os.system instead of subprocess.call, because I # was having problems with subprocess and I just don't know enough # about win32 to debug this reliably. Os.system may be the 'old # fashioned' way to do it, but it works just fine. If someone # later can clean this up that's fine, as long as the tests run # reliably in win32. # What types of problems are you having. They may be related to # running Python in unboffered mode. BG.

I think unbuffered mode was something we needed when we were still using twisted, so maybe that's not applicable any more. We really need more Windows testers for things like this. Let's cook up something we think should work, then ping jstenar to test it.

I'm going to look into obtaining a reliable and persistent Windows box we can all log into remotely so we can once and for all have more solid Windows testing and access to easier debugging on Windows as needed. Stay tuned, though do go ahead on this with @jstenar as well in the meantime. It also occurred to me that @jdmarch might have access to Windows testing, any chance you know what to do here, Jonathan?

jdmarch · 2012-06-11T03:34:19Z

A few comments, mostly in the context of Windows. Wish I had a clear & full response but I am out of time tonight, mostly offline for the next week, and probably out of time for the week after that.

The use of os.system here was already fragile because call_args[1](the path to ipytest.py) may contain spaces (e.g. if ipython source is in user home dir on Win XP); while this would not bother Popen, os.system cannot interpret it correctly unless it is explicitly quoted (which it is not).
Even before this PR, there are a number of (presumably unrelated) test failures on Windows. In particular, Windows: parallel test fails assert, leaves 14 python processes alive #1901 complicates evaluation of the PR.
Even apart from (1), I would expect Popen to work as well or better than os.system, at least for running a single test. Both before and after this PR, when I use the non-Windows version of _run_cmd for Windows, one test at a time, the tests seem to run no worse than before (cf (2)).
The possible issues are in running multiple tests, when there are failures. It's not clear to me how well this code works then in Windows.
I am using Python 2.7. Note that os.kill is not supported in Windows prior to Python 2.7. I would need to understand better how the recursive test runner handles failures in *nix before I would have an opinion whether even with this we would be any worse off with Popen than with os.system.

takluyver · 2012-06-11T10:19:19Z

OK, so by the sounds of it we should try changing it to subprocess.call and seeing what - if anything - breaks?

fperez · 2012-06-11T18:36:23Z

That could be done, and we can ask @jstenar to have a test of the branch on Windows as well...

bfroehle · 2012-06-22T01:54:57Z

Well, to really get the ball rolling here I've removed the windows os.system call and run the different test groups in their own processes using multiprocessing.Pool.map.

Now this is pretty wasteful. By that, I mean multiprocessing spins up a fresh Python process to run 'runner.run()' which then launches a new Python process for the actual test.

$ ps f
  PID TTY      STAT   TIME COMMAND
 1791 pts/21   Ss     0:00 bash
 4565 pts/21   R+     0:00  \_ ps f
29891 pts/5    Ss     0:00 bash
 4462 pts/5    Sl+    0:00  \_ /usr/bin/python /home/bfroehle/.local/bin/iptest
 4467 pts/5    S+     0:00      \_ /bin/sh -c octave -q --braindead
 4468 pts/5    S+     0:00      |   \_ octave -q --braindead
 4469 pts/5    S+     0:00      \_ /usr/bin/python /home/bfroehle/.local/bin/iptest
 4491 pts/5    D+     0:00      |   \_ /usr/bin/python /home/bfroehle/src/ipython/IPython/testing/iptest.py IPython.config
 4555 pts/5    S+     0:00      |       \_ /bin/sh -c octave -q --braindead
 4557 pts/5    S+     0:00      |           \_ octave -q --braindead
 4470 pts/5    S+     0:00      \_ /usr/bin/python /home/bfroehle/.local/bin/iptest
 4488 pts/5    D+     0:00      |   \_ /usr/bin/python /home/bfroehle/src/ipython/IPython/testing/iptest.py IPython.core
 4547 pts/5    S+     0:00      |       \_ /bin/sh -c octave -q --braindead
 4550 pts/5    S+     0:00      |           \_ octave -q --braindead
 4471 pts/5    S+     0:00      \_ /usr/bin/python /home/bfroehle/.local/bin/iptest
 4489 pts/5    D+     0:00      |   \_ /usr/bin/python /home/bfroehle/src/ipython/IPython/testing/iptest.py IPython.extensions
 4553 pts/5    S+     0:00      |       \_ /bin/sh -c octave -q --braindead
 4554 pts/5    S+     0:00      |           \_ octave -q --braindead
 4472 pts/5    S+     0:00      \_ /usr/bin/python /home/bfroehle/.local/bin/iptest
 4490 pts/5    D+     0:00      |   \_ /usr/bin/python /home/bfroehle/src/ipython/IPython/testing/iptest.py IPython.frontend
 4543 pts/5    S+     0:00      |       \_ /bin/sh -c octave -q --braindead
 4544 pts/5    S+     0:00      |           \_ octave -q --braindead
...

But it does cut the test time down to ~56 seconds (from ~136 seconds).

fperez · 2012-06-22T03:59:02Z

That looks like pretty good progress! I'm going to tag this as 0.14 material though, because it makes pretty serious changes that I'm not comfortable with this close to release time...

jdmarch · 2012-06-24T01:35:59Z

On WinXP-32, test IPython.core has a failure with this PR, but not with PR #2016, and not when run freestanding.
This is in addition to #2017 and #2018, core test issues which are independent of this PR.

======================================================================
FAIL: Testing of issue https://github.com/ipython/ipython/issues/1107
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\documents and settings\jdm\ipython\IPython\core\tests\test_completerlib.py", line 83, in test_import_invalid_module
    assert valid_module_names.issubset(s), valid_module_names.intersection(s)
AssertionError: set([])
>>  assert set(['foobar']).issubset(set([])), set(['foobar']).set([])(set([]))

-------------------- >> begin captured stdout << ---------------------

Caching the list of root modules, please wait!
(This will only be done once - type '%rehashx' to reset cache!)

This is taking too long, we give up.


--------------------- >> end captured stdout << ----------------------

jdmarch · 2012-06-24T01:47:37Z

On WinXP-32, test IPython.zmq freezes during post-test cleanup with this PR, but not with PR #2016, and not when run freestanding. Possible segfault (eventually the Windows error reporting dialog pops up). Leaves many python processes alive but nothing responsive.

**********************************************************************
IPython test group: IPython.zmq
.......................................................................................................................................
...............
----------------------------------------------------------------------
Ran 160 tests in 21.656s

OK
Exception AttributeError: AttributeError("'NoneType' object has no attribute 'remove'",) in <bound method BlockingKernelManager.__del__
 of <IPython.zmq.blockingkernelmanager.BlockingKernelManager object at 0x05F47FB0>> ignored

fperez · 2012-06-24T08:24:13Z

Thanks for those tests, @jdmarch! This is 0.14 material, so there's no danger of introducing instability for the release, but having this feedback is excellent so we can get this one in good shape.

bfroehle · 2012-07-17T00:38:46Z

This pull request is too unwieldy. I'm going to parcel it out into a few distinct pull requests, beginning with #2148: "win32 iptest: Use subprocess.Popen() instead of os.system()."

fperez · 2012-07-20T13:05:18Z

Great thanks!

bfroehle reviewed Jun 8, 2012
View reviewed changes

bfroehle added 3 commits June 21, 2012 18:20

iptest: Use a temporary directory for IPYTHONDIR

a2e65b3

iptest: Use subprocess.Popen instead of os.system in win32.

93a106f

Run each IPython test group in its own process.

86a8e1a

This was referenced Jun 23, 2012

Windows: test runner fails if repo path contains spaces #760

Closed

BUG: test runner fails in Windows if filenames contain spaces. #2016

Merged

bfroehle closed this Jul 17, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iptest: Use a temporary directory for IPYTHONDIR #1888

iptest: Use a temporary directory for IPYTHONDIR #1888

bfroehle commented Jun 8, 2012

bfroehle Jun 8, 2012

takluyver Jun 8, 2012

bfroehle Jun 8, 2012

takluyver Jun 8, 2012

fperez Jun 9, 2012

jdmarch commented Jun 11, 2012

takluyver commented Jun 11, 2012

fperez commented Jun 11, 2012

bfroehle commented Jun 22, 2012

fperez commented Jun 22, 2012

jdmarch commented Jun 24, 2012

jdmarch commented Jun 24, 2012

fperez commented Jun 24, 2012

bfroehle commented Jul 17, 2012

fperez commented Jul 20, 2012

iptest: Use a temporary directory for IPYTHONDIR #1888

iptest: Use a temporary directory for IPYTHONDIR #1888

Conversation

bfroehle commented Jun 8, 2012

bfroehle Jun 8, 2012

Choose a reason for hiding this comment

takluyver Jun 8, 2012

Choose a reason for hiding this comment

bfroehle Jun 8, 2012

Choose a reason for hiding this comment

takluyver Jun 8, 2012

Choose a reason for hiding this comment

fperez Jun 9, 2012

Choose a reason for hiding this comment

jdmarch commented Jun 11, 2012

takluyver commented Jun 11, 2012

fperez commented Jun 11, 2012

bfroehle commented Jun 22, 2012

fperez commented Jun 22, 2012

jdmarch commented Jun 24, 2012

jdmarch commented Jun 24, 2012

fperez commented Jun 24, 2012

bfroehle commented Jul 17, 2012

fperez commented Jul 20, 2012