Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add version_request/reply messaging protocol #2649

Merged
merged 13 commits into from

6 participants

@tkf
tkf commented

Test in test_message_spec.py passes. No real world test is done yet.

Do we need user visible change (e.g., %kernelversion magic)?

@minrk
Owner

Version field is already in the msg header, what utility does this serve?

@ellisonbg
Owner
@Carreau
Owner

IIRC from the ruby integration I did [0,14,dev] https://github.com/Carreau/iruby/blob/master/lib/session.rb#L36

@jasongrout

Wow, the IPython version number is transmitted in every message with that change? Doesn't that seem a bit wasteful? Maybe it could be transmitted at the start of a session or something, but since the version number probably won't change in a single session, it seems a shame to have to waste the bandwidth.

@jasongrout

(Note that I like the idea of this pull request, which adds a new message handler instead of a new header field). Also, it does appear that @carreau is talking about the IPython version number, while @tkf is talking about the messaging spec version number.

@jasongrout

And: the confusion here between IPython version number and messaging spec version number indicates that this message should probably have a more descriptive type name, maybe protocol_version or something.

@tkf
tkf commented

@minrk Sorry, I should have clarified the context. This PR is based on the discussion in the mailing list:
http://thread.gmane.org/gmane.comp.python.ipython.devel/9500/

@Carreau What does it tells to client? Something like "you need at least this version"?

Probably @jasongrout is right about the protocol naming. So, should it be protocol_version_request and protocol_version_reply? Or message properties should be protocol_version, protocol_version_major, and so on?

@tkf
tkf commented

OK, so this is how IPython sends version of IPython itself.:

import IPython

_version_info_list = list(IPython.version_info)

def msg_header(msg_id, msg_type, username, session):
    date = datetime.now()
    version = _version_info_list
    return locals()

https://github.com/ipython/ipython/blob/6c36cd9332f1138bfe2910516b80f8a7c750ac40/IPython/zmq/session.py#L189

I agree with @jasongrout that this is a bit of waste. But probably a list in the header won't give you any significant penalty? I can't tell, as I don't do any benchmarks.

More important than that, if it is in the header, client needs to send a no-op message to get that. I guess execute_reply with an empty code does the job but maybe it is not straight forward for client writer.


BTW I'd write it this way:

def msg_header(**kwds):
    kwds.update(date=datetime.now(), version=_version_info_list)
    return kwds
@tkf
tkf commented

Other topic we need to discuss is weather we need the "patch version" (Z in X.Y.Z). @ellisonbg suggested to go without the patch version like notebook format number (i.e., X.Y) in the mailing list. Unlike notebook format number, kernel can change the behavior (i.e., bug fix) without changing protocol interface. This is why I suggested Semantic Versioning in the mailing list.

Disclaimer: I've never used the semantic versioning. I just thought it fit well with protocol versioning.

@minrk
Owner

Okay, then if we are adding this, I would suggest a few changes:

  • the protocol version should live in zmq.session, not ipkernel. session is where the protocol implementation is.
  • the version info currently included in the header should be removed, as it is wasteful and redundant.
  • the version request should reply with more information (at least IPython version, msg spec version, Python version, and maybe even platform info). This message should be the only one we need for a whole "who are you, and what are your capabilities"
  • a version tuple makes more sense than a dict that duplicates its own content

I think using semantic versioning is fine, but may be overkill - a 'bugfix' to a protocol definition doesn't make a lot of sense to me, but people generally expect 3-part versions, so we might as well use it, even if the last field is always zero.

@tkf
tkf commented

So the message spec will be something like this?

content = {
    'protocol_version': [int, int],  # or [int, int, int]?
    'ipython_version': [int, int],
    'python_version': [int, int, int],
    'platform': str,
}

I guess python_version will be ruby_version for @Carreau's ruby kernel.

@minrk
Owner

As you point out, with other kernel implementations, perhaps adding 'language' makes sense, or something more generic, such as 'kernel', which identifies the kernel implementation.

Also, IPython's version is a 3-tuple, not 2 (0.13.1, etc.) - dev should give inf, so Python 3's new comparison rules don't barf.

@Carreau
Owner

I guess python_version will be ruby_version for @Carreau's ruby kernel.

I guess you will have to add also 'julia_version' soon (even if it does not move fast...)

I agree with min that we should make the protocol without ipython/python specificities.

(Hoooo we can drag and drop images into comment field now....)

@tkf
tkf commented

OK, protocol spec version 3!

content = {
    'protocol_version': [int, int],       # or [int, int, int]?
    'ipython_version': [int, int, int],   # e.g. [0, 13, 0]
    'kernel_version': [int, int, int],    # e.g. [2, 7, 2]
    'language': str,                      # e.g., 'python'
    'platform': str,                      # = sys.platform
}

How about other general stuff, such as $HOST and $USER?

[0, 14, inf] for 0.14.dev feels wrong as it implies 0.14.0 < 0.14.dev. We never have 0.14.dev after 0.14.0, right? How about [0, 14, -1]?

Can't wait to see Julia kernel!

@tkf
tkf commented

I forgot platform.platform(). I guess it's better for 'platform'.

@tkf
tkf commented

Also, probably interpreter is better than language? It can be cpython/ironpython/pypy.

@minrk
Owner

[0, 14, inf] for 0.14.dev feels wrong as it implies 0.14.0 < 0.14.dev.

This is the correct relationship.

It is not important to place dev versions in-between patch revisions. dev should always evaluate as 'latest', so inf is actually correct. -1 would give the wrong semantics, as comparing with 0.14.0 should give True for any 0.14dev, whereas -1 would make that not true.

@minrk
Owner
@tkf
tkf commented

Well, I just thought kernel_version won't mean anything as long as you don't know what implementation of language you are talking to.

Also you can't stop clients to get interpreter name as they can execute any code. All the properties other than protocol_version and language is redundant in that sense. They just provides convenient access.

@minrk
Owner

Well, I just thought kernel_version won't mean anything as long as you don't know what implementation of language you are talking to.

pypy, CPython, and IronPython all report the Python version they implement. So I would expect a pypy interpreter to identify itself as Python 2.7.1. It's fine if interpreter is a separate field, but it shouldn't be used in place of the more significant fact that it implements a particular version of Python.

@tkf
tkf commented

Right. I was wrong about replacing the language property.

Close to the landing point?:

content = {
    'protocol_version': [int, int],       # or [int, int, int]?
    'ipython_version': [int, int, int],   # e.g. [0, 13, 0]
    'kernel_version': [int, int, int],    # e.g. [2, 7, 2]
    'language': str,                      # e.g., 'python'
    'interpreter': str,                   # e.g., platform.python_implementation()
    'platform': str,                      # ?
}

How about uname instead of platform? We can use platform.uname():

platform.uname()
Fairly portable uname interface. Returns a tuple of strings (system, node, release, version, machine, processor) identifying the underlying platform.

@jasongrout

Would other languages need to conform to the python uname format? Maybe we're overreaching ourselves here, and need to just go back to protocol version and language (and possibly language version). It's easier to add to the spec than to take away from it if we decide later we made a wrong decision.

@takluyver
Owner

I'm inclined to agree with @jasongrout - let's not try to add every potentially relevant field we can think of here.

