Skip to content

kernel.js consistency with generic IPython message format.#2000

Closed
kramer314 wants to merge 1 commit into
ipython:masterfrom
kramer314:js-msgformat
Closed

kernel.js consistency with generic IPython message format.#2000
kramer314 wants to merge 1 commit into
ipython:masterfrom
kramer314:js-msgformat

Conversation

@kramer314
Copy link
Copy Markdown
Contributor

Messages sent by the notebook js currently only include the id and type in the header of the message, not the top level of the message. This is inconsistent with the documented generic message format found at http://ipython.org/ipython-doc/dev/development/messaging.html#general-message-format :

"A message's unique identifier and type are stored in the header but are also accessible at the top-level for convenience."

The inconsistency currently doesn't have any impact on the functionality of the ipython notebook, but could create issues for other people trying to use portions of the notebook's javascript files for managing an IPython kernel independent of the notebook, while also attempting to do other things with the generated messages.

A message's unique identifier and type are stored in the header but are
also accessible at the top-level for convenience.
@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 21, 2012

Test results for commit 89ffa1e merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib oct2py pygments pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 21, 2012

This looks clean, but before merging it, I guess we should ask whether we want this fix or the converse: changing the spec to not promise these two fields being available at the top-level. We've obviously been able to work fine without them by reading them off the header, so the question is whether it's really worth anything having that duplication. I'm now leaning very mildly towards not, but I'd like to hear other opinions.

@minrk, you're the one who has most recently dealt with the message spec, what's your take here?

@minrk
Copy link
Copy Markdown
Member

minrk commented Jun 22, 2012

From the docs:

The msg's unique identifier and type are stored in the header, but
are also accessible at the top-level for convenience.

These values are copied from the header to the top-level for convenience in code, storing these values at the top-level is not part of the wire protocol, but part of the Python API provided by the Session object. Only the header, parent header, and content are sent over the wire.

This PR should not be merged.

@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 22, 2012

Ah, are they copied when the message is turned into a python object?

@minrk
Copy link
Copy Markdown
Member

minrk commented Jun 22, 2012

There is a bit of ambiguity that should be clarified in the docs:

  • A message is three dictionaries: header, parent header, content.
  • For convenience, the Session object copies the msg_type and msg_id out of the header when it unpacks messages - these as top-level keys are not part of the message spec, but rather a part of the extended API provided by the Python implementation of the message spec.

@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 22, 2012

Right, I now see the code in zmq/session.py...

@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 22, 2012

@kramer314, we'll close this PR then, and we should improve the wording of the docs as indicated by @minrk above. If you're willing to do that, great!

@fperez fperez closed this Jun 22, 2012
@kramer314
Copy link
Copy Markdown
Contributor Author

Makes sense. I'll go ahead and modify the docs and submit a new PR. Thanks for the clarification!

@fperez
Copy link
Copy Markdown
Member

fperez commented Jun 22, 2012

Excellent, thanks for contributing!

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.

3 participants