-
Notifications
You must be signed in to change notification settings - Fork 48
[Issue: 169] Frontend hardcodes python3 #211
Changes from all commits
4d02072
de29821
e4cfbd7
d3ec6ca
d215e05
5cb688c
aaf70fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
var $ = require('jquery'); | ||
var Widgets = require('jupyter-js-widgets'); | ||
|
||
var WidgetManager = function(kernel, msgHandler) { | ||
var WidgetManager = function(kernel, kernelname, msgHandler) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so 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) { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, However, we do need the kernel name for the widget manager. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See below. |
||
// Call the base class. | ||
Widgets.ManagerBase.call(this); | ||
|
||
|
@@ -24,17 +24,21 @@ var Widgets = require('jupyter-js-widgets'); | |
// Register the comm target | ||
this.commManager.register_target(this.comm_target_name, this.handle_comm_open.bind(this)); | ||
|
||
// Validate the version requested by the backend. | ||
var validate = (function validate() { | ||
this.validateVersion().then(function(valid) { | ||
if (!valid) { | ||
console.warn('Widget frontend version does not match the backend.'); | ||
} | ||
}).catch(function(err) { | ||
console.error('Could not cross validate the widget frontend and backend versions.', err); | ||
}); | ||
}).bind(this); | ||
validate(); | ||
// Validate the version requested by the backend -- required for ipywidgets | ||
// If kernel name is not defined, default to python3 | ||
kernelname = kernelname.toLowerCase(); | ||
if (!kernelname || kernelname === 'python3' || kernelname === 'python2') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We need it here in this check to run the validation code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.) |
||
var validate = (function validate() { | ||
this.validateVersion().then(function(valid) { | ||
if (!valid) { | ||
console.warn('Widget frontend version does not match the backend.'); | ||
} | ||
}).catch(function(err) { | ||
console.error('Could not cross validate the widget frontend and backend versions.', err); | ||
}); | ||
}).bind(this); | ||
validate(); | ||
} | ||
|
||
this._shimWidgetsLibs(); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
{ | ||
"cells": [ | ||
{ | ||
"cell_type": "code", | ||
"execution_count": 3, | ||
"metadata": { | ||
"collapsed": false, | ||
"urth": { | ||
"dashboard": { | ||
"layout": { | ||
"col": 0, | ||
"height": 4, | ||
"row": 0, | ||
"width": 4 | ||
} | ||
} | ||
} | ||
}, | ||
"outputs": [ | ||
{ | ||
"name": "stdout", | ||
"output_type": "stream", | ||
"text": [ | ||
"Hello World!\n" | ||
] | ||
} | ||
], | ||
"source": [ | ||
"println(\"Hello World!\")" | ||
] | ||
}, | ||
{ | ||
"cell_type": "code", | ||
"execution_count": null, | ||
"metadata": { | ||
"collapsed": true, | ||
"urth": { | ||
"dashboard": { | ||
"hidden": true | ||
} | ||
} | ||
}, | ||
"outputs": [], | ||
"source": [] | ||
} | ||
], | ||
"metadata": { | ||
"kernelspec": { | ||
"display_name": "Apache_Toree", | ||
"language": "", | ||
"name": "apache_toree" | ||
}, | ||
"language_info": { | ||
"name": "scala" | ||
}, | ||
"urth": { | ||
"dashboard": { | ||
"cellMargin": 10, | ||
"defaultCellHeight": 20, | ||
"layout": "grid", | ||
"maxColumns": 12 | ||
} | ||
} | ||
}, | ||
"nbformat": 4, | ||
"nbformat_minor": 0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where work needs to be done:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For first one, it's something like:
RUN /opt/conda/envs/python2/bin/pip install jupyter_declarativewidgets
Check the path (should be in the docker-stacks README) and probably add a version too.