Jupyter v5 protocol by SpencerPark · Pull Request #70 · gopherdata/gophernotes

SpencerPark · 2017-08-26T04:05:26Z

Communicate with front-ends using the current Jupyter messaging spec.

See http://jupyter-client.readthedocs.io/en/latest/messaging.html

sbinet

not an expert wrt Jupyter protocol (including v5 :P)
thus, just a superficial review...

sbinet · 2017-08-28T20:13:06Z

 import (
 	"fmt"
-	"go/token"
+


please remove that blank line (we group stdlib imports together)

sbinet · 2017-08-28T20:15:18Z

 	"log"
 	"os"

+	"runtime"


please keep the stdlib imports together

dwhitena · 2017-08-30T13:39:45Z

@SpencerPark This is great. I will review asap, and thank you so much for the contribution. FYI, I am working in the version-1 branch on a pretty major refactor that takes out the dependence on my hacky REPL package and uses gomacro. That being said, these protocol changes are likely valid there as well. I think we can maybe integrate them as part of the refactor to get compatibility with the latest Jupyter versions. I will look into that while I review. Any related comments?

dwhitena · 2017-08-30T13:54:39Z

@SpencerPark This adds some great improvements. I would like to integrate this into the version 1 refactor if that is ok with you? I would really love to focus on that, as it will be a huge improvement in general. Do you think you could re-arrange this to merge into that branch?

I don't want to burden you too much, so I can help with the re-arranging if you like based on this PR. Just let me know your thoughts? We can always merge this into master prior to the version-1 branch, but I'm hoping to release v1 soon.

SpencerPark · 2017-08-30T14:26:39Z

@dwhitena using a less hacky interpreter sounds fantastic! I'm still new to Go so I don't have much to offer with respect to comments on libraries. I would really like to help get v1 out as it looks to be quite the improvement.

I don't mind integrating this into the v1 branch but I'll have to see about how to actually switch up the PR to do that. Should it just be a separate one or can I salvage this one?

dwhitena · 2017-08-30T14:31:44Z

That would be awesome @SpencerPark! I think you could do this: https://github.com/blog/2224-change-the-base-branch-of-a-pull-request.

It's so great to have you as a contributor, and I can't wait to integrate your changes.

(Also, we would love to have you in the discussion at #data-science in Gophers slack. Although happy to interact here as well. Feel free to reach out if I can offer any help in getting plugged into the Go data community in general.)

communication.

dwhitena

This is awesome!!! Thank you so much. Some huge improvements here overall. Most of my comments are small. The main things that need to be addressed are the handling of errors for Publishing messages.

dwhitena · 2017-09-06T17:10:13Z

 	ExecutionState string `json:"execution_state"`
 }

+// KernelLanguageInfo holds information about the language that this kernel executes code in


Comment should be a complete sentence (i.e., including periods etc.). This is because these comments turn into the GoDocs. Minor thing, but should be consistent throughout.

dwhitena · 2017-09-06T17:10:26Z

+	NBConvertExporter string `json:"nbconvert_exporter"`
+}
+
+// HelpLink stores data to be displayed in the help menu of the notebook


Period in Comment.

dwhitena · 2017-09-06T17:12:10Z

 		ExecCounter++
 	}

+	// Prepare the map that will hold the reply content


Period. I will stop commenting on these, but you get the idea. I would just scrub the comments.

dwhitena · 2017-09-06T17:15:19Z


+	// Tell the front-end that the kernel is working and when finished notify the
+	// front-end that the kernel is idle again
+	receipt.PublishKernelBusy()


I might prefer to just put the Publish method inline here for busy, idle, etc. messages. This could be a one line call and would avoid hiding what's actually happening in these methods. Also, I would handle the error here. Looks like the error from Publish isn't handled in these methods.

Also, many of these publish messages (busy, idle, etc.) appear to be identical minus the actual status. Could you just pass in the status in as an argument and not duplicate the methods. You could just have PublishKernelStatus().

The reasoning behind hiding the message being sent is to encapsulate the details of the protocol inside messages.go. This should make it easier to maintain when Jupyter updates again in the future.

I agree with the second comment, I can clean up the kernel status messages.

I guess I could see that separation of concerns. Good point.

dwhitena · 2017-09-06T17:15:28Z

+	// Tell the front-end that the kernel is working and when finished notify the
+	// front-end that the kernel is idle again
+	receipt.PublishKernelBusy()
+	defer receipt.PublishKernelIdle()


See comment above.

dwhitena · 2017-09-06T17:27:55Z

+
+// PublishExecuteResult publishes a serialized error that was encountered during execution.
+func (receipt *msgReceipt) PublishExecutionError(err string, trace string) {
+	receipt.Publish("error",


dwhitena · 2017-09-06T17:28:08Z

+	)
+}
+
+// WriteStreamData holds data to be written to a stream (stdout, stderr)


Period in comment.

dwhitena · 2017-09-06T17:28:14Z

