Merge lp:~vvatsa/ipython/ipcluster-dev into lp:~ellisonbg/ipython/trunk-dev

Proposed by Vishal Vatsa
Status: Merged
Merge reported by: Brian Granger
Merged at revision: not available
Proposed branch: lp:~vvatsa/ipython/ipcluster-dev
Merge into: lp:~ellisonbg/ipython/trunk-dev
To merge this branch: bzr merge lp:~vvatsa/ipython/ipcluster-dev
Reviewer Review Type Date Requested Status
Brian Granger Approve
Review via email: mp+1965@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vishal Vatsa (vvatsa) wrote :

currently the ssh cluster is managed by executing ssh and scp system commands, and it not very portable outside *nix, but it might be worth using paramiko for the ssh commands, should make it a hole lot more portable.

lp:~vvatsa/ipython/ipcluster-dev updated
1166. By Vishal Vatsa

accepting merge from Brians branch

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
Revision history for this message
Vishal Vatsa (vvatsa) wrote :

I will document the clusterfile stuff in Sphinx.

As for the sh files in the scripts dir, it does not matter about the remote system as both sshx.sh and engine_killer.sh are scp'd to /tmp and executed from there. So the only assumption is that there is a /tmp , (should be fine for all *nix and OSX) and there is a flag for custom sshx.sh.

Will look at the os.system call and replace them with twisted's version, although since I always start the controller 1st on local then start the remote engines (have not seen problems yet ;), guess I have not thought about the usecase if you want to start the controller over ssh also.

Will work more the docs. :)

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

> I will document the clusterfile stuff in Sphinx.

Great.

> As for the sh files in the scripts dir, it does not matter about the remote system as both sshx.sh and engine_killer.sh are scp'd to /tmp and executed from there. So the only assumption is that there is a /tmp , (should be fine for all *nix and OSX) and there is a flag for custom sshx.sh.

Nice, I guess I missed that point. In that case, leaving them in the
scripts dir makes sense.

I think we can and should be a bit safer about our assumptions about
/tmp. Can you either use the tempfile module or something like a tmp
subdir of the IPython directory (see genutils.get_ipython_dir)? I
think those would be more cross platform.

> Will look at the os.system call and replace them with twisted's version, although since I always start the controller 1st on local then start the remote engines (have not seen problems yet ;), guess I have not thought about the usecase if you want to start the controller over ssh also.

I could imagine that for some users, starting the controller somewhere
else is very desirable, but it definitely complicates things. It this
first new version, I am fine with forcing the controller to be started
locally. But this should be documented and evventually we should
relax this contraint.

> Will work more the docs. :)
>
>
>
> --
> https://code.launchpad.net/~vvatsa/ipython/ipcluster-dev/+merge/1965
> You are subscribed to branch lp:~ellisonbg/ipython/trunk-dev.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

lp:~vvatsa/ipython/ipcluster-dev updated
1167. By Vishal Vatsa

using twistd getProcessOuptut rather than
os.system, to make sure we are non-blocking.

1168. By Vishal Vatsa

oops, tiding up

1169. By Vishal Vatsa

oops, tiding up

1170. By Vishal Vatsa

handle the known case of trying to kill a ps pid, otherwise raise

1171. By Vishal Vatsa

use tempfile.tempdir to be more os netural going fwd.

1172. By Vishal Vatsa

added documentation for ipcluster ssh method

1173. By Vishal Vatsa

fixing up minor details

Revision history for this message
Vishal Vatsa (vvatsa) wrote :

The documentation for the ssh method is in parallel_process.txt

os.system calls have been dumped in favor of getProcessOuput.

Let me know if you have any other concerns or if I have missed something.

Revision history for this message
Min Ragan-Kelley (minrk) wrote :

Brian's requested revisions appear to have been implemented, and I don't see anything else needed.

Looks good to me, nice work!

lp:~vvatsa/ipython/ipcluster-dev updated
1174. By Vishal Vatsa

merge from BG branch

1175. By Vishal Vatsa

accepting merger from main ipython

1176. By Vishal Vatsa

was getting problems with shell scripts locations when deploying via eggs, now sh scripts are just embeded and written out to disk temp location when needed

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

Merged to trunk from ellisonbg's trunk-dev.

review: Approve

Subscribers

People subscribed via source and target branches

to all changes: