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

IMessage.buffers kernel testing, protocol changelog #520

Closed
jamesdbrock opened this issue Feb 25, 2020 · 9 comments · Fixed by #522
Closed

IMessage.buffers kernel testing, protocol changelog #520

jamesdbrock opened this issue Feb 25, 2020 · 9 comments · Fixed by #522

Comments

@jamesdbrock
Copy link

jamesdbrock commented Feb 25, 2020

I want to add support for IMessage.buffers to the IHaskell Jupyter kernel. IHaskell/IHaskell#1144

Once I've implemented this, how can I test it, without writing a special client extension specifically to test this feature?

Question 1: Is there some part of stock Jupyter or JupyterLab client which uses IMessage.buffers, and which will fail if the kernel is not correctly receiving or sending IMessage.buffers?

What version of Jupyter client protocol added IMessage.buffers, and what else is supported in that version? Here is the changelog for jupyter-client. https://jupyter-client.readthedocs.io/en/stable/changelog.html

Here is the jupyter-client messaging doc, which mentions changes to the Jupyter client protocol: https://jupyter-client.readthedocs.io/en/stable/messaging.html

Question 2: Is there a changelog for the Jupyter client protocol?

Edit: crossed out question 1, because that's a frontend question.

@jamesdbrock
Copy link
Author

Gitter https://gitter.im/jupyterlab/jupyterlab?at=5e5487899aeef6523218189e

Jason Grout @jasongrout 11:44
FYI, the Image ipywidgets widget uses binary message buffers to transfer the image data

Jason Grout @jasongrout 11:45
That may be the simplest thing in stock Jupyter that uses binary message buffers
especially if you already have ipywidgets support
If you have generic comm support, you could write a custom comm object that transfers things with binary buffers

James Brock @jamesdbrock 11:46
yeah... IHaskell no longer supports ipywidgets, the support bitrotted because we didn't keep up with the protocol changes.

@jasongrout
Copy link
Member

The specific IMessage.buffers interface sounds like a haskell implementation detail, not terminology from the protocol spec. I assume you are referring to the buffers key in the General Message Format?

I couldn't find which spec version had the buffers key added, but here is the commit that added it to the docs: 2bf9602. Viewing the file at that commit shows it was already showing it was message spec 5.0, so presumably this was a change in spec 5.1. However, raw data buffers was already in the wire format from the very first version of that message spec documentation file, which was apparently copied from IPython. I didn't trace it back in the ipython repo more.

@jasongrout
Copy link
Member

The jupyter_client changelog does mention spec changes, so presumably it doubles as a spec changelog (perhaps the official changelog is the inline comments in the messaging spec document?)

It looks to me like there should probably be a note added to the message spec document saying that the optional buffers key in the general message format should tagged as a spec 5.1 change.

@minrk - thoughts?

@minrk
Copy link
Member

minrk commented Feb 26, 2020

We should clarify that General Message Format section (#522), because for kernels and such, this never really exists. It's an implementation detail for each kernel / logical deserialization of the wire format. The only 'true' protocol is the wire protocol, which is what's sent and received over the network. Kernels are free to deserialize this as they like. Deserializing a message to a dict where the header is a field called 'header', buffers are a field called 'buffers', etc. is pretty sensible, but not dictated anywhere. So what 'buffers' really means is all zmq frames arriving after the content in a multipart message.

#522 aims to do this.

And yes, since the spec is contained in this repo, changes to the spec are included in the changelog here, and also represented with Changed in.. notes in the messaging doc. We could maintain a separate changelog for the protocol, if that would be helpful, though.

@jamesdbrock
Copy link
Author

The specific IMessage.buffers interface sounds like a haskell implementation detail, not terminology from the protocol spec. I assume you are referring to the buffers key in the General Message Format?

Yes that one, which is IMessage.buffers in JupyerLab services:

https://github.com/jupyterlab/jupyterlab/blob/b66bb79116285ccbd0876a6263ac2c0d6de27c33/packages/services/src/kernel/messages.ts#L277

We could maintain a separate changelog for the protocol, if that would be helpful, though.

Yes, that's what I'm looking for, a changelog for the Jupyter client message protocol.

I was hoping to find an easy answer to the question: If we already support General Message Format 5.0, then what do we have to add to support General Message Format 5.3?

(It looks like the IHaskell kernel was last upgraded to version "5.0", when the version key was added to the General Message Format header )

This closed issue is a vote for yes please, a separate changelog for the protocol.

@jasongrout
Copy link
Member

The only 'true' protocol is the wire protocol, which is what's sent and received over the network.

Very good point, thanks for clarifying.

To be clear, the wire protocol refers to the header, parent, metadata, and content dicts, so by extension the structure of each of those dicts is part of the actual versioned protocol, not just the representation.

@minrk
Copy link
Member

minrk commented Feb 27, 2020

by extension the structure of each of those dicts is part of the actual versioned protocol, not just the representation.

Yes! The only piece that's not part of it is how to combine those separate dicts (and one list) into a single Message object, which was a bit ambiguous in the doc since we showed a Message as a dict.

We also have not yet versioned and documented the actual second wire protocol used by the notebook server and web frontends. That's technically part of the Jupyter Notebook Server spec, not this zmq protocol spec, but the overlap is pretty large so maybe they should be together (at the very least cross-linked).

I opened #525 to add a changelog, mostly just copying the change notes into a transpose representation (grouped by version then message type rather than the reverse). I'm not sure about it.

@jamesdbrock
Copy link
Author

These are good answers to my question, thank you @jasongrout @minrk

@kevin-bates
Copy link
Member

Thinking about future stuff, we'll need to decide how best to organize this kind of stuff once the proposed kernel management framework is adopted. This essentially splits jupyter_client into jupyter_kernel_mgmt and jupyter_protocol, with the idea that the latter is the defacto location for protocol definitions and methods.

cc: @takluyver

jamesdbrock added a commit to jamesdbrock/IHaskell that referenced this issue Feb 27, 2020
https://jupyter-client.readthedocs.io/en/stable/messaging.html#the-wire-protocol

Resolves issue

IHaskell#1144

This field has been in "the Wire Protocol" since before the Jupyter Message
specification version 5.0.

jupyter/jupyter_client#520

I've tested this feature with a proprietary JupyterLab extension and
I've verified that it works. It's difficult to provide a public
reproducible test.

TODO test sending
jamesdbrock added a commit to jamesdbrock/IHaskell that referenced this issue Feb 28, 2020
The Wire Protocol allows for "extra raw data buffer(s)" at the end of a
ZeroMQ message. This commit enables buffers in the ipython-kernel.

https://jupyter-client.readthedocs.io/en/stable/messaging.html#the-wire-protocol

This field has been in "the Wire Protocol" since before the Jupyter Message
specification version 5.0.

jupyter/jupyter_client#520

Resolves issue

IHaskell#1144

I've tested this feature with a proprietary JupyterLab extension and
I've verified that it works. It's difficult to provide a public
reproducible test. The best test may be to get ipywidgets Image working--
that uses buffers.
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 a pull request may close this issue.

4 participants