+
+// PublishWriteStdOut publishes the data string to the front-end's stdout
+func (receipt *msgReceipt) PublishWriteStdOut(data string) {
+	receipt.Publish("stream",


dwhitena · 2017-09-06T17:28:19Z

+
+// PublishWriteStdErr publishes the data string to the front-end's stderr
+func (receipt *msgReceipt) PublishWriteStdErr(data string) {
+	receipt.Publish("stream",


dwhitena · 2017-09-06T17:28:31Z

+}
+
+// WriteStreamData holds data to be written to a stream (stdout, stderr)
+type WriteStreamData struct {


See comment above on types.

fix godoc comment formatting.

dwhitena · 2017-09-11T23:49:30Z

I'm getting a bad kernel info error when starting a Go kernel notebook:

➜  gophernotes git:(SpencerPark-jupyter-v5-protocol) ✗ jupyter notebook --version
5.0.0
➜  gophernotes git:(SpencerPark-jupyter-v5-protocol) ✗ jupyter notebook
[I 19:21:24.689 NotebookApp] Serving notebooks from local directory: /home/dwhitena/go/src/github.com/gopherdata/gophernotes
[I 19:21:24.689 NotebookApp] 0 active kernels 
[I 19:21:24.689 NotebookApp] The Jupyter Notebook is running at: http://localhost:8888/?token=537bc04944a98fb763de6d965e92ea3de0ee7f98112a2bca
[I 19:21:24.689 NotebookApp] Use Control-C to stop this server and shut down all kernels (twice to skip confirmation).
[C 19:21:24.690 NotebookApp] 
    
    Copy/paste this URL into your browser when you connect for the first time,
    to login with a token:
        http://localhost:8888/?token=537bc04944a98fb763de6d965e92ea3de0ee7f98112a2bca
[4465:4500:0911/192125.040923:ERROR:browser_gpu_channel_host_factory.cc(103)] Failed to launch GPU process.
Created new window in existing browser session.
[I 19:21:25.056 NotebookApp] Accepting one-time-token-authenticated connection from 127.0.0.1
[I 19:21:28.435 NotebookApp] Creating new notebook in 
[W 19:21:28.806 NotebookApp] 404 GET /nbextensions/widgets/notebook/js/extension.js?v=20170911192124 (127.0.0.1) 11.39ms referer=http://localhost:8888/notebooks/Untitled2.ipynb?kernel_name=gophernotes
[I 19:21:28.870 NotebookApp] Kernel started: 5b4f1864-42cc-4ef2-9502-5518b58d93a6
[E 19:21:28.905 NotebookApp] Bad kernel_info reply
    Traceback (most recent call last):
      File "/usr/local/lib/python2.7/dist-packages/notebook/services/kernels/handlers.py", line 166, in _handle_kernel_info_reply
        msg = self.session.deserialize(msg)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/session.py", line 937, in deserialize
        return adapt(message)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 398, in adapt
        return adapter(msg)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 96, in __call__
        return handler(msg)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 246, in kernel_info_reply
        if content['language'].startswith('python') and 'ipython_version' in content:
    KeyError: 'language'
[E 19:21:28.918 NotebookApp] Uncaught exception GET /api/kernels/5b4f1864-42cc-4ef2-9502-5518b58d93a6/channels?session_id=4D886C098FC042758E9008462D9830C3 (127.0.0.1)
    HTTPServerRequest(protocol='http', host='localhost:8888', method='GET', uri='/api/kernels/5b4f1864-42cc-4ef2-9502-5518b58d93a6/channels?session_id=4D886C098FC042758E9008462D9830C3', version='HTTP/1.1', remote_ip='127.0.0.1', headers={'Origin': 'http://localhost:8888', 'Upgrade': 'websocket', 'Accept-Language': 'en-US,en;q=0.8', 'Accept-Encoding': 'gzip, deflate, br', 'Sec-Websocket-Version': '13', 'Host': 'localhost:8888', 'Sec-Websocket-Key': 'hoYIhxFfpmi1nbhZ7Z5law==', 'User-Agent': 'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36', 'Connection': 'Upgrade', 'Cookie': '_ga=GA1.1.830181016.1500129053; _xsrf=2|c4c37db4|c93dfba3dfb9d6a3e59e7cae4056a40a|1504106788; username-localhost-8888="2|1:0|10:1505172085|23:username-localhost-8888|44:NjBlMjk3OGEyZGJhNDM4NWExYmZjYzQ3NTA3M2I3Njk=|fc0556792d50af43096a82598b1e942af9ce9e26d2d2304480eb253829c13050"', 'Pragma': 'no-cache', 'Cache-Control': 'no-cache', 'Sec-Websocket-Extensions': 'permessage-deflate; client_max_window_bits'})
    Traceback (most recent call last):
      File "/usr/local/lib/python2.7/dist-packages/tornado/web.py", line 1467, in _stack_context_handle_exception
        raise_exc_info((type, value, traceback))
      File "/usr/local/lib/python2.7/dist-packages/tornado/stack_context.py", line 316, in wrapped
        ret = fn(*args, **kwargs)
      File "/usr/local/lib/python2.7/dist-packages/zmq/eventloop/zmqstream.py", line 191, in <lambda>
        self.on_recv(lambda msg: callback(self, msg), copy=copy)
      File "/usr/local/lib/python2.7/dist-packages/notebook/services/kernels/handlers.py", line 299, in _on_zmq_reply
        msg = self.session.deserialize(fed_msg_list)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/session.py", line 937, in deserialize
        return adapt(message)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 398, in adapt
        return adapter(msg)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 96, in __call__
        return handler(msg)
      File "/usr/local/lib/python2.7/dist-packages/jupyter_client/adapter.py", line 246, in kernel_info_reply
        if content['language'].startswith('python') and 'ipython_version' in content:
    KeyError: 'language'

Any ideas @SpencerPark?

dwhitena · 2017-09-12T12:14:05Z

On a super positive note. This allows gophernotes to work with nteract!!!

dwhitena · 2017-09-12T12:48:21Z

@SpencerPark My old Python is likely the problem:

Jupyter installation requires Python 3.3 or greater, or Python 2.7. IPython 1.x, which included the parts that later became Jupyter, was the last version to support Python 3.2 and 2.6.

from http://jupyter.readthedocs.io/en/latest/install.html

SpencerPark · 2017-09-12T14:27:16Z

The absence of the version field in the header was the reason the v5 adapter wasn't triggered on installations where v5 was a future protocol version.

When starting the kernel a log message should appear like the following

[I 10:10:01.551 NotebookApp] Adapting to protocol v5.0 for kernel d659d40f-7872-48ff-9242-10d07917d605

While adding the field I noticed the date timestamp was also missing and even though is officially required in 5.1, is also unofficially in previous versions. See jupyter general message format

dwhitena

Just a few nitpicks and we are good to merge! Awesome work.

dwhitena · 2017-09-12T14:25:59Z

+	return receipt.SendResponse(receipt.Sockets.ShellSocket, msg)
+}
+
+// MIMEDataBundle holds data that can be presented in multiple formats. The keys are MIME types


Let's move this and any other types up to the top of messages.go, with all the funcs below.

dwhitena · 2017-09-12T14:27:12Z

+	StreamStderr        = "stderr"
+)
+
+// PublishWriteStream prints the data string to a stream on the front-end. This is


Is this function or the above constants and types used at this point? If not, let's remove and add them when needed.

dwhitena · 2017-09-12T14:31:43Z

+
+type KernelStatus string
+
+const (


I suggest the following minor changes regarding these constants and the PublishKernelStatus function:

Let's let PublishKernelStatus accept a string rather than the KernelStatus type. Not sure that the type buys us a lot, unless you have something specific in mind. It doesn't prevent other devs from defining a new KernelStatus that we didn't anticipate, and we are only going to use the constant statuses.

We can then remove the KernelStatus type and move the const (...) declarations to the top of kernel.go, because they are really relevant to the kernel itself, not the messaging.

Lastly, I think we can make the status constants unexported.

dwhitena · 2017-09-12T14:32:04Z

+// MIMEDataBundle holds data that can be presented in multiple formats. The keys are MIME types
+// and the values are the data formatted with respect to it's MIME type. All bundle should contain
+// at least a "text/plain" representation with a string value.
+type MIMEDataBundle map[string]interface{}


I think we can make this type unexported right?

dwhitena · 2017-09-12T21:20:35Z

This works for me now without error! LGTM. Merging.

SpencerPark mentioned this pull request Aug 26, 2017

Major bug fixes and meet the most recent messaging protocol for execution #69

Closed

sbinet reviewed Aug 28, 2017

View reviewed changes

SpencerPark force-pushed the jupyter-v5-protocol branch from 2edaa6b to 5e1f16d Compare August 31, 2017 02:10

SpencerPark added 2 commits August 30, 2017 22:21

Wrap common message construction patterns to clean up the jupyter

0e04bea

communication.

Update messages to the 5.0 spec and encapsulate.

825aefd

SpencerPark force-pushed the jupyter-v5-protocol branch from 5e1f16d to 825aefd Compare August 31, 2017 02:24

SpencerPark changed the base branch from master to version-1 August 31, 2017 02:24

dwhitena requested changes Sep 6, 2017

View reviewed changes

Handle errors from publish messages, clean up protocol wrappers, and

e287c99

fix godoc comment formatting.

Add 'date' and required 'version' field to the message header

1d4547e

dwhitena requested changes Sep 12, 2017

View reviewed changes

Remove unused types/methods and clean up the messages interface.

f75e44f

dwhitena approved these changes Sep 12, 2017

View reviewed changes

dwhitena merged commit b8090e3 into gopherdata:version-1 Sep 12, 2017

Uh oh!

Conversation

SpencerPark commented Aug 26, 2017

Uh oh!

sbinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwhitena commented Aug 30, 2017

Uh oh!

dwhitena commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpencerPark commented Aug 30, 2017

Uh oh!

dwhitena commented Aug 30, 2017

Uh oh!

dwhitena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwhitena commented Sep 11, 2017

Uh oh!

dwhitena commented Sep 12, 2017

Uh oh!

dwhitena commented Sep 12, 2017

Uh oh!

SpencerPark commented Sep 12, 2017

Uh oh!

dwhitena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwhitena commented Sep 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dwhitena commented Aug 30, 2017 •

edited

Loading