Decouple the notebook from the widget manager. #151
Conversation
|
@rgbkrk is interested in this |
|
Sweet, thanks for the ping. I'll watch this closely, use it in the notebook, and review where I can. |
jdfreder
changed the title
|
From Jon in chat:
|
|
@rgbkrk @SylvainCorlay this is ready for review : ) |
|
|
| }).catch(utils.reject('Could not set widget manager state.', true)); | ||
| }; | ||
|
|
||
| WidgetManager.prototype._get_connected_kernel = function() { | ||
| ManagerBase.prototype._create_comm = function(comm_target_name, model_id, metadata) { | ||
| throw new Error("Manager ._create_comm not implemented"); |
There was a problem hiding this comment.
I'm assuming you didn't mean to put a space after Manager and before the . here or below.
|
Is it too much to ask that since you created new classes and functions that you could write (at the very least) some small |
|
@rgbkrk I just tried, and mocha tests in this PR are a no-go because the widgets use AMD. I do not object to node-ifying the widgets in another PR and then adding mocha tests to that. |
| WidgetManager.prototype.create_model = function (options) { | ||
| console.warn('WidgetManager.create_model is deprecated. Use WidgetManager.new_widget'); | ||
| ManagerBase.prototype.create_model = function (options) { | ||
| console.warn('ManagerBase.create_model is deprecated. Use ManagerBase.new_widget'); |
There was a problem hiding this comment.
Could we move this to WidgetManager, so that we don't carry deprecated classes to the cleaner future?
|
A big 👍 to this. @ChakriCherukuri could be interested to look at this as well. |
|
I was looking into what needs to be done to build a shiny clone (like a notebook, which is executed in the backend and only the output is sent to the frontend -> static content and widgets; only widgets can communicate with the backend via the widget protocol) and one thing I found that the widgets have / need direct access to the kernel. IMO this would prevent such a usage, as this means that a potential public interface can (via the browser js console) submit python code to execute on the kernel. Is it planned to also decouple the kernel connection from the widgets in this PR? |
|
Hi @JanSchulz , This PR decouples the notebook, kernel, etc from the WidgetManager. My next step, in another PR, is to do the same for the Widget base class. Once that's done, all that is required to display widgets outside of the notebook is an implementation of the ManagerBase class and a dummy comm. I did this work a couple of times in the past, never to be merged. You can see the last go, almost a year and a half ago here. |
|
Currently running on this branch and things seem to work. |
|
Awesome, well it's ready to merge when you are. I'd be happy to continue my decoupling in more PRs, to follow. |
|
Awesome! Once this is shipped if there's more decoupling, I'll try it out in jupyter-sidecar. /cc @parente |
|
Basic question... If you take the Comm out of the widget manager... what does it do then? From the perspective of jupyter-incubator/declarativewidgets we've identified WidgetManager as a class that is core to interactive widgets. What are the use case that is requiring both Comm and Notebook to be decoupled? |
|
|
Thanks @rgbkrk! I think many of these PRs need to start with some documentation of WHY. It is hard to keep up and almost feel like unnecessary effort until the dust settles. |
|
@lbustelo sorry, in the next PR I will try to better explain the "why". In the following PRs I'll embrace duck typing for the comm. |
|
Thank you @lbustelo for asking! I definitely don't want to hide any of this away. |
|
hop! |
Decouple the notebook from the widget manager.
|
🎉 |
|
Can't wait to be able to use pythreejs and bqplot in other front-ends like the sidecar. |
github-actions
bot
added
the
resolved-locked
Separate the parts of the widget manager than need comm_manager and notebook instances to work from the base. Allows people to write there own widget managers, without a notebook or comm_manager.