[Issue: 169] Frontend hardcodes python3

[jameslmartin]

This resolves the language of the kernel before sending the POST request to the kernel gateway (#169) . Though the request body is successfully set to the correct language, no activity is observed over the WebSocket channels.

[jhpedemonte]

I know this is WIP, so I won't have full review comments, but I just want to say that you should adhere to the coding style already in the file:

1. Indentation is 4 spaces (not 2).
2. Add appropriate spaces around keywords (i.e. } else {).

[parente]

@jameslmartin Focus on trying Toree next since the IRKernel fix might not be released for some time. I know for a fact that Toree works through jupyter-js-services and kernel gateway since it's one of our demos for the kernel gateway project. If you don't see it working here with a simple print, then something else is wrong.

Other than code style review like @jhpedemonte gave above, we're holding off on merging this until we see it work with at least one other kernel beyond Python.

[parente]

I see you fixed Toree in your last commit. Cool.

I think this just needs a rebase on master, a fix of the style @jhpedemonte pointed out, and the inclusion of one really simple Scala notebook for manual test and we can send it in.

[parente]

@jameslmartin and @jhpedemonte will pair today to see where this stands and how to get it in for 0.6.0

[jhpedemonte]

Paired with @jameslmartin to finish this up. Found a couple of things:

1. Kept getting validation error for ipywidgets code. We decided to expose the kernel name in the HTML template and only run the validation for python kernels.
2. Ran across the Missing property 'buffers' error. This happens when the Scala kernel craps out due to insufficient memory. Looks like the kernel gateway tries to restart it and sends back websocket messages with execution_state: "restarting". However, those don't have a buffer property, which the jupyter-js-services code expects (even if empty). Kernel gateway bug?

[jhpedemonte]

IKernel (type of kernel) has a name property, but it always seems to come back as empty string in my testing.

[jhpedemonte]

OK, so kernel.name is an empty string because we pass an empty string now when calling startNewKernel. Instead, I think we should do this:

diff --git a/client/js/dashboard.js b/client/js/dashboard.js
index fec0656..c88d37e 100644
--- a/client/js/dashboard.js
+++ b/client/js/dashboard.js
@@ -50,7 +50,7 @@ if (Element && !Element.prototype.matches) {
     var renderMime = _createRenderMime();

     // start a kernel
-    Kernel.start().then(function(kernel) {
+    Kernel.start(Config.kernelname).then(function(kernel) {
         // do some additional shimming
         _setKernelShims(kernel);

@@ -58,7 +58,7 @@ if (Element && !Element.prototype.matches) {
         _registerKernelErrorHandler(kernel);

         // initialize an ipywidgets manager
-        var widgetManager = new WidgetManager(kernel, Config.kernelname, _consumeMessage);
+        var widgetManager = new WidgetManager(kernel, _consumeMessage);

         // initialize Declarative Widgets library
         var widgetsReady = _initDeclWidgets();
diff --git a/client/js/kernel.js b/client/js/kernel.js
index 4444210..a15135a 100644
--- a/client/js/kernel.js
+++ b/client/js/kernel.js
@@ -18,14 +18,14 @@ var Services = require('jupyter-js-services');

     var _kernel;

-    function _startKernel() {
+    function _startKernel(kernelname) {
         var loc = window.location;
         var kernelUrl = loc.protocol + '//' + loc.host;
         var clientId = _uuid();
         var kernelOptions = {
             baseUrl: kernelUrl,
             wsUrl: kernelUrl.replace(/^http/, 'ws'),
-            name: '', // Set by API when making POST request to kernel gateway
+            name: kernelname,
             clientId: clientId,
             ajaxSettings: {
                 requestHeaders: {
diff --git a/client/js/widget-manager.js b/client/js/widget-manager.js
index 278b2e8..d3a717c 100644
--- a/client/js/widget-manager.js
+++ b/client/js/widget-manager.js
@@ -10,7 +10,7 @@
 var $ = require('jquery');
 var Widgets = require('jupyter-js-widgets');

-    var WidgetManager = function(kernel, kernelname, msgHandler) {
+    var WidgetManager = function(kernel, msgHandler) {
         //  Call the base class.
         Widgets.ManagerBase.call(this);

@@ -26,7 +26,7 @@ var Widgets = require('jupyter-js-widgets');

         // Validate the version requested by the backend -- required for ipywidgets
         // If kernel name is not defined, default to python3
-        kernelname = kernelname.toLowerCase();
+        var kernelname = kernel.name.toLowerCase();
         if (!kernelname || kernelname === 'python3' || kernelname === 'python2') {
             var validate = (function validate() {
                 this.validateVersion().then(function(valid) {

[jameslmartin]

I'm not sure if this is what we want to do here. The original bug was that the front end was hardcoding 'python3' into that field, kernel.name. @parente and I discussed that the back end should determine what type of kernel should be started, rather having the front end pass along the name.

However, we do need the kernel name for the widget manager. Thoughts?

[parente]

See below.

[jhpedemonte]

Notebooks (i.e. test_widget_overlap) which specify python2 as kernel don't work correctly. We get these errors:

Error: Object 'ipython.widget' not found in registry

And I see this from the kernel gateway:

[IPKernelApp] ERROR | No such comm target registered: jupyter.widget.version

Running these notebooks in Notebook works just fine. Editing notebook to use python3 instead makes them work correctly.

[parente]

Help me disentangle.

Notebooks (i.e. test_widget_overlap) which specify python2 as kernel don't work correctly.

The kernel gateway dev Docker image needs to install ipywidgets against the python2 environment.

1. Kept getting validation error for ipywidgets code. We decided to expose the kernel name in the HTML template and only run the validation for python kernels.

Was this only for python2? If so, see above. If it's for Toree too, are the necessary components for decl widgets installed in the kernel gateway container as well?

@parente and I discussed that the back end should determine what type of kernel should be started, rather having the front end pass along the name.

The frontend will never know the language of the dashboard unless its encoded in the HTML returned by the server. Even then, if that language is passed back to the server, it can't be trusted since the client can be manipulated.

Why exactly does the widget manager need to know the language?

1. Ran across the Missing property 'buffers' error. This happens when the Scala kernel craps out due to insufficient memory. Looks like the kernel gateway tries to restart it and sends back websocket messages with execution_state: "restarting". However, those don't have a buffer property, which the jupyter-js-services code expects (even if empty). Kernel gateway bug?

Kernel gateway is using the restart handler from notebook as-is. So doubtful. This is a separate issue about OOM handling, so let's put it aside for now.

[parente]

You'll also have to include just "python" here, but I really suspect the Python check won't be required once the other issues are resolved (e.g., properly installing the decl widgets scala jar, installing ipywidgets against python2 conda env) .

[jameslmartin]

Why exactly does the widget manager need to know the language?

We need it here in this check to run the validation code.

[parente]

Right. I still think once Scala is setup properly, that special casing for Python will not be needed. I'll get with you and or @jhpedemonte today.

[parente]

Discussed with @jhpedemonte . For now, we're not going to bother setting up decl widgets for scala since the example is only hello world. Adding whatever config is necessary for a scala decl widgets example to work can come as part of #203.

[parente]

@jhpedemonte also clarified that the validation check does not actually inhibit the Toree kernel from working. It's just an ugly error. We agreed that switching it to a warning that explicitly says ala "Compatible version of ipywidgets is not available" is cleaner than hardcoding python checks and still makes sense with other kernels (yes, ipywidgets is not available for R, Toree, etc.)

[jhpedemonte]

One reason we need the kernel name on the client side is because it does the call to startNewKernel for jupyter-js-services. The Kernel instance returned uses this passed in kernel name, not what is returned from backend (yay!). If we don't specify a kernel name to startNewKernel, then kernel.name is an empty string.

[parente]

One reason we need the kernel name on the client side is because it does the call to startNewKernel for jupyter-js-services.

I'm fine including the kernel name in the template like you're doing as long as we put as many guards as we can in place to make sure that the kernel name determined on the server-side is the only one that's trusted and sent to the kernel gateway.

If there's an issue or question to open about how the name is used in jupyter-js-services, please go ahead and report it.

[jhpedemonte]

Superseded by #236