Skip to content

Add adapter for msg spec versions#5752

Merged
minrk merged 6 commits into
ipython:masterfrom
minrk:msgspec-adapter
May 9, 2014
Merged

Add adapter for msg spec versions#5752
minrk merged 6 commits into
ipython:masterfrom
minrk:msgspec-adapter

Conversation

@minrk
Copy link
Copy Markdown
Member

@minrk minrk commented Apr 29, 2014

in IPython.kernel.adapter

Based on PR #4536

Basic changes:

  • Add IPython.kernel.adapter with translation code. Mainly an adapt(msg) function.
  • Adapt messages to/from the current version in the Session object.
  • Frontends request kernel_info for the protocol version, which selects the version to adapt to, if any.

With this PR, at least the Julia and R kernels work in all three zmq-based frontends, without updating to the v5 msg spec.

@minrk
Copy link
Copy Markdown
Member Author

minrk commented Apr 29, 2014

the real diff for this PR.

@minrk minrk added this to the 3.0 milestone May 7, 2014
@takluyver
Copy link
Copy Markdown
Member

With the kernel_info_channel bit: it took me a moment to understand that this is what the server uses to decide whether it needs to adapt messages. A comment about that would be useful.

@takluyver
Copy link
Copy Markdown
Member

The base Adapter class hardcodes that no transformations are done on messages where status is error or aborted. It seems a bit odd to implement this at the base class level. Can you justify that some more?

@takluyver
Copy link
Copy Markdown
Member

object_info_request going V5 -> V4 adds a 'status' field that shouldn't be there.

@takluyver
Copy link
Copy Markdown
Member

Min's mental note: Update display_json mechanisms - kernel and possibly JS

@takluyver
Copy link
Copy Markdown
Member

In V4toV5, you set version = kernel_protocol_version - should this be explicitly v5, just to hedge against the day that we have to do protocol v6?

@takluyver
Copy link
Copy Markdown
Member

On line 196, I think you could use _tuple_to_str().

@takluyver
Copy link
Copy Markdown
Member

There's a TODO in v4 to v5 complete_reply, but it's not clear to me what the current code will result in at the UI level.

@takluyver
Copy link
Copy Markdown
Member

Completed one review pass, only minor comments.

@minrk
Copy link
Copy Markdown
Member Author

minrk commented May 8, 2014

With the kernel_info_channel bit: it took me a moment to understand that this is what the server uses to decide whether it needs to adapt messages. A comment about that would be useful.

Comment added.

The base Adapter class hardcodes that no transformations are done on messages where status is error or aborted. It seems a bit odd to implement this at the base class level. Can you justify that some more?

Probably doesn't belong in the base class. It's just because that hasn't changed between 4 and 5. I'll move it to another method that subclasses can override, in case we do change it.

object_info_request going V5 -> V4 adds a 'status' field that shouldn't be there.

copy/paste error, fixed.

In V4toV5, you set version = kernel_protocol_version - should this be explicitly v5, just to hedge against the day that we have to do protocol v6?

Yes, probably.

On line 196, I think you could use _tuple_to_str().

That's actually why I wrote it, but then realized I don't need it and only would call it in one place, so I removed it.

In V4toV5, you set version = kernel_protocol_version - should this be explicitly v5, just to hedge against the day that we have to do protocol v6?

Makes sense, changed to '5.0'.

There's a TODO in v4 to v5 complete_reply, but it's not clear to me what the current code will result in at the UI level.

As discussed and demonstrated in person, this is changed to:

  • cursor_end = None
  • cursor_start = -len(matched_text)

which gets special handling in the frontend.

minrk added 6 commits May 9, 2014 12:04
for adapting msg spec versions
adds Session.adapt_version for target version.

- serialize adapts to Session.adapt_version (default: 0, no adaptation)
- unserialize always adapts to current active version
- docs and comments
- adapt complete_reply with `end=null`, `start=-len(matched_text)`
- remove some incorrect `status` fields
- add `handle_reply_status_error` for handling `status=error` replies (no-op, currently)
@minrk
Copy link
Copy Markdown
Member Author

minrk commented May 9, 2014

Rebased on master, now that #5436 is merged.

minrk added a commit that referenced this pull request May 9, 2014
Add adapter for msg spec versions
@minrk minrk merged commit aeae57d into ipython:master May 9, 2014
@minrk minrk deleted the msgspec-adapter branch May 9, 2014 20:02
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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.

2 participants