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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

kramer314
Copy link
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
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
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
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
Member

fperez commented Jun 22, 2012

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

@minrk
Copy link
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
Member

fperez commented Jun 22, 2012

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

@fperez
Copy link
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
Contributor Author

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

@fperez
Copy link
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.

None yet

3 participants