Code review comment for lp:~vvatsa/ipython/ipcluster-dev

Revision history for this message
Brian Granger (ellisonbg) wrote :

A few comments:

clusterfile.py should be moved into the Sphinx documentation on this stuff.

Do you think that putting sshx.sh and engine_killer.sh into the scripts dir makes sense. I guess looking at __file__ allows you to grab them, but doesn't that assume that their location is the same on all machines. This often won't be the case (I have a Mac laptop, but run on a Linux cluster). What about just having them installed somewhere when IPython is installed? But I am not sure if distutils can install .sh files. Could you investigate these issues?

Calls to os.system should be replaced by Twisted's getProcessOutput, which will be done asynchronously. This will require a rewrite of some of the code to use Deferred's though. Please ask if you want help doing this. Because os.system blocks, it will mess up Twisted, thus it should be avoided in our Twisted using code. See the PBS engine set for examples of how this is used.

Can you document the places where you make assumptions that Windows isn't the OS? That way, it will make it easier to eventually get working on Windows. Which hosts need to be posix based and not Windows for example?

Once you finish coding, we will also have you document this in our sphinx docs in docs/source/parallel/parallel_process.py.

Looks like a great start though. Thanks for the work on this!!! Don't hesitate to ask questions.

review: Needs Fixing

« Back to merge proposal