Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

Refactor out reference to IPython.* reference away from behaviors and replace them with Urth.* #141

Closed
lbustelo opened this issue Dec 8, 2015 · 3 comments

Comments

@lbustelo
Copy link
Collaborator

lbustelo commented Dec 8, 2015

In various behaviors we got references to IPython global. Due to potential inconsistencies based on the environment that the notebook code is running (i.e. dashboards), we need a more reliable (injectable) way to set these dependencies.

I suggest that we isolate that into our nb-extension/js/init/init.js and allow it to be injected by the app code.

Most of the code that reference IPython should be under elements/urth-core-behaviors

@jhpedemonte
Copy link
Contributor

Here's the code we currently use on dashboards server to shim declwidgets: https://github.com/jupyter-incubator/dashboards_server/blob/c7909c79ae21930334d78a39dac21d71b192fa52/public/js/widget-manager.js#L177-L208. There are two sections: the first is only called once, on initialization; the latter is called per cell, on execution of code.

The first section (_shimDeclWidgets) sets up a lot of the IPython globals. It makes use of the kernel instance (from jupyter-js-services) and sets the widget_manager property. If we pass in the WidgetManager instance as suggested by #196, the code could also get the kernel instace. But not sure how "correct" that is.

The second sections (_hookupDeclWidgetsCallbacks) is a bit more complicated and much more tailored to the latest jupyter-js-widgets code. Also, the "callbacks" situation has changed (see jupyter-widgets/ipywidgets@c56b85b) -- that code was moved out of the generic ManagerBase and moved to the notebook-specific WidgetManager class. Plus, you'll need to somehow set this up during code execution -- maybe we'll need an API/callback on widget manager?


To start off with, I think the declwidgets init code could do a simple check to see if it's running in the notebook or not:

if (!window.IPython || !window.IPython.notebook) {
    // not in Notebook, setup shims
}

init() could take an optional widget manager instance in order to setup the shims.

My branch with declwidgets changes is here: master...jhpedemonte:StandaloneExperiment and contains some necessary changes to work with latest code (hopefully also backwards compatible):

  • I don't think the change to nb-extension/js/init/init.js is necessary.
  • The change to DeclWidgetModel.js is fairly straight forward, just a new way of extending the base WidgetModel class so it works with other changes they made in the code.
  • The change to jupyter-widget-behavior.html is interesting. They changed the API to new_widget -- not sure if you can pass the empty object and have it work with both old notebook code and the newer jupyter-js-widgets code.

cc @deedubbu @parente

jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 7, 2016
Should allow us to use init.js/DeclWidgetModel.js outside of
Notebook env.

Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 7, 2016
Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 7, 2016
Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
... and move into init().
Fix issue with widget manager instance reference.

Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Should allow us to use init.js/DeclWidgetModel.js outside of
Notebook env.

Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
... and move into init().
Fix issue with widget manager instance reference.

Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 8, 2016
Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 9, 2016
Helps with case where kernel is restarted and a new kernel
instance used.

Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 10, 2016
Change other 'IPython' references to 'Jupyter'

Ref jupyter#141

(c) Copyright IBM Corp. 2016
jhpedemonte added a commit to jhpedemonte/declarativewidgets that referenced this issue Mar 14, 2016
Should allow us to use init.js/DeclWidgetModel.js outside of
Notebook env.
Fix `IPython` object references.
Newer `new_widget` API takes 3rd param.
`init()` should be backwards compat with older API.
Use getter for kernel instance -- helps with case where kernel
is restarted and a new kernel instance used.
Fix tests which weren't properly testing server state.

Ref jupyter#141
Ref jupyter#196

(c) Copyright IBM Corp. 2016
@purdrew
Copy link
Contributor

purdrew commented Mar 14, 2016

Commit 1085b21 now uses dependency injection for the Jupyter definition. However, the code is still assuming certain things to be in that Jupyter object (ie. Jupyter.notebook.kernel, Jupyter.notebook.base_url). @lbustelo Do you think the code changes in 1085b21 is sufficient to close this item?

@lbustelo
Copy link
Collaborator Author

I think so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants