Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow connection to kernels by files #847

Merged
merged 17 commits into from Oct 13, 2011
Merged

Allow connection to kernels by files #847

merged 17 commits into from Oct 13, 2011

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 9, 2011

This moves all the connection info for a kernel into JSON files like the parallel code, so there's no need to specify 4 ports to connect to an existing kernel. In fact, if you just specify ipython qtconsole --existing, it will try to connect to the most recent kernel, including those started by the notebook.

It also cleans up some KernelManager code in the notebook, so that changes to the base KernelManager will be inherited in the notebook. The main immediate benefit being that restarting a notebook kernel really does restart it, so attached qtconsoles will remain attached.

I will also turn HMAC signatures on by default before merging.

This should address #688, #806, and even probably #691.

@minrk
Copy link
Member Author

minrk commented Oct 10, 2011

HMAC signatures are now on by default in all situations, including notebook, kernel, and qtconsole.

@minrk
Copy link
Member Author

minrk commented Oct 10, 2011

Also add %connect_info magic, to make it easy to connect secondary clients.

For reference, this function may be useful to folks like @fperez, who like to connect a qtconsole to their current notebook:

def connect_qtconsole():
    from subprocess import Popen, PIPE
    from IPython.zmq.kernelapp import KernelApp
    app = KernelApp.instance()
    return Popen(['ipython', 'qtconsole', '--existing', app.connection_file])

Which, when called, will launch a qtconsole connected to the kernel that called it. This applies to kernels started with a notebook, other qtconsole, etc.

@fperez
Copy link
Member

fperez commented Oct 10, 2011

@minrk, that's great, and I'd actually vote for wrapping it in a magic also, that we expose interactively for all kernels. Havig the function api is useful for scripted control, but users would simply type %qtconsole. I love the usability this would provide!

@minrk
Copy link
Member Author

minrk commented Oct 10, 2011

%qtconsole magic added

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

@ivanov can you check if any of the changes here cause problems for you with vim-ipython? That's the only third party client I know of. The change of ip/port pairs to just ports might mean you have to make an adjustment, but in the long run it should be simpler, because you shouldn't have to keep track of ports at all.

@fperez
Copy link
Member

fperez commented Oct 11, 2011

@minrk, thanks a lot for the magic! I think this is a major usability improvement, awesome work. Will wait for @ivanov's feedback before moving further.

@fperez
Copy link
Member

fperez commented Oct 11, 2011

Just to say, %qtconsole is awesome. Using it now in production work, it rocks.

@ivanov
Copy link
Member

ivanov commented Oct 11, 2011

looking into it now...

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

I peeked into vim-ipython a bit, and it shouldn't be a big deal. You just need to specify one argument: BlockingKM(connection_file=/path/to/kernel-12345.json) instead of trying to parse the argv. The check for whether this is available will be:

if 'connection_file' in BlockingKernelManager.class_trait_names():
    # 0.12
    km = BlockingKernelManager(connection_file=s)
else:
    # parse line as before for 0.11

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

It did require a small change to be convenient, but I think it should be working now: ivanov/vim-ipython#13

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

Just a note: this will certainly conflict with #813 in qtconsoleapp.py, so let's pick merge order carefully, as that one has already been rebased at least once.

@ivanov
Copy link
Member

ivanov commented Oct 11, 2011

as usual, Min fixed the issues i was going to complain about, even before I had the chance to verbalize the complaint :)

My only concern is related to profiles, since, as I understand it - clients aren't apps, but the connection_file get stored in the active profile's security/ folder. I suppose it's not a big issue, since the connection_info magic prints the files contents

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

as usual, Min fixed the issues i was going to complain about, even before I had the chance to verbalize the complaint :)

We do aim to please.

My only concern is related to profiles, since, as I understand it - clients aren't apps, but the connection_file get stored in the active profile's security/ folder. I suppose it's not a big issue, since the connection_info magic prints the files contents

Note that you can always specify the full path of the connection file, regardless of profiles, etc. The profile
security dir is just the default, and used if only a filename is given.

@ivanov
Copy link
Member

ivanov commented Oct 11, 2011

Note that you can always specify the full path of the connection file, regardless of profiles, etc.

yes, true, but you have to work a bit to get that full path, since it isn't what's printed, but that's ok.

The profile security dir is just the default, and used if only a filename is given.

right - so i guess that's the trade off - before, i didn't have to know anything about profiles, just hookup to the right ports and go. Now it seems I have to look in the profile directory in order to get to the file (though, i suppose, there's nothing stopping me from using the contents of the file as printed by the connection_info magic in the same manner as i used to in 0.11)

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

Note that you can always specify the full path of the connection file, regardless of profiles, etc.

yes, true, but you have to work a bit to get that full path, since it isn't what's printed, but that's ok.>> The profile security dir is just the default, and used if only a filename is given.

The full path is only not printed if it's in the default location, so if the output is just --existing kernel-12345.json, that unambiguously means IPYTHONDIR/profile_default/security/kernel-12345.json. It will also print the profile name if it is anything other than default (--existing kernel-12345.json --profile foo), and it will print the full path in all other cases.

right - so i guess that's the trade off - before, i didn't have to know anything about profiles, just hookup to the right ports and go. Now it seems I have to look in the profile directory in order to get to the file (though, i suppose, there's nothing stopping me from using the contents of the file as printed by the connection_info magic in the same manner as i used to in 0.11)

Yes, it's a tradeoff. An alternate approach would be if I made the KernelManager itself profile-aware (it's not currently), which would mean that clients using the KernelManager would generally not need to be. An example of the KernelManager not being profile aware - if you create a KernelManager unconfigured and use it to start a Kernel, the connection file will go in a temp dir:

>>> from IPython.zmq.blockingkernelmanager import BlockingKernelManager
>>> km = BlockingKernelManager()
>>> km.start_kernel()
[IPKernelApp] To connect another client to this kernel, use:
[IPKernelApp] --existing /var/folders/zz/v3fz17cd2nz0bqvpd2s7btq00000gn/T/tmpMBJMP6.json
>>> km.start_channels()

So it's only non-IPython apps that want to start clients connected to IPython-started kernels under default config that are at any disadvantage.

We could also add an API like:

get_security_file(fname, profile='default'):
    """returns security file from IPython profile directory"""
    return os.path.join(get_ipython_dir(), 'profile_%s' % profile, 'security', fname)

So that apps would not need to know about our directory structure, only the more abstract idea of profiles.

@ivanov
Copy link
Member

ivanov commented Oct 11, 2011

The full path is only not printed if it's in the default location, so if the output is just --existing kernel-12345.json, that unambiguously means IPYTHONDIR/profile_default/security/kernel-12345.json.  It will also print the profile name if it is anything other than default (--existing kernel-12345.json --profile foo), and it will print the full path in all other cases.

Oh, I didn't realize this - that's actually very good!

Yes, it's a tradeoff.  An alternate approach would be if I made the KernelManager itself profile-aware (it's not currently), which would mean that clients using the KernelManager would generally not need to be.

I think it's great as is right now - I can't think of a reason why the
KernelManager would need to be profile aware, and with your suggestion
below, I think it'll be great to keep the profile-agnostic KMs.

We could also add an API like:

get_security_file(fname, profile='default'):
   """returns security file from IPython profile directory"""
   return os.path.join(get_ipython_dir(), 'profile_%s' % profile, 'security', fname)

So that apps would not need to know about our directory structure, only the more abstract idea of profiles.

This would be great - it was exactly the kind of thing I couldn't
figure out quickly enough last night when I started looking at this PR

@minrk
Copy link
Member Author

minrk commented Oct 11, 2011

get_security_file() added to utils.path

# alias passed with arg via space
swallow_next = True

def init_connection_file(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small docstring here summarizing what this puppy does would be good...

@fperez
Copy link
Member

fperez commented Oct 13, 2011

I only have some trivial cosmetic comments on most of it (made inline), and one of substance on the magics: let's make both magics be trivial wrappers around regular Python library functions. Testing magics is kind of annoying, and using them from other code also awkward, b/c you have to call them via the magic system. And for example, the %connect_info should be split into a function that just builds that info string --but doesn't print it, simply returns it--, and the actual magic to simply do print connection_info(). That makes it easy for other clients who may want to show this same info to collect it as a string from a single location.

Same thing for the %qtconsole, even though it's easy code, it should just be a wrapper around a library function. The point of the magics, as I see them now, is just to isolate a namespace of convenient things, but they should not have much implementation code in them. Historically that hasn't been true (%run is a monster), but we should move towards that model. Ultimately, I'd like the magics to be all more or less trivial wrappers around regular library calls.

@fperez
Copy link
Member

fperez commented Oct 13, 2011

Other than the above trivial cosmetic fixes and the slightly more substantial (but easy) fixing of the magics, I'm +100 on merging this ASAP. Unless anyone objects, let's merge it once it's fixed up; the improvements it brings are really major. Fantastic work!

@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

I did the cleanup on qtconsoleapp.

The library functions make perfect sense. What would you call them, and where would you put them? IPython.lib.kernel? They are general utilities for connecting to a kernel. Should the get_connection_info() function return the unpacked JSON dict, the filename, or the string contents of the file?

all foo_address traits became foo_port, adding one 'ip' trait for all

This matches the Kernel, which does not allow specifying multiple
IPs for each channel.
$> ipqt --shell 12345

would not scrub the port, previously
this addresses security issues (ipython#688)
and ease-of-multi-frontend
This means that proper restart is now available, rather than
killing and starting a new kernel, breaking connections to
secondary frontends.
add session_aliases and session_flags to consolidate these settings

use new consolidated flags in parallel apps as well

add default_secure() function for use in apps where the default
behavior should be to enable HMAC signatures.
This is separate from previous, because it is more likely to be rejected.

It requires the the Session objects in Handlers get a reference all the way back up to the IPython App that started the environment.
for loading connection info from its connection_file
ensure str_to_bytes and bytes_to_str are used to transform Session.key to/from JSON
This makes it easier for third party tools to retrieve security files
(e.g. json connection files) by name and [optional] profile name alone, without
knowledge of IPython's directory structure.

For example, whenever ipkernel outputs:

` --existing kernel-12345.json --profile foo`

that file can be found with:

get_security_file('kernel-12345.json', profile='foo')
@ivanov
Copy link
Member

ivanov commented Oct 13, 2011

not sure if i'm OT, but I'd like to also see init_ssh functionality grafted out of qtconsoleapp and placed wherever other general utilities go

@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

@ivanov argh! That's a good idea. I will never be done with this.

@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

@ivanov, tunnel_to_kernel should now be a generic function for tunneling connections to a kernel, given a connection file or its unpacked contents.

@fperez
Copy link
Member

fperez commented Oct 13, 2011

On Wed, Oct 12, 2011 at 9:00 PM, Min RK
reply@reply.github.com
wrote:

The library functions make perfect sense.  What would you call them, and where would you put them? IPython.lib.kernel? They are general utilities for connecting to a kernel. Should the get_connection_info() function return the unpacked JSON dict, the filename, or the string contents of the file?

  • IPython.lib.kernel sounds good for a location...
  • Maybe call it get_connection_file and have it return the path is
    the best one. From the path a simple open(path).read() gets the data,
    where as given the data one can't reconstruct the path. How does that
    sound?

principally adding big docstring to complicated init_ssh method
get_connection_info and connect_qtconsole are now implemented as library functions
in the new module.  The zmqshell magics that call them are now simple wrappers.
added to new IPython.lib.kernel with other kernel connection utilities.
@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

I also split the connection file search into a library function, so that will be easier to reuse as well.

* fileglob-based searching for connection file added as
  IPython.lib.kernel.find_connection_file()

* also use this search in other lib.kernel methods, to make them
  more useful when run outside an IPython kernel.
@fperez
Copy link
Member

fperez commented Oct 13, 2011

@minrk, this is looking pretty solid, and big enough we shouldn't let it linger much further. I can't think of much more to do on it at this point, can you? Otherwise, let's push it through. Awesome job!

@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

I'm just playing with various use cases to make sure it works, then I'll go ahead and merge.

@ivanov, I also updated the vim-ipython PR, to use the new glob-based file search

minrk added a commit that referenced this pull request Oct 13, 2011
* JSON connection files are now used to connect files
* HMAC message signing is now on by default in all IPython apps
* Adds IPython.lib.kernel, which contains utility functions for connecting
  clients. These were mostly copied out of qtconsoleapp in order to be
  more general.
* Adds %connection_info and %qtconsole magics to zmqshell

closes gh-688
closes gh-806
closes gh-691
@minrk minrk merged commit 48450ef into ipython:master Oct 13, 2011
@minrk
Copy link
Member Author

minrk commented Oct 13, 2011

merged

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
* JSON connection files are now used to connect files
* HMAC message signing is now on by default in all IPython apps
* Adds IPython.lib.kernel, which contains utility functions for connecting
  clients. These were mostly copied out of qtconsoleapp in order to be
  more general.
* Adds %connection_info and %qtconsole magics to zmqshell

closes ipythongh-688
closes ipythongh-806
closes ipythongh-691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants