Updating the message spec (finish IPEP 13, 24) #4536

Merged
merged 34 commits into from May 9, 2014

Projects

None yet

9 participants

@minrk
Member
minrk commented Nov 14, 2013

I will implement IPEP 24 here as well, once some discussion has happened there.

closes #854
closes #2825
closes #2826

Main changes:

  • pyout -> execute_result (no effect on nbformat)
  • pyerr -> error (same)
  • pyin -> execute_input
  • remove user_variables (redundant with user_expressions)
  • user_expressions is mime-bundle
  • kernel_info versions are strings
  • add banner to kernel_info
  • add version string to message headers
  • support getpass in input_request messages
  • lex kernel-side in complete/object_info requests
  • object_info_reply is mime-bundle
  • pager is mime-bundle
  • completions may include extra info

I plan to add a shim for translating between msg spec 4.1 and 5.0, but I will do that after this PR is finalized, so I'm not writing a translation against a moving target.

@ivanov ivanov referenced this pull request Nov 26, 2013
Merged

propagate display metadata to all mimetypes #4533

9 of 9 tasks complete
@ivanov ivanov and 1 other commented on an outdated diff Nov 26, 2013
IPython/html/static/notebook/js/outputarea.js
@@ -298,9 +296,9 @@ var IPython = (function (IPython) {
}
if (json.output_type === 'pyout') {
@ivanov
ivanov Nov 26, 2013 Member

drive-by comment, but wont' this key change, to === 'execute_result' as well?

@minrk
minrk Nov 26, 2013 Member

unfortunately, no (or not yet) - changing that value would mean changing the notebook format. Only the message types are changed. In nbformat 4, the json (nbformat) will catch up.

@ivanov ivanov commented on an outdated diff Nov 26, 2013
IPython/html/static/notebook/js/outputarea.js
@@ -298,9 +296,9 @@ var IPython = (function (IPython) {
}
if (json.output_type === 'pyout') {
- this.append_pyout(json, dynamic);
+ this.append_execute_result(json, dynamic);
} else if (json.output_type === 'pyerr') {
@ivanov
ivanov Nov 26, 2013 Member

=== 'error' here

@minrk minrk added this to the 3.0 milestone Apr 2, 2014
@minrk
Member
minrk commented Apr 9, 2014

I think this is getting close to ready, so I would really like feedback from IJulia (@stevengj) and IHaskell (@gibiansky) on the changes here, especially the proposals in completion and object info.

IPEP 13: msg spec updates
IPEP 24: completion and object_info

@stevengj
stevengj commented Apr 9, 2014

Where is the documention of the changes, especially to object_info_reply and completion requests?

I'm not sure what the pager is .... the "payload" stuff in execute_reply was undocumented last I checked, or what getpass requests are (they aren't mentioned in the messaging spec online).

@minrk
Member
minrk commented Apr 9, 2014

The changes are documented in the two IPEP links I made in the comment above.

@stevengj
stevengj commented Apr 9, 2014

Regarding completion requests, one of the things we discussed with @fperez recently (see Fernando's notes) is that we'd like:

  • The request to consist of just the entire cell and the cursor position (and you need to document whether this is a byte or character offset for UTF8 data... we would prefer the former). e.g. the front-end shouldn't try to break it into lines (as Julia expressions can easily span multiple lines).
  • The reply should consist of (start position, end position, list of replacements). The reason is that we would like to be able to do tab substitution, not just replacement, e.g. replace \alpha<tab> with α (see JuliaLang/julia#6340).

I'm not a big fan of magic undocumented metadata (e.g. the matches_extra field) in the message spec. If you need a magic undocumented back channel, can't the kernel just stuff extra JSON fields into the message which can be ignored by the front-end?

@stevengj
stevengj commented Apr 9, 2014

The object_info_reply in IPEP 24 looks good, if that is what is implemented here.

@stevengj
stevengj commented Apr 9, 2014

The payloads field still seems to be mostly undocumented and hence things like pager are meaningless to me.

@takluyver
Member

@stevengj : Any preference as to whether cursor position is a raw offset in the cell, or a line number & column? I think Min has implemented the latter at present, but we didn't have a strong opinion.

@minrk
Member
minrk commented Apr 9, 2014

if that is what is implemented here.

As discussion proceeds, this PR and the IPEPs may get a bit out of sync, but the IPEPs should be considered the official proposal, and once we settle on things, I will make sure this PR reflects the IPEPs.

The request to consist of just the entire cell and the cursor position (and you need to document whether this is a byte or character offset for UTF8 data... we would prefer the former). e.g. the front-end shouldn't try to break it into lines (as Julia expressions can easily span multiple lines).

This is what the IPEP currently proposes - code is the entire cell, and the cursor is specified with line and column.

I will investigate what makes the most sense with respect to multi-byte encodings. I suspect the former will be preferable for kernels, and the latter will be preferable in javascript, so we will have to be careful.

The reply should consist of (start position, end position, list of replacements). The reason is that we would like to be able to do tab substitution, not just replacement, e.g. replace \alpha with α (see JuliaLang/julia#6340).

Makes sense. Currently, the reply contains matched_text and matches, from which you can infer the replacements, but perhaps it makes more sense to include the cursor position(s) explicitly in the reply, rather than the text. How about this for the reply:

given:

x = \alpha<tab>
012345678901234

request:

{
  "code" : "x = \\alpha",
  "col" : 10,
  "lineno" : 0,
}

reply:

```python
{
  "matches" : [ "α" ],
  "start_col" : 4,
  "end_col" : 10,
  "start_lineno" : 0,
  "end_lineno" : 0,
}

conveying the cursor range to select. If we don't want to allow multi-line replacements, we could have just lineno, col, and length.

I'm not a big fan of magic undocumented metadata

The proposal has been for simple categorical matches, such as files, attributes, arguments. Originally, this was proposed as allowing matches to be a dict. IHaskell does some things to add further hints, for which a simple dict wouldn't help. We could back off from this change if we don't come up with a decent use case.

payloads

arg, yes. payloads are mostly undocumented, and mostly horrible, as well. I will draft some documentation on how they work now. I really don't like how they work, but I haven't figured out a better alternative, yet.

I think Min has implemented the latter at present, but we didn't have a strong opinion.

I implemented both - absolute offset first, then switched to line & col. I mainly did this because it's marginally easier to convert line & col to offset (CodeMirror wants line & col, kernel probably wants offset) than it is to convert the other direction. Plus, I would have to implement the other conversion in javascript, which I always like to avoid. It's not a strong preference, but that's where it is right now.

@stevengj

@takluyver, I think we would prefer a raw offset within the cell (and document whether this is a UTF8 byte offset or a Unicode character offset), since we'll have to compute this anyway if you give us a line number and a column.

("Lines" and "columns" are also somewhat ambiguous in Unicode. There are multiple ways to terminate "lines", and "columns" could mean bytes, characters, or glyphs.)

Raw offsets will also shorten and simplify the messages, since you replace two fields with one.

I see the difficulty with CodeMirror, although it is a bit annoying to complicate the message spec over this. I don't suppose CodeMirror can be patched to accept raw offsets? Or does CodeMirror use an array of lines internally? Why does the conversion need to be done in Javascript?

We can certainly do whatever conversion is necessary in the end, of course, as long as things are sufficiently well documented to resolve the ambiguities above.

@stevengj

@fperez also indicated a slight preference for list of (start,end,replacement) in case different replacements replace different ranges of characters, although I'm not quite sure why since we can always just expand the replacements to the union of the (start,end) ranges.

@minrk
Member
minrk commented Apr 10, 2014

I looked into it, and 'unicode character' is more natural for both the IPython kernel and the js frontend, since both use native unicode strings. @stevengj do you have a preference for UTF8 bytes in Julia? I think converting unicode offset to bytes offset is easier than the other way around, so it probably makes more sense to use the unicode offset.

I don't know what's simpler between (start, end, replacement) and [replacements], start, end. Since there can be many completions and start/end won't vary much, I am inclined to have a single start/end and a list of replacements assuming start/end are the union of all replaced text. This most closely matches the current behavior, as well.

I will switch it back to absolute offset from lineno, col. It's not a complex conversion.

Why does the conversion need to be done in Javascript?

Only because the message spec and CodeMirror have different data structures, so the conversion happens between CodeMirror and messages.

@stevengj

@minrk, Julia has native Unicode strings, but usually stores strings as UTF8, so byte offsets are easiest for us (since byte offsets allow random access in UTF8, whereas character offsets require linear-time access). However, if you give us a character offset, we can easily work with that (converting to a byte offset is a straightforward linear-time operation).

Of course, we will have to convert back from a byte offset to a character offset for the reply message. So, no matter how you do it, conversions in both directions will need to be implemented somewhere. I don't mind doing this on the Julia side, though. We already have the requisite conversion functions in the Julia standard library (JuliaLang/julia#6496).

@stevengj

Speaking of Unicode, you should make a decision about normalization. Python uses NFKC normalization for its identifiers, whereas Julia uses NFC normalization. Any tab-completion code will have to normalize identifiers before deciding on possible matches, and will probably return normalized matches. (Hence, even for "normal" tab completion, the completed string might well replace some of the preceding string.)

Recommendation: IPython should perform no normalization whatsoever of the code input, and just send the unnormalized Unicode data in the completion request. Proper normalization should be up to the kernel. The reply can (and probably will) send normalized replacement text.

Probably this is what you are already doing, but I just wanted to make sure.

@stevengj stevengj referenced this pull request in JuliaLang/julia Apr 10, 2014
Closed

convert byte offsets to/from character offsets #6496

@minrk
Member
minrk commented Apr 10, 2014

That is indeed what we are already doing, but it is good to be clear.

@Carreau
Member
Carreau commented Apr 11, 2014

CodeMirror internally used a tree to store the line, computing it from the raw offset would I suppose be annoying but fast enough.

I disagree with ['matches'],start,end beeing enough,The union of start-end can be empty if one completion append stuff and one other prepend stuff to current token. Especially because I wrote most of the code that deals with completing magics in javascript and having played a lot with completion from different source it is super painfull if you do not have the from-to range.

@stevengj

@Carreau, "union" was the wrong word; "convex hull" would be better. Given intervals [a,b) and [c,d), the convex hull is [min(a,c),max(b,d)).

For example, if one completion wants to prepend (e.g. barfoobar) and another completion wants to append (barbarbaz), then the replaced region would be bar and the matches would be foobar and barbaz.

Note also that I would define the interval as [start,end), i.e. not including end. That way, if the reply just wants to insert something before position p without replacing anything, it can send the interval [p,p).

At the same time, you might recommend that implementations generally replace the whole token that they are completing, even if they are just appending or prepending. That way IPython can display the whole replaced token without having to tokenize by itself (which is hard for IPython to do since it doesn't know the language).

@gibiansky

Thanks for pinging me for feedback; sorry it's taken me a while to take a look, but I really love all these changes. Pretty much all of them seem like good ideas from IHaskell's point of view. I especially like the addition of a priority list for the display_data messages and using the display_data-style API elsewhere.

I had a few questions/concerns, which I've tallied here. My questions about object_info_reply aren't as well thought out as I'd like, so maybe you all have some ideas about how I should be using it and such.

matched_extra:

  • matched_extra is fairly ambiguous about how it's used and how it'll be displayed. Here's what I want to be able to do. I want each autocompletion reply to have in it a few fields:
    • Type signatures for autocompleted items
    • The package name for each item
    • The module name for each item

I also want for each of these to be styled in my own way, so I'd really like to just return HTML associated with each match. Or something like that - basically, I'd like a bit more control over how these things are displayed, and be able to style them (color, font, etc). This could also be accomplished via custom.js or custom.css somehow. For instance, I could provide keys for each of these corresponding to CSS class names, so for each autocomplete response I'd have a dict that has some attributes; e.g. matched_extra would be a list of dicts, where the keys were the CSS classes to apply. So if it were matched_extra = [{"module": "Data.Hello"}], then I would want the text "Data.Hello" to show up in the autocomplete styled with some class along the lines of autocomplete-module which I could stick in custom.css. (This is just an idea for how to implement what I want; I have no idea how this might fit into the overall architecture.)

object_info_reply:

  • I definitely agree about removing the existing structure and leaving it more up to the kernel and using the display_data style interface. I have no real concerns or problems with the new spec, so what follows next is just some questions/thoughts about object_info_request as a whole; it's something I've had some questions and issues dealing with even in the previous version of the spec.
  • Where is object_info_request used other than when you type things like a? or a???
  • How do you decide when object_info_request is used? Can that be modified via custom.js somehow? In particular, Haskell allows the following code:
-- Declare ? and ?? as operators that are just + and *
(?) :: Int -> Int -> Int
a ? b = a + b

(??) :: Int -> Int -> Int
a ?? b = a * b

-- I want a type error or a syntax error, probably?
a ?
b ??

Basically, I'm not sure how object_info_request can work in a language-agnostic way, and it might be nice to have some clarification about how this info is requested. I think it's also called after a '(' in IPython, which IHaskell changes to a space.

  • Also, what is the "name" field of the object? e.g. can I request info about the result of an operation, such as:
a + b? -- using IPython ? syntax, which is a bit strange

Should "name" then be "a + b"?

All of these may be a bit redundant for IHaskell, as I have things like :doc and :hoogle and :info for requesting special types of information about expressions and identifiers.

Thanks for all the hard work, by the way! I'm really happy to see so much focus on including other language kernels and providing a clean API. I'm very glad you guys seem to be willing to tackle and pay off technical debt incurred from previous versions.

@dsblank
Contributor
dsblank commented Apr 16, 2014

Just saw this thread... there are quite a few kernels being developed... might be nice to mention on the mailing list where you are looking for feedback.

Regarding:

{
"protocol_version" : "5.0.0",
"implementation" : "ipython",
"implementation_version" : "3.0.0",
"language" : "python",
"language_version" : "3.4.0",
"banner" : "Python 3.4.0 (/usr/local/bin/python)\nIPython 3.0.0\n",
}

What should happen for those kernels that can change languages? Should the kernel be able to update this information later in a session?

@dsblank
Contributor
dsblank commented Apr 16, 2014

Regarding completion, one of our languages is Scheme, and method info would be given at this point:

(Object.method

In other words, giving function/method details should not be triggered until after space after a open-paren and symbol. Of course, in Scheme, regular syntax has the same format as a function call:

(define (lambda (n) (Object.method arg1 "23"))
@Carreau
Member
Carreau commented Apr 16, 2014

Where is object_info_request used other than when you type things like a? or a???

and

In other words, giving function/method details should not be triggered until after space after a open-paren and symbol. Of course, in Scheme, regular syntax has the same format as a function call:

Object info request/reply are sent on shift-tab, the ? triggering object info request is a kernel side decision IIUC, so you do not need change in message spec to update that.

Will re-read the rest of this dicussion more carefully in the plane in 48h smth, sprinting for now.
Thanks for your feedback !

@minrk
Member
minrk commented Apr 16, 2014

@dsblank sorry about that, I just sent a ping to the list.

@minrk
Member
minrk commented Apr 16, 2014

What should happen for those kernels that can change languages? Should the kernel be able to update this information later in a session?

@dsblank That's a really tricky one. IPython probably isn't going to query this value more than once. I would probably return either 'calico' or whatever the current language is. Figuring out highlighting correctly for that case is probably going to be fairly tricky, and may require a js plugin to work reliably.

@minrk
Member
minrk commented Apr 18, 2014

Changes I am making based on feedback (will update code and IPEPs shortly):

  • Don't add priority to display_data. This can be considered in a separate Issue, but it's unclear whether it's possible to do this correctly.
  • use single integer cursor_pos (absolute unicode position), not column and line number
  • complete_reply will include top-level cursor_start and cursor_end for the range to be replaced
  • don't add matched_extra to complete_reply, instead add a metadata dict for extended data. IPython isn't going to handle this at all, so it will be up to custom js to interpret the extra info (e.g. IHaskell html completion display). I don't think it's feasible to make something generic that IHaskell can use without any custom js.
  • remove namefrom object_info_reply, since it may not be meaningful.
  • rename object_info to inspect, since it's not necessarily inspecting a single object.
@gibiansky

@minrk Those changes sound fine, but I'd definitely want more info on how to use the metadata dict (the one that replaces matched_extra) and what sort of JS library support will exist for using it. Those things should probably be ironed out?

@minrk
Member
minrk commented Apr 18, 2014

@gibiansky My current thought is that there will be no support in IPython itself for adding colors or HTML to completion display. I'm not convinced there is a way to do this that is simple enough while still being useful. Any such logic will be handled by custom js that would override a method on the Completer (I'm not sure which, and the API may need to be changed a little to allow this). The idea behind metadata is that you would write your own information there (e.g. in metadata.ihaskell_extra_data), and it is entirely up to you to write the js that interprets that. With the new KernelSpec, that definition would live in kernel.js.

@gibiansky

@minrk Yup, that sounds good, I just meant that there should be a standard and clear API for doing that via kernel.js and overriding aspects of the Completer. (I've found that some of the client-side API is sometimes fairly difficult to navigate, though that might be my own JS inexperience showing.)

@jdfreder
Member

(I've found that some of the client-side API is sometimes fairly difficult to navigate,

I agree. I'm hoping that when we make the complete transition require.js we will do some cleaning and reorganization.

@minrk
Member
minrk commented Apr 18, 2014

@gibiansky yes, we can think about the specific API for that in a smaller PR. My main concern here is getting the information to the frontend, and what structure the baseline information should have.

An aside: when we switch to using CodeMirror's actual completer API, some aspects of custom display may get a bit harder. I'm not sure about that.

@Carreau
Member
Carreau commented Apr 19, 2014

An aside: when we switch to using CodeMirror's actual completer API, some aspects of custom display may get a bit harder. I'm not sure about that.

Yup, there are some nifty things you can do with codemirror api, like completing with placeholders.
But I think we have to ply with it before adding real support for it.

@jdfreder
Member

@takluyver mentions in the last post of #4983 that msg spec changes are needed to allow the comm msg replies to have a payload tells the front-end to make a new cell. I don't think I see those changes here (I'm not sure I'm looking for them correctly). Is that something you want to add here too or is that something for another PR later?

@jdfreder
Member

Also x-linking #4993

@minrk
Member
minrk commented Apr 25, 2014

Okay, I think this is ready to merge. The docs are updated. A current render of the message spec doc is here

@Carreau
Member
Carreau commented Apr 28, 2014

Maybe dumb question or I missed something, but couldn't shouldn't we add a 'metadata' field to error (ex pyerr) which might be usefull for traceback for example, and as many things already have metadata?

@takluyver takluyver changed the title from [WIP] updating the message spec (finish IPEP 13, 24) to Updating the message spec (finish IPEP 13, 24) May 5, 2014
@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
IPython/core/interactiveshell.py
@@ -773,6 +785,28 @@ def restore_sys_module_state(self):
sys.modules[self._orig_sys_modules_main_name] = self._orig_sys_modules_main_mod
#-------------------------------------------------------------------------
+ # Things related to the banner
+ #-------------------------------------------------------------------------
+
+ def _banner1_changed(self):
+ self.compute_banner()
+
+ def _banner2_changed(self):
+ self.compute_banner()
+
+ def compute_banner(self):
+ self.banner = self.banner1
+ if self.profile and self.profile != 'default':
+ self.banner += '\nIPython profile: %s\n' % self.profile
+ if self.banner2:
+ self.banner += '\n' + self.banner2
@takluyver
takluyver May 7, 2014 Member

Is there any reason to do it like this rather than just defining banner as a property and joining the strings together when it's requested?

@minrk
minrk May 7, 2014 Member

Nope. Just copying old logic. Will make it a property.

@takluyver takluyver commented on the diff May 7, 2014
IPython/core/page.py
@@ -152,6 +152,10 @@ def page(strng, start=0, screen_lines=0, pager_cmd=None):
If no system pager works, the string is sent through a 'dumb pager'
written in python, very simplistic.
"""
+
+ # for compatibility with mime-bundle form:
+ if isinstance(strng, dict):
+ strng = strng['text/plain']
@takluyver
takluyver May 7, 2014 Member

The docstring should probably mention that this works.

@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
IPython/core/release.py
@@ -40,7 +40,8 @@
version_info = (_version_major, _version_minor, _version_patch, _version_extra)
# Change this when incrementing the kernel protocol version
-kernel_protocol_version_info = (4, 1)
+kernel_protocol_version_info = (5, 0, 0)
@takluyver
takluyver May 7, 2014 Member

I'm not convinced we need a 3 part version number for the protocol. And if we one day do, adding another number on the end doesn't break the sort order.

@minrk
minrk May 7, 2014 Member

Fair enough It's 5.0 now.

@takluyver takluyver commented on the diff May 7, 2014
IPython/kernel/channels.py
"""Tab complete text in the kernel's namespace.
Parameters
----------
- text : str
- The text to complete.
- line : str
- The full line of text that is the surrounding context for the
- text to complete.
- cursor_pos : int
- The position of the cursor in the line where the completion was
- requested.
- block : str, optional
- The full block of code in which the completion is being requested.
+ code : str
+ The context in which completion is requested.
+ Can be anything between a variable name and an entire cell.
+ cursor_pos : int, optional
+ The position of the cursor in the block of code where the completion was requested.
@takluyver
takluyver May 7, 2014 Member

What's the default if it's not passed? I guess treating it as if the cursor is at the end of the supplied text makes most sense. If that's right, should the default be -1 instead of 0 to make this clear?

@minrk
minrk May 7, 2014 Member

I'll make the default value None, and convert to len(code), so it's at the end of the block. I don't want to use a negative index.

@takluyver takluyver commented on the diff May 7, 2014
IPython/kernel/channels.py
Parameters
----------
- oname : str
- A string specifying the object name.
+ code : str
+ The context in which info is requested.
+ Can be anything between a variable name and an entire cell.
+ cursor_pos : int, optional
+ The position of the cursor in the block of code where the info was requested.
@takluyver
takluyver May 7, 2014 Member

Same questions about the default here.

@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
IPython/kernel/tests/test_message_spec.py
-class RContent(Reference):
- status = Enum((u'ok', u'error'))
+mime_pat = re.compile(r'\w+/\w+')
@takluyver
takluyver May 7, 2014 Member

Do we want a $ on the end of this so it doesn't match a/b/c? Also, I know at least . is valid in the second part of mimetypes; I'm not sure what other characters may be.

@minrk
minrk May 7, 2014 Member

I'm not too concerned about this one, since anything in this test code will come from our own known formatters, but I made it a bit better, after looking around.

@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
IPython/kernel/tests/test_message_spec.py
+class Version(Unicode):
+ def validate(self, obj, value):
+ min_version = self.default_value
@takluyver
takluyver May 7, 2014 Member

Using the default value as the minimum is somewhat unclear. I'd prefer to make this an explicit piece of metadata, e.g. protocol_version = Version(min='5.0'), even if that takes a little more code.

@minrk
minrk May 7, 2014 Member

Good point. Done.

@takluyver takluyver commented on an outdated diff May 7, 2014
IPython/kernel/zmq/ipkernel.py
+ builtin_mod.input = lambda prompt='': eval(self.raw_input(prompt))
+ self._save_getpass = getpass.getpass
+ getpass.getpass = self.getpass
+
+ def _restore_input(self):
+ """Restore raw_input, getpass"""
+ if py3compat.PY3:
+ builtin_mod.input = self._sys_raw_input
+ else:
+ builtin_mod.raw_input = self._sys_raw_input
+ builtin_mod.input = self._sys_eval_input
+
+ getpass.getpass = self._save_getpass
+
+ def set_parent(self, ident, parent):
+ """Record the parent state
@takluyver
takluyver May 7, 2014 Member

I think this docstring could be a bit clearer.

@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
IPython/kernel/zmq/ipkernel.py
@@ -584,9 +617,9 @@ def apply_request(self, stream, ident, parent):
shell = self.shell
shell.set_parent(parent)
- # pyin_msg = self.session.msg(u'pyin',{u'code':code}, parent=parent)
- # self.iopub_socket.send(pyin_msg)
- # self.session.send(self.iopub_socket, u'pyin', {u'code':code},parent=parent)
+ # execute_input_msg = self.session.msg(u'execute_input',{u'code':code}, parent=parent)
+ # self.iopub_socket.send(execute_input_msg)
+ # self.session.send(self.iopub_socket, u'execute_input', {u'code':code},parent=parent)
@takluyver
takluyver May 7, 2014 Member

Is this worth keeping?

@minrk
minrk May 7, 2014 Member

No, just leftover comments from merging execute_request and apply_request.

@takluyver takluyver commented on an outdated diff May 7, 2014
IPython/qt/console/frontend_widget.py
@@ -515,21 +517,13 @@ def _handle_object_info_reply(self, rep):
# syntax-highlight it ourselves for nicer formatting in the
# calltip.
@takluyver
takluyver May 7, 2014 Member

This comment is now unrelated to the code beneath it, and should be removed.

@takluyver takluyver commented on an outdated diff May 7, 2014
docs/source/development/execution.rst
@@ -0,0 +1,70 @@
+.. _execution_semantics:
+
+Execution semantics in the IPython kernel
+=========================================
+
+The execution of use code consists of the following phases:
+
+1. Fire the ``pre_execute`` event.
+2. Fire the ``pre_run_cell`` event unless silent is True.
+3. Execute the ``code`` field, see below for details.
+4. If execution succeeds, expressions in ``user_expressions`` are computed.
+ This ensures that any error in the expressions don't affect the main code execution.
+5. Fire the post_execute eventCall any method registered with :meth:`register_post_execute`.
@takluyver
takluyver May 7, 2014 Member

Looks like the second part of this should be removed.

@takluyver takluyver commented on an outdated diff May 7, 2014
docs/source/development/execution.rst
+
+The execution of use code consists of the following phases:
+
+1. Fire the ``pre_execute`` event.
+2. Fire the ``pre_run_cell`` event unless silent is True.
+3. Execute the ``code`` field, see below for details.
+4. If execution succeeds, expressions in ``user_expressions`` are computed.
+ This ensures that any error in the expressions don't affect the main code execution.
+5. Fire the post_execute eventCall any method registered with :meth:`register_post_execute`.
+
+.. warning::
+
+ The API for running code before/after the main code block is likely to
+ change soon. Both the ``pre_runcode_hook`` and the
+ :meth:`register_post_execute` are susceptible to modification, as we find a
+ consistent model for both.
@takluyver
takluyver May 7, 2014 Member

This warning is out of date.

@takluyver
takluyver May 7, 2014 Member

But putting in a link to :doc:config/callbacks`` would probably be useful.

@takluyver takluyver and 1 other commented on an outdated diff May 7, 2014
docs/source/development/execution.rst
+ * if the last one is a single line long, run all but the last in 'exec' mode
+ and the very last one in 'single' mode. This makes it easy to type simple
+ expressions at the end to see computed values.
+
+ * if the last one is no more than two lines long, run all but the last in
+ 'exec' mode and the very last one in 'single' mode. This makes it easy to
+ type simple expressions at the end to see computed values. - otherwise
+ (last one is also multiline), run all in 'exec' mode
+
+ * otherwise (last one is also multiline), run all in 'exec' mode as a single
+ unit.
+
+
+Errors in any registered post_execute functions are reported,
+and the failing function is removed from the post_execution set so that it does
+not continue triggering failures.
@takluyver
takluyver May 7, 2014 Member

This bit is also out of date

@minrk
minrk May 7, 2014 Member

What happens when an event handler raises? I didn't think about this behavior when reviewing that PR. Does it get unregistered, or continue to raise errors?

@takluyver
Member

Completed one review pass on the Python and docs changes.

This has merge conflicts, but I assume Min is going to rebase it when review is finished.

@minrk
Member
minrk commented May 7, 2014

Awesome, thanks for the review. I'll go through it tonight or tomorrow.

minrk added some commits Nov 13, 2013
@minrk minrk rev kernel protocol version, and include it in message headers (as a …
…string)
4a6e9af
@minrk minrk kernel_info versions are strings 7fc8e4a
@minrk minrk pyin -> execute_input feb0a3d
@minrk minrk pyout -> execute_result
nbformat and nbconvert are not affected
6800056
@minrk minrk pyerr -> error eb3c0ac
@minrk minrk remove user_variables
leave only user_expressions
e599b65
@minrk minrk remove unused channel definitions from managerabc
they were moved to channelsabc, but the originals were never removed.
3579190
@minrk minrk pass on msgspec doc with updates 09ccdda
@minrk minrk support password in input_request f80d93b
@minrk minrk track parent in kernel
raw_input / getpass are not lambdas, but Kernel methods.
parent state is tracked in Kernel traitlets.
cdfdd10
@minrk minrk password key added to input_request e351532
@minrk minrk fix inprocess input request a33ed60
@minrk minrk handle getpass in terminal frontend 134fbf2
@minrk minrk add utils.tokenutil for getting the token at a cursor offset c21ac70
@minrk minrk add object_inspect_text
in preparation for object_info_request returning formatted mime-bundle
1e7b2d3
@minrk minrk update completion_ and objection_info_request
both requests take:

- code (up to full cell)
- cursor_pos (cursor offset in unicode characters)

and object_info_replies return a mime-bundle, instead of structured data
de473a1
@minrk minrk move banner to base InteractiveShell class 1318a87
@minrk minrk add banner to kernel_info_reply f58c520
@minrk minrk add kernel banner to terminal and qt frontends fa2b8a3
@minrk minrk add implementation and implementation_version to kernel_info_reply 31a7998
@minrk minrk remove `source` key from display_data 51c5b86
@minrk minrk pager payload is a mime-bundle 095e73a
@minrk minrk s/object_info_request/inspect_request 7f1fdd0
@minrk minrk complete_reply has cursor_start and cursor_end, not matched_text c089ced
@minrk minrk move pyout/execute_result and pyerr/error transforms to to/fromJSON
like the mime-type maps
f6285f1
@minrk minrk fix safe_append_output test 00b6f5c
@minrk minrk remove 'name' from inspect_reply 6a91fb6
@minrk minrk msg spec 5.0 797390c
@minrk minrk add style to version changed in html docs
- italic text, like the default
- indent the block
d74976f
@minrk minrk wrap text in pre tags
so that they don't stretch beyond their container
a8d7be6
@minrk minrk make InteractiveShell.banner a property 1787af6
@minrk
Member
minrk commented May 7, 2014

Review comments should be addressed in the last two commits. Rebased, etc.

@Carreau Carreau commented on the diff May 7, 2014
IPython/html/static/notebook/js/outputarea.js
json.ename = content.ename;
json.evalue = content.evalue;
json.traceback = content.traceback;
+ } else {
@Carreau
Carreau May 7, 2014 Member

Still do not want to add metadata field to the error message ?

@minrk
minrk May 7, 2014 Member

Since adding a key is not a breaking change, I think I would prefer to wait until someone has an actual need for this.

@Carreau
Carreau May 7, 2014 Member

Ok, fine with me, but unless it is present, it is hard to do something with it without patching IPython.

@Carreau Carreau commented on the diff May 7, 2014
IPython/html/static/notebook/js/outputarea.js
@@ -850,7 +856,18 @@ var IPython = (function (IPython) {
for (var i=0; i<len; i++) {
data = outputs[i];
var msg_type = data.output_type;
- if (msg_type === "display_data" || msg_type === "pyout") {
+ if (msg_type == "pyout") {
+ // pyout message has been renamed to execute_result,
+ // but the nbformat has not been updated,
+ // so transform back to pyout for json.
+ msg_type = data.output_type = "execute_result";
+ } else if (msg_type == "pyerr") {
+ // pyerr message has been renamed to error,
+ // but the nbformat has not been updated,
+ // so transform back to pyerr for json.
+ msg_type = data.output_type = "error";
+ }
@Carreau
Carreau May 7, 2014 Member

Do you want to log warnings here ?

@minrk
minrk May 7, 2014 Member

What is there to warn about? Error messages are just tracebacks from execution, which get rendered.

@Carreau
Carreau May 7, 2014 Member

I was meaning just console.log that the pyout has been renamed and will be removed in further versions of IPython.

@minrk
minrk May 7, 2014 Member

Ah, no. This is a shim around the notebook format. The transform will happen 100% of the time until we update the nbformat.

@Carreau Carreau and 1 other commented on an outdated diff May 7, 2014
IPython/html/static/notebook/js/tooltip.js
@@ -260,17 +251,17 @@ var IPython = (function (IPython) {
this.reset_tabs_function (cell, text);
}
- // don't do anything if line beggin with '(' or is empty
- if (text === "" || text === "(") {
- return;
- }
+ // don't do anything if line begins with '(' or is empty
+ // if (text === "" || text === "(") {
+ // return;
+ // }
@Carreau
Carreau May 7, 2014 Member

You can totally remove it i suppose.

@minrk
minrk May 7, 2014 Member

removed.

@Carreau Carreau commented on the diff May 7, 2014
docs/source/development/messaging.rst
-- ``user_expressions``: For more complex expressions that require function
- evaluations, a dict can be provided with string keys and arbitrary python
- expressions as values. The return message will contain also a dict with the
- same keys and the :func:`repr()` of the evaluated expressions as value.
-
-With this information, frontends can display any status information they wish
-in the form that best suits each frontend (a status line, a popup, inline for a
-terminal, etc).
+ {
+ 'status' : 'error',
+ 'ename' : 'NameError',
+ 'evalue' : 'foo',
+ 'traceback' : ...
+ }
@Carreau
Carreau May 7, 2014 Member

If you decide to add the metadata field you probably should complete here.

@Carreau
Member
Carreau commented May 7, 2014

A few inline comment, but too much to read at once.

@takluyver
Member

One thing I think you missed from my review (unless you don't think it's worth doing, of course):

  • Update docstring of IPython.core.page.page() to clarify that passing a mimebundle works.
minrk added some commits May 7, 2014
@minrk minrk updates per review
- two-part protocol version (5.0)
- default value for cursor_pos is end of code
- docs, comment, and docstring touchups
c08bdde
@minrk minrk add version key to js message headers 3d99fce
@minrk minrk add shim for double-serialized JSON
msgspec gives unserialized JSON,
nbformat v3 wants JSON strings.
c47fdf5
@minrk
Member
minrk commented May 8, 2014

page docstring updated, thanks.

@takluyver
Member

So if @Carreau is happy, I think this and the adapter PR are about ready.

@minrk
Member
minrk commented May 9, 2014

Shall we throw the switch? If so, I'll rebase #5752 after merging this, so the PR diff looks right.

@takluyver
Member

I'm pushing the button.

@takluyver takluyver merged commit a2329cd into ipython:master May 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@stevengj
stevengj commented May 9, 2014

What version of IPython is this slated for?

What should be the new protocol_version in the kernel_info_request?

I want to make sure that if I connect a new IJulia with the old IPython front-end, or vice versa, then it will break noisily and with a helpful error message. (It's not a problem for you guys, because you always install the Python front-end and back-end together so the versions are matched, but for IJulia mismatches and backwards compatibility are a serious concern.)

When start the kernel, you currently pass a JSON "profile" via the command-line args (e.g. {"key":"b928f9ea-2898-46b1-9036-32eb653cc7d7","transport":"tcp","signature_scheme":"hmac-sha256","ip":"127.0.0.1","hb_port":55758,"control_port":55757,"shell_port":55754,"stdin_port":55756,"iopub_port":55755}), and it would be helpful if this contained some kind of protocol version. That way, I could check for it at startup, and say Please install IPython 3000 (or whatever) if a mismatch is detected.

Or are you going to check the protocol_version in the kernel_info_request on startup and issue a useful error message if a mismatch is detected?

@takluyver
Member

@stevengj : This will be in IPython 3.0. The new protocol version will be 5.0, and it will be added to the header of every message. @minrk has written an adaptor so that our frontends will continue to work with kernels speaking protocol version 4 (i.e. currently all of them apart from the IPython kernel).

Putting the protocol version in the connection file is an interesting possibility, though we'd have to make the interpretation clear, since both ends may be able to speak more than one version of the protocol.

@minrk
Member
minrk commented May 9, 2014

@stevengj I tested PR #5752 with the Julia kernel, so you can actually use current IJulia with IPython master now that these two PRs have been merged, and the v4 messages sent/received by the Julia kernel will be transformed in the notebook server.

Or are you going to check the protocol_version in the kernel_info_request on startup and issue a useful error message if a mismatch is detected?

A kernel_info request is made first, and a translation layer is added if it is detected to be an older version. If IJulia drops support for msg spec 4.x, you can check the message header on a request, and fail with an informative message. Alternately, you could implement the same adaptation logic that I did in #5752.

Putting the protocol version in the connection file is an interesting possibility.

Probably a good idea. As with message headers, it should be appropriate to interpret the absence of a version as the last version to lack the key (IPython 2.0 / msg spec 4.1). I don't know if we want to encode 'all supported protocols' in the connection file, or just the current version. I suspect that just specifying the current version is sufficient.

@minrk minrk deleted the minrk:msgspec5 branch May 9, 2014
@takluyver
Member

I suspect that just specifying the current version is sufficient.

I think if we do that, we should work out protocol negotiation more carefully. E.g. if a protocol 6* client wrote a connection file specifying protocol version 6, but it can actually adapt to protocol version 5, a protocol 5 kernel shouldn't refuse to start.

I started writing some draft guidelines for this, but it quickly got more complex than I had thought. Briefly, however:

  • Protocol flexibility should be implemented in clients, not kernels, wherever possible, because the same message may be broadcast to multiple clients.
  • A kernel should never refuse to start merely because the client prefers a protocol version it doesn't support; the client is responsible for deciding whether communication is feasible.

We have a rather unique situation going from v4 to v5, because I guess v4 clients won't handle v5 kernels gracefully. So kernels may need to do some work this time, but it should be clear that this is a one off.

(* To avoid giving the wrong impression: we have no plans to do a protocol version 6 any time soon, I'm using it as a hypothetical example)

@minad
minad commented Aug 21, 2014

@minrk There are a lot of changes which are relevant for multi-language kernels recently. I would be grateful if these changes are properly reflected in the changelog with reference to appropriate documentation, which makes it easier for us to do the porting.

@minrk
Member
minrk commented Aug 21, 2014

Yup, we are working on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment