Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Various Windows-related fixes to IPython.parallel #374

Merged
merged 11 commits into from Apr 21, 2011

Conversation

Projects
None yet
3 participants
Owner

minrk commented Apr 13, 2011

This is the a pull request containing fixes that, when merged should hopefully close #365.

Issues fixed:

  • multiprocessing does not propagate Config objects properly, so they are coerced to dict and back for Scheduler launch
  • on Windows, select cannot poll non-socket FDs, so subprocess IO is forwarded over zmq sockets in ipcluster
  • SIGINT caused a crash when shutting down subprocesses in ipcluster, so it is replaced with CTRL_C_EVENT on Windows
  • allow win32api to be missing on import of IPython.parallel.apps.launcher

The removal of the module-specific default serialization selection in StreamSession is not Windows specific, but is done for simplicity. While the stdlib json in 2.6 is extremely slow compared to jsonlib, the chance of having defaults differ between machines is too annoying to keep this behavior.

Owner

fperez commented Apr 14, 2011

Any feedback on the list about this?

It passes the test suite (modulo a failing test that's also in master) for me, but I only tested on linux :) Unfortunately I can't test for a few days on windows, with all the moving coming up...

Owner

minrk commented Apr 14, 2011

Real usage seems to be working fine, but several tests don't pass on Windows, so I need to look into those.

Owner

fperez commented Apr 14, 2011

On Thu, Apr 14, 2011 at 3:07 PM, minrk
reply@reply.github.com
wrote:

Real usage seems to be working fine, but several tests don't pass on Windows, so I need to look into those.

Ah, OK. If you can reproduce the failures, then at least you have a
shot at fixing them. I'll open an issue for the parallel failure I'm
seeing; I'll make it critical because it's already failing on master.

Owner

minrk commented Apr 15, 2011

All tests now pass on Windows. I believe there are still some signaling / shutdown / process cleanup issues to work out, but it's very close at least.

Owner

minrk commented Apr 17, 2011

Now have 100% tests passing on my Windows VMs, and I can't seem to make processes hang around longer than they should, so I think this is ready for review/merge.

Owner

ellisonbg commented Apr 20, 2011

Min, have you tested this branch on a non-Windows system. I have often found that fixing Windows bugs, leads to non-Windows bugs. Other than that, I think this is fine.

minrk added some commits Apr 12, 2011

@minrk minrk always default to json packer in streamsession b491840
@minrk minrk handle potentially absent win32api in launcher.py b5a7d66
@minrk minrk forward subprocess IO over zmq on Windows
This is required on Windows, because select can only poll
sockets, unlike [all] other platforms
3e8626a
@minrk minrk interrupt windows subprocesses with CTRL-C instead of SIGINT
CTRL_C_EVENT is new in 2.7
22552e5
@minrk minrk pass config obj to Scheduler as dict
This is another Windows fix.  Windows doesn't properly
preserve the config object across the multiprocessing spawn.
4312fde
@minrk minrk remove @default_block from client, in favor of clearer implementation
also remove dead _execute/_push/_pull methods from client after transition to View as primary API provider
b191e9a
@minrk minrk Start each test with clear engine namespaces
closes gh-386
0d3d807
@minrk minrk use crash that will also kill a Windows engine
crash is from the Python test suite

Windows crash dialog is suppressed prior to crash

also fixed 'assertRaisesRemote' message, which had names backwards
151aca8
@minrk minrk move check for any engines into _build_targets
This prevents load-balancing apply submissions from raising NoEnginesRegistered
c005f92
@minrk minrk improve process cleanup on Windows
* use system taskkill call to shutdown process trees on Windows
* use apps.launchers in tests, to make use of the fix

Also remove unnecessary and sometimes wrong assert in test_asyncresult

As of this commit, all parallel tests pass on Windows
1a93dba
Owner

minrk commented Apr 20, 2011

I have indeed. The one change I haven't made is making IPython.parallel depend on pyzmq-2.1.5 on Windows, which it does require, due to fixes in zmq.poll and zmqstream.flush.

However, we haven't released pyzmq-2.1.5, so I don't know what exactly I should do there. Should I add the dependency, noting that the code has not been released and really means 2.1dev? Also, should I make the dependency consistent, and bump the dep to 2.1.5 across the board, or make it different for Windows and non-Windows?

Owner

ellisonbg commented Apr 21, 2011

There was a problem with zmq 2.1.5, so we should wait on releasing 2.1.5 as well. At some level, we simply need to wait for zmq 2.1.5 and until that comes out and we can release 2.1.5, we should probably put a dep of 2.1dev. Not sure of a better way of handling it. Obviously, simple messages/docs informing the user of the dep is going to be better than a format dependency in code.

@minrk minrk depend on pyzmq-2.1dev on Windows
This is a temporary dependency, and should be changed to the next release of pyzmq when it arrives.
877366d

@minrk minrk merged commit 877366d into ipython:master Apr 21, 2011

@minrk minrk referenced this pull request Apr 21, 2011

Merged

DB fixes and Scheduler HWM #389

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