Add version_request/reply messaging protocol #2649

Merged
merged 13 commits into from Jan 15, 2013

Projects

None yet

6 participants

@tkf
tkf commented Dec 4, 2012

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
Member
minrk commented Dec 5, 2012

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

@ellisonbg
Member

I don't see that in the message spec. What version is written there, the
ipython version of a messaging protocol version?

On Wed, Dec 5, 2012 at 2:41 PM, Min RK notifications@github.com wrote:

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


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11064359.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@Carreau
Member
Carreau commented Dec 6, 2012

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

@jasongrout
Member

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
Member

(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
Member

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 Dec 7, 2012

@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 Dec 7, 2012

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 Dec 7, 2012

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
Member
minrk commented Dec 7, 2012

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 Dec 7, 2012

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
Member
minrk commented Dec 7, 2012

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
Member
Carreau commented Dec 7, 2012

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 Dec 7, 2012

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 Dec 7, 2012

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

@tkf
tkf commented Dec 7, 2012

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

@minrk
Member
minrk commented Dec 7, 2012

[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
Member
minrk commented Dec 7, 2012

On Fri, Dec 7, 2012 at 12:16 PM, Takafumi Arakaki
notifications@github.comwrote:

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

You would have to be careful, as you don't want to require frontends to
support a whitelist of Python interpreters to tell if a kernel is a Python
one.
For most frontends, the only thing that would matter is the language, not
the interpreter.

@tkf
tkf commented Dec 7, 2012

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
Member
minrk commented Dec 7, 2012

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 Dec 7, 2012

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
Member

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
Member

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 Dec 7, 2012

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
Member
minrk commented Dec 7, 2012

On Fri, Dec 7, 2012 at 2:20 PM, Jason Grout notifications@github.comwrote:

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.

I don't think it's particularly important to have extra information be
formal. We should define a core message (spec version, IPython version,
language, language version), and anything beyond that should be considered
extra identifying information - it could even go in a subdict called
'details', which is not actually defined in the spec as anything other than
a black box (like 'content' in any other message).

I think more information is better, but there should be a line between
'this is the formal spec, which all kernels must provide and frontends can
rely upon', and 'extra identifying information'. For instance, it may make
sense to include zmq / pyzmq versions, but the use of zmq is not even a
part of the msg spec, so the spec wouldn't change even if zmq is totally
dropped.

We may even be going beyond what is useful here - a significant change to
the msg spec will mean that the message won't even be readable, so it
doesn't matter what's inside. Plus, the msg spec version is fully
redundant
with the IPython version.

Depending on how we define the message spec, we haven't made a release yet
that wouldn't require a major version bump (0.14 is incompatible with 0.13
is incompatible with 0.12 is incompatible with 0.11, taking the full spec
into account). So currently, and probably for the next few releases at
least, IPython version == msg spec version.

Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11148903.

@tkf
tkf commented Dec 8, 2012

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
Member
minrk commented Dec 9, 2012

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
Member

I like this 4 key version. We can always add new keys later if they
become important.

On Sat, Dec 8, 2012 at 4:28 PM, Min RK notifications@github.com wrote:

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 https://github.com/tkf proposed interpreteras 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.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11165752.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@tkf
tkf commented Dec 9, 2012

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
Member
minrk commented Dec 9, 2012

On Sat, Dec 8, 2012 at 6:22 PM, Takafumi Arakaki
notifications@github.comwrote:

I agree that @minrk https://github.com/minrk's suggestion is good for a
start.

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

2-part seems to cover it, but I wouldn't object to having the third field
anyway. Can anyone propose a scenario where the message spec itself would
actually have a 'patch'? I can't think of one.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11166676.

@tkf
tkf commented Dec 9, 2012

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
Member
minrk commented Dec 9, 2012

If it's an implementation bugfix, then that's information already contained in the IPython version. It's not related to
the protocol.

@tkf
tkf commented Dec 12, 2012

OK, I changed the protocol according to the discussion.

@minrk minrk commented on an outdated diff Dec 14, 2012
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
minrk Dec 14, 2012 IPython member

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

@minrk minrk and 1 other commented on an outdated diff Dec 14, 2012
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
minrk Dec 14, 2012 IPython member

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

@tkf
tkf Dec 14, 2012

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.

@tkf
tkf commented Dec 14, 2012

Applied @minrk's suggestion.

@minrk
Member
minrk commented Dec 14, 2012

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
@takluyver takluyver commented on an outdated diff Dec 15, 2012
IPython/zmq/tests/test_message_spec.py
+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
takluyver Dec 15, 2012 IPython member

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

@tkf
tkf commented Dec 15, 2012

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

@tkf
tkf commented Dec 15, 2012

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
Member
minrk commented Dec 15, 2012

On Sat, Dec 15, 2012 at 12:10 PM, Takafumi Arakaki <notifications@github.com

wrote:

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".

A generic number does not make sense to express 'partial' support, where
the part is undefined. I see two options:

  1. make this a 'capabilities' message, which replies with a list of
    supported requests.
  2. add that 'kernel' key, so that kernel + kernel_version is meaningful

That is, the dict with kernel='ipython', ipython_version=[0, 14, 0] means
the kernel in IPython version 0.14.0.
If you write your own kernel, then it would be kernel='tkfspecial', tkf_special=[1,2,3].

The advantage of 1. is that frontends don't need to know about which
implementations support which messages.

Or we can later add a way to "raise" not implemented error.

There is already a single location where the kernel handles unrecognized
message types. It would be quite easy to add the behavior for this to reply
with unsupported msg type: foo_request. I think right now, it just
ignores the request as invalid, but the kernel will only get to this point
if the requester is authenticated, so I think a reply is acceptable.

https://github.com/ipython/ipython/blob/master/IPython/zmq/ipkernel.py#L231

@tkf
tkf commented Dec 15, 2012

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
Member
minrk commented Dec 15, 2012

On Sat, Dec 15, 2012 at 1:05 PM, Takafumi Arakaki
notifications@github.comwrote:

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?

yes, I suppose so - though I don't expect we will actually need more than
one message type. These are not two separate problems,
but just one - how to identify a kernel and its capabilities.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11410399.

@tkf
tkf commented Dec 15, 2012

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
Member
minrk commented Dec 15, 2012

On Sat, Dec 15, 2012 at 3:34 PM, Takafumi Arakaki
notifications@github.comwrote:

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.

Sure, or just 'kernel_info_req/rep'.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/2649#issuecomment-11412021.

@tkf
tkf commented Dec 16, 2012

@minrk I like kernel_info. Renamed the message.

@tkf tkf added a commit to tkf/emacs-ipython-notebook that referenced this pull request Dec 18, 2012
@tkf tkf Add ein:kernel-kernel-info-request c897626
@tkf
tkf commented Jan 10, 2013

Are there anything left to do?

@ellisonbg
Member

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
Member

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
Member
minrk commented Jan 14, 2013

@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
Member
minrk commented Jan 14, 2013

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

@tkf tkf and 1 other commented on an outdated diff Jan 15, 2013
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 Jan 15, 2013

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

@minrk
minrk Jan 15, 2013 IPython member

yes - best to be consistent

@tkf
tkf commented Jan 15, 2013

@minrk Fixed.

@minrk
Member
minrk commented Jan 15, 2013

Great, I will merge. Thanks!

@minrk minrk merged commit 22c83b3 into ipython:master Jan 15, 2013
@tkf tkf added a commit to tkf/emacs-ipython-notebook that referenced this pull request Jan 18, 2013
@tkf tkf Add ein:kernel-kernel-info-request 735e14e
@minrk minrk added a commit to minrk/ipython that referenced this pull request Jan 26, 2013
@tkf tkf Change version protocol according to the discussion in #2649 dc1f0c1
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@tkf tkf Change version protocol according to the discussion in #2649 fd22636
@smoofra smoofra pushed a commit to smoofra/emacs-ipython-notebook that referenced this pull request Mar 21, 2015
@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