(As to the question of language version numbers: my understanding is that the Python language has a two-part version number, e.g. 2.7, and bugfix releases of the implementations increment the third part. So CPython 2.7.2 is not directly comparable to IronPython 2.7.2.

@tkf
tkf commented

OK, so the remaining decision is the format of protocol_version. 2-part? 3-part? If 3-part, use semvar?

content = {
    'protocol_version': [int, int],       # or [int, int, int]?
    'ipython_version': [int, int, int],   # e.g. [0, 13, 0]
    'kernel_version': [int, int, int],    # e.g. [2, 7, 2]
    'language': str,                      # e.g., 'python'
    'interpreter': str,                   # e.g., platform.python_implementation()
}
@minrk
Owner
@tkf
tkf commented

I don't think it's particularly important to have extra information be
formal.

I think extra information is better be optional (can be omitted), but it
should be formal. If it is not, client depending on that information
must know which kernel send particular information in what format.
It may be end up defining its own code to fetch that data.

Plus, the msg spec version is fully redundant with the IPython version.

If you ignore users of master branch, yes. Also, it requires client
to know what version of IPython kernel is backward compatible to what
version.

@minrk
Owner

I think this much is fairly well settled, yes?

content = {
    'protocol_version': [int, int, int],       # or [int, int]?
    'ipython_version': [int, int, int],   # e.g. [0, 13, 0]
    'language_version': [int, int, int],    # e.g. [2, 7, 2]
    'language': str,                      # e.g., 'python', 'ruby', or 'R'
}

Here are some additional differentiators we may be interested in:

  • Python interpeter (@tkf proposed interpreter as a top-level key). I'm not sure this is needed for non-Python kernels.
  • Kernel implementation - for instance, if someone writes subclasses of the IPython kernel, which adds a few message types. The spec is meant to handle this, so it seems this is the place for such a Kernel to identify itself. We could use 'ipython' by default, and if we ever bring back the pure-Python kernel, we would call it 'ipython-pure' or something.
  • platform - It could be defined on a per-language basis, rather than trying to force each language to conform to Python's platform.platform().

If these aren't easily settled, we can go with the 4-key version above for now, and add more as we discover a need for them.

@ellisonbg
Owner
@tkf
tkf commented

I agree that @minrk's suggestion is good for a start.

Remaining decision: format of protocol_version. 2-part? 3-part? If 3-part, use semvar?

@minrk
Owner
@tkf
tkf commented

I am OK with 2-part version number, but the use case of patch number I have in mind to use it for bugfix in implementation.

(Now I understand what @takluyver said about the third number of Python version is implementation specific.)

For example, let's say in protocol version 1.4.0, history_request with hist_access_type = search, n > 100000 eats up memory for some reason and the kernel hangs. This is fixed in protocol version 1.4.1. So client can split up request for < 1.4.1 using n = 100 and start = 0, 100, ... [1] but use plain single request for >= 1.4.1. The patch version can provide information for client to know when to workaround or just disable some feature.

[1] although you can't use start in the current protocol; just imagine you can do it in this hypothetical protocol 1.4

Thinking about it, probably calling it kernel_version is better if we include the patch version? If it is 3-part version number, the first two parts are for protocol spec and the last part is for bug fixes, just like Python version number.

@minrk
Owner
@tkf

OK, I changed the protocol according to the discussion.

IPython/zmq/session.py
@@ -75,7 +74,9 @@ def squash_unicode(obj):
# globals and defaults
#-----------------------------------------------------------------------------
-_version_info_list = list(IPython.version_info)
+# Change this when incrementing the kernel protocol version
+protocol_version = [1, 1]
@minrk Owner
minrk added a note

minor nitpick, but let's set this to [4,0], since it is technically the fourth backwards-incompatible revision of the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
IPython/zmq/tests/test_message_spec.py
@@ -185,6 +196,13 @@ class CompleteReply(Reference):
matches = List(Unicode)
+class VersionReply(Reference):
+ protocol_version = Enum((ipkernel.protocol_version,))
+ ipython_version = Enum((ipkernel.ipython_version,))
+ language_version = Enum((ipkernel.language_version,))
+ language = Enum(("python",))
@minrk Owner
minrk added a note

None of these are actually enums - they should be List and Unicode.

@tkf
tkf added a note

But you can't check values with List and Unicode. I'd use Constant if there is such trait. But Constant is a special version of Enum, so I thought Enum is fine. But I think using List and Unicode is fine. The test will be weaker, though.

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

Applied @minrk's suggestion.

@minrk
Owner

Thanks, makes sense to me, then.

Things left to do:

  • fix the 2.6 failure (you might be relying on changes to fancy formatting)
  • add these messages to the msg_spec doc
IPython/zmq/tests/test_message_spec.py
((4 lines not shown))
+def Version(num, trait=Integer):
+ return List(trait, default_value=[0] * num, minlen=num, maxlen=num)
+
+
+class VersionReply(Reference):
+
+ protocol_version = Version(2)
+ ipython_version = Version(4, Any)
+ language_version = Version(3)
+ language = Unicode()
+
+ def _ipython_version_changed(self, name, old, new):
+ for v in new:
+ nt.assert_true(
+ isinstance(v, int) or isinstance(v, basestring),
+ 'expected int or string as version component, got {!r}'
@takluyver Owner

This is what's causing the test failure on 2.6 - the field needs to be {0!r} there.

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

@minrk @takluyver Thanks, I fixed the failure and added the document.

@tkf

What version number should be used for a kernel implementation which does not support all the protocols? For example, you don't need history request protocol to support current IPython notebook so it makes sense to release a kernel without history request. One possibility is to reserve minor version 0 for such "partial implementation". Or we can later add a way to "raise" not implemented error.

@minrk
Owner
@tkf

If the partial implementation problem should be solved by the mechanism other than version number, I guess the discussion should go the other issue thread. Shall I open one?

@minrk
Owner
@tkf

What I meant was that if we are solving the problem by playing with version number, we need to change version number in this pull request. If not, we can do it later.

But probably we should do it here. As you said, if we don't want another message type, probably we should just change the message name to capabilities_request and capabilities_reply.

@minrk
Owner
@tkf

@minrk I like kernel_info. Renamed the message.

@tkf tkf referenced this pull request from a commit in tkf/emacs-ipython-notebook
@tkf tkf Add ein:kernel-kernel-info-request c897626
@tkf

Are there anything left to do?

@ellisonbg
Owner

I haven't followed the discussion too closely, but overall the code looks good. One thing that I did see is that the protocol version is buried in the session.py module. I think we should put it in a more prominent location, perhaps, in the same location where we put the rest of IPython's version information.

@ellisonbg
Owner

I see @minrk recommended putting the protocol version in session.py. @minrk do you think it makes sense to put it in IPython.core.release with the other version information? I am wondering if we should put the nbformat stuff there as well. You followed this PR more closely than I did, so I will leave it up to you.

@minrk
Owner

@ellisonbg I don't have strong feelings about it anymore - release makes some sense, but I initially picked Session as that's where the implementation lives. I could go either way.

core.release has the advantage that it will always be importable for IPython.sys_info, so maybe there.

@minrk
Owner

per @ellisonbg's recommendation, let's move the protocol_version to core.release, then I think this is ready to go.

IPython/core/release.py
@@ -38,6 +38,9 @@
version = __version__ # backwards compatibility name
version_info = (_version_major, _version_minor, _version_micro, _version_extra)
+# Change this when incrementing the kernel protocol version
+kernel_protocol_version = (4, 0)
@tkf
tkf added a note

Should we call this kernel_protocol_version_info, to be consistent with version_info?

@minrk Owner
minrk added a note

yes - best to be consistent

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

@minrk Fixed.

@minrk
Owner

Great, I will merge. Thanks!

@minrk minrk merged commit 22c83b3 into from
@tkf tkf referenced this pull request from a commit in tkf/emacs-ipython-notebook
@tkf tkf Add ein:kernel-kernel-info-request 735e14e
@minrk minrk referenced this pull request from a commit in minrk/ipython
@tkf tkf Change version protocol according to the discussion in #2649 dc1f0c1
@mattvonrocketstein mattvonrocketstein referenced this pull request from a commit in mattvonrocketstein/ipython
@tkf tkf Change version protocol according to the discussion in #2649 fd22636
@smoofra smoofra referenced this pull request from a commit in smoofra/emacs-ipython-notebook
@tkf tkf Merge branch 'kernel-info-request'
Add support for `kernel_info` request for IPython kernel protocol.
See: ipython/ipython#2649
1023327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
3  IPython/core/release.py
@@ -38,6 +38,9 @@
version = __version__ # backwards compatibility name
version_info = (_version_major, _version_minor, _version_micro, _version_extra)
+# Change this when incrementing the kernel protocol version
+kernel_protocol_version_info = (4, 0)
+
description = "IPython: Productive Interactive Computing"
long_description = \
View
18 IPython/zmq/ipkernel.py
@@ -39,6 +39,7 @@
from IPython.config.application import boolean_flag, catch_config_error
from IPython.core.application import ProfileDir
from IPython.core.error import StdinNotImplementedError
+from IPython.core import release
from IPython.core.shellapp import (
InteractiveShellApp, shell_flags, shell_aliases
)
@@ -61,6 +62,11 @@
# Main kernel class
#-----------------------------------------------------------------------------
+protocol_version = list(release.kernel_protocol_version_info)
+ipython_version = list(release.version_info)
+language_version = list(sys.version_info[:3])
+
+
class Kernel(Configurable):
#---------------------------------------------------------------------------
@@ -156,6 +162,7 @@ def __init__(self, **kwargs):
# Build dict of handlers for message types
msg_types = [ 'execute_request', 'complete_request',
'object_info_request', 'history_request',
+ 'kernel_info_request',
'connect_request', 'shutdown_request',
'apply_request',
]
@@ -509,6 +516,17 @@ def connect_request(self, stream, ident, parent):
content, parent, ident)
self.log.debug("%s", msg)
+ def kernel_info_request(self, stream, ident, parent):
+ vinfo = {
+ 'protocol_version': protocol_version,
+ 'ipython_version': ipython_version,
+ 'language_version': language_version,
+ 'language': 'python',
+ }
+ msg = self.session.send(stream, 'kernel_info_reply',
+ vinfo, parent, ident)
+ self.log.debug("%s", msg)
+
def shutdown_request(self, stream, ident, parent):
self.shell.exit_now = True
content = dict(status='ok')
View
6 IPython/zmq/kernelmanager.py
@@ -363,6 +363,12 @@ def history(self, raw=True, output=False, hist_access_type='range', **kwargs):
self._queue_send(msg)
return msg['header']['msg_id']
+ def kernel_info(self):
+ """Request kernel info."""
+ msg = self.session.msg('kernel_info_request')
+ self._queue_send(msg)
+ return msg['header']['msg_id']
+
def shutdown(self, restart=False):
"""Request an immediate kernel shutdown.
View
3  IPython/zmq/session.py
@@ -43,7 +43,6 @@
from zmq.eventloop.ioloop import IOLoop
from zmq.eventloop.zmqstream import ZMQStream
-import IPython
from IPython.config.application import Application, boolean_flag
from IPython.config.configurable import Configurable, LoggingConfigurable
from IPython.utils.importstring import import_item
@@ -75,7 +74,6 @@ def squash_unicode(obj):
# globals and defaults
#-----------------------------------------------------------------------------
-_version_info_list = list(IPython.version_info)
# ISO8601-ify datetime objects
json_packer = lambda obj: jsonapi.dumps(obj, default=date_default)
json_unpacker = lambda s: extract_dates(jsonapi.loads(s))
@@ -188,7 +186,6 @@ def __getitem__(self, k):
def msg_header(msg_id, msg_type, username, session):
date = datetime.now()
- version = _version_info_list
return locals()
def extract_header(msg_or_header):
View
49 IPython/zmq/tests/test_message_spec.py
@@ -21,7 +21,7 @@
from IPython.testing import decorators as dec
from IPython.utils import io
from IPython.utils.traitlets import (
- HasTraits, TraitError, Bool, Unicode, Dict, Integer, List, Enum,
+ HasTraits, TraitError, Bool, Unicode, Dict, Integer, List, Enum, Any,
)
#-----------------------------------------------------------------------------
@@ -83,7 +83,17 @@ def execute(code='', **kwargs):
class Reference(HasTraits):
-
+
+ """
+ Base class for message spec specification testing.
+
+ This class is the core of the message specification test. The
+ idea is that child classes implement trait attributes for each
+ message keys, so that message keys can be tested against these
+ traits using :meth:`check` method.
+
+ """
+
def check(self, d):
"""validate a dict against our traits"""
for key in self.trait_names():
@@ -185,6 +195,25 @@ class CompleteReply(Reference):
matches = List(Unicode)
+def Version(num, trait=Integer):
+ return List(trait, default_value=[0] * num, minlen=num, maxlen=num)
+
+
+class KernelInfoReply(Reference):
+
+ protocol_version = Version(2)
+ ipython_version = Version(4, Any)
+ language_version = Version(3)
+ language = Unicode()
+
+ def _ipython_version_changed(self, name, old, new):
+ for v in new:
+ nt.assert_true(
+ isinstance(v, int) or isinstance(v, basestring),
+ 'expected int or string as version component, got {0!r}'
+ .format(v))
+
+
# IOPub messages
class PyIn(Reference):
@@ -226,12 +255,16 @@ def _data_changed(self, name, old, new):
'object_info_reply' : OInfoReply(),
'status' : Status(),
'complete_reply' : CompleteReply(),
+ 'kernel_info_reply': KernelInfoReply(),
'pyin' : PyIn(),
'pyout' : PyOut(),
'pyerr' : PyErr(),
'stream' : Stream(),
'display_data' : DisplayData(),
}
+"""
+Specifications of `content` part of the reply messages.
+"""
def validate_message(msg, msg_type=None, parent=None):
@@ -421,6 +454,18 @@ def test_complete():
yield nt.assert_true(name in matches, "Missing match: %r" % name)
+@dec.parametric
+def test_kernel_info_request():
+ flush_channels()
+
+ shell = KM.shell_channel
+
+ msg_id = shell.kernel_info()
+ reply = shell.get_msg(timeout=2)
+ for tst in validate_message(reply, 'kernel_info_reply', msg_id):
+ yield tst
+
+
# IOPub channel
View
41 docs/source/development/messaging.txt
@@ -670,6 +670,47 @@ Message type: ``connect_reply``::
}
+Kernel info
+-----------
+
+If a client needs to know what protocol the kernel supports, it can
+ask version number of the messaging protocol supported by the kernel.
+This message can be used to fetch other core information of the
+kernel, including language (e.g., Python), language version number and
+IPython version number.
+
+Message type: ``kernel_info_request``::
+
+ content = {
+ }
+
+Message type: ``kernel_info_reply``::
+
+ content = {
+ # Version of messaging protocol (mandatory).
+ # The first integer indicates major version. It is incremented when
+ # there is any backward incompatible change.
+ # The second integer indicates minor version. It is incremented when
+ # there is any backward compatible change.
+ 'protocol_version': [int, int],
+
+ # IPython version number (optional).
+ # Non-python kernel backend may not have this version number.
+ # The last component is an extra field, which may be 'dev' or
+ # 'rc1' in development version. It is an empty string for
+ # released version.
+ 'ipython_version': [int, int, int, str],
+
+ # Language version number (mandatory).
+ # It is Python version number (e.g., [2, 7, 3]) for the kernel
+ # included in IPython.
+ 'language_version': [int, ...],
+
+ # Programming language in which kernel is implemented (mandatory).
+ # Kernel included in IPython returns 'python'.
+ 'language': str,
+ }
+
Kernel shutdown
---------------
Something went wrong with that request. Please try again.