Use dependency injection for Jupyter Notebook deps #251

jhpedemonte · 2016-03-07T23:11:23Z

Should allow us to use init.js/DeclWidgetModel.js outside of Notebook env.

jhpedemonte · 2016-03-07T23:16:31Z

I pulled the Notebook-specific references into nb-extension/js/main.js and inject those dependencies as the config arg to init(). This will allow us to call the same init function in dashboard server and pass along the correct WidgetManager and WidgetModel instances (derived from jupyter-js-widgets instead of the Notebook). main.js should only be called from within the Notebook.

I'm also passing along the IPython dependency, which allows me to check if it is null (which it would be in dashboard server case). If null, we can then do the shimming of IPython object (should be temporary, until those references are cleaned up).

The shims need a bit more work while I try to figure out how to handle events, etc.

cc @lbustelo @parente @deedubbu

parente · 2016-03-07T23:35:31Z

Is this backward compat with the old init api or do we have to update the bundlers and declare their version compatibility. (Better if its backward compat.)

jhpedemonte · 2016-03-08T15:38:40Z

No, not backwards compat, but I can make that work.

parente · 2016-03-08T16:05:08Z

If it's not crazy to make it work, 👍 to keep it backward compat. Will make cross extension compatibility so much easier to reason about.

jhpedemonte · 2016-03-08T16:44:22Z

I'm fine with passing in WidgetManager and WidgetModel to init, but I don't like having to pass in the kernel instance as well. This would be necessary in order for us to setup IPython.notebook.kernel.is_connected(). Also, we need to trigger the kernel_ready.Kernel event somehow.

It seems that those two kernel related issues (as well as some shimming after each execution request) should be handled outside of DeclWidgets, in the dashboard server scripts themselves. This means that the dashboard server will then needs to also setup the IPython namespace, etc.

Plus, trying to get the other toolkits working will likewise require some shimming inside of the dashboard server scripts.

@parente @lbustelo: What do you think?

parente · 2016-03-08T17:23:34Z

I'm fine with collecting all the shims in the dashboard server. I feel like, as the application container, it's responsible for configuring the environment properly for the various toolkits to work properly.

jhpedemonte · 2016-03-08T17:54:25Z

Commit c9ca0f2 removes a few IPython references. But I'm not happy about having both Urth._baseURL (which is based off of IPython.notebook.base_url) and Urth.BASE_URL (which may be equal to _baseURL or something else). Suggestions welcome form DeclWidgets folks.

lbustelo · 2016-03-09T12:50:17Z

Consider that passing the kernel is not enough. In the notebook, the kernel might not be available it might be restated.

Gino B.

On Mar 8, 2016, at 11:54 AM, Javier Pedemonte [email protected] wrote:

Commit c9ca0f2 removes a few IPython references. But I'm not happy about having both Urth._baseURL (which is based off of IPython.notebook.base_url) and Urth.BASE_URL (which may be equal to _baseURL or something else). Suggestions welcome form DeclWidgets folks.

—
Reply to this email directly or view it on GitHub.

jhpedemonte · 2016-03-09T17:47:34Z

@lbustelo What about using a getter for kernel (see commit d46a2e6)? This still keeps the access of IPython in init. Just need to figure out what to do in non-notebook case.

jhpedemonte · 2016-03-09T17:49:29Z

Getting the following test failures with this patch, which I don't see on master:

Installing and starting Selenium server for local browsers
Selenium server running on port 63284
Web server running on port 2000 and serving from /Users/admin/projects/jupyter-incubator/declarativewidgets
chrome 48                Beginning tests via http://localhost:2000/generated-index.html?cli_browser_id=0
404 GET /doesnotexist/doesnotexist.html
chrome 48                ✖ elements/urth-core-import/test/urth-core-import.html » package attribute » should cause POST server request to install the package if href is not found

  expected importHref to have been called exactly once, but it was called 0 times
    <unknown> at               urth-core-import.<anonymous> at urth-core-import.html:87:0
    <unknown> at   urth-core-import.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at   urth-core-import.window.Urth.urth-core-import.Polymer._onLoadError at /elements/urth-core-import/urth-core-import.html:203:0
    <unknown> at               urth-core-import.<anonymous> at /elements/urth-core-import/urth-core-import.html:164:0
    <unknown> at    iron-ajax.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at             iron-ajax.Polymer._handleError at /elements/iron-ajax/iron-ajax.html:480:0

chrome 48                ✖ elements/urth-core-import/test/urth-core-import.html » package attribute » should cause POST server request to install the package if href is not found

  expected importHref to have been called exactly once, but it was called 0 times
    <unknown> at               urth-core-import.<anonymous> at urth-core-import.html:87:0
    <unknown> at   urth-core-import.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at   urth-core-import.window.Urth.urth-core-import.Polymer._onLoadError at /elements/urth-core-import/urth-core-import.html:203:0
    <unknown> at               urth-core-import.<anonymous> at /elements/urth-core-import/urth-core-import.html:164:0
    <unknown> at    iron-ajax.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at             iron-ajax.Polymer._handleError at /elements/iron-ajax/iron-ajax.html:480:0

chrome 48                ✖ elements/urth-core-import/test/urth-core-import.html » importerror » should fire if the href can not be loaded

  expected importHref to have been called exactly once, but it was called 0 times
    <unknown> at               urth-core-import.<anonymous> at urth-core-import.html:126:0
    <unknown> at   urth-core-import.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at   urth-core-import.window.Urth.urth-core-import.Polymer._onLoadError at /elements/urth-core-import/urth-core-import.html:203:0
    <unknown> at               urth-core-import.<anonymous> at /elements/urth-core-import/urth-core-import.html:164:0
    <unknown> at    iron-ajax.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at             iron-ajax.Polymer._handleError at /elements/iron-ajax/iron-ajax.html:480:0

chrome 48                ✖ elements/urth-core-import/test/urth-core-import.html » importerror » should fire if the href can not be loaded

  expected importHref to have been called exactly once, but it was called 0 times
    <unknown> at               urth-core-import.<anonymous> at urth-core-import.html:126:0
    <unknown> at   urth-core-import.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at   urth-core-import.window.Urth.urth-core-import.Polymer._onLoadError at /elements/urth-core-import/urth-core-import.html:203:0
    <unknown> at               urth-core-import.<anonymous> at /elements/urth-core-import/urth-core-import.html:164:0
    <unknown> at    iron-ajax.Polymer.Base._addFeature.fire at /elements/polymer/polymer.html:1264:0
    <unknown> at             iron-ajax.Polymer._handleError at /elements/iron-ajax/iron-ajax.html:480:0

404 GET /dummy/dummy.html
404 GET /elementsNo/iron-ajax/iron-ajax.html
chrome 48                Tests failed: 4 failed tests
Test run ended in failure: 4 failed tests

chrome 48 (208/0/4)                   


4 failed tests

I took a quick look at the test, but nothing sticks out to me as to why it would fail with my changes. Any ideas?

parente · 2016-03-10T16:30:38Z

@deedubbu @lbustelo any insight (or are you guys talking face to face with @jhpedemonte)?

lbustelo · 2016-03-10T21:28:30Z

All I can say is that I tried it locally and I also see it fail. The problem is that the failing tests are a mystery to my how they ever worked. They look like there might be timing issues.

lbustelo · 2016-03-14T18:11:23Z

I've given a quick test of this branch on the notebook server and seems that it all is working. I was able to restart a kernel and reexec cells and everything seems to be working fine.

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 · 2016-03-14T21:53:30Z

Paired with Javier to fix issue that was causing unit test failures. We also improved the tests to make them more reliable in the process. Reviewed changes, ran tests and several notebooks. Looks good. Merging.

Use dependency injection for Jupyter Notebook deps

jhpedemonte changed the title ~~Use dependency injection for Jupyter Notebook deps~~ [WIP] Use dependency injection for Jupyter Notebook deps Mar 8, 2016

jhpedemonte force-pushed the 141-non-notebook-use branch from b288c1c to ee31459 Compare March 8, 2016 21:40

jhpedemonte mentioned this pull request Mar 9, 2016

Load declwidgets init code from bundled dashboard jupyter/dashboards_server#133

Closed

2 tasks

jhpedemonte force-pushed the 141-non-notebook-use branch from 3a142be to 1085b21 Compare March 14, 2016 21:24

jhpedemonte changed the title ~~[WIP] Use dependency injection for Jupyter Notebook deps~~ Use dependency injection for Jupyter Notebook deps Mar 14, 2016

purdrew pushed a commit that referenced this pull request Mar 14, 2016

Merge pull request #251 from jhpedemonte/141-non-notebook-use

7cfcb7f

Use dependency injection for Jupyter Notebook deps

purdrew merged commit 7cfcb7f into jupyter:master Mar 14, 2016

jhpedemonte mentioned this pull request Mar 14, 2016

[Issue 133] Load DeclWidgets init scripts from bundle jupyter/dashboards_server#136

Merged

lbustelo mentioned this pull request Mar 16, 2016

[WIP] Changes necessary to work standalone similar to ipywidgets examples #157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dependency injection for Jupyter Notebook deps #251

Use dependency injection for Jupyter Notebook deps #251

jhpedemonte commented Mar 7, 2016

jhpedemonte commented Mar 7, 2016

parente commented Mar 7, 2016

jhpedemonte commented Mar 8, 2016

parente commented Mar 8, 2016

jhpedemonte commented Mar 8, 2016

parente commented Mar 8, 2016

jhpedemonte commented Mar 8, 2016

lbustelo commented Mar 9, 2016

jhpedemonte commented Mar 9, 2016

jhpedemonte commented Mar 9, 2016

parente commented Mar 10, 2016

lbustelo commented Mar 10, 2016

lbustelo commented Mar 14, 2016

purdrew commented Mar 14, 2016

Use dependency injection for Jupyter Notebook deps #251

Use dependency injection for Jupyter Notebook deps #251

Conversation

jhpedemonte commented Mar 7, 2016

jhpedemonte commented Mar 7, 2016

parente commented Mar 7, 2016

jhpedemonte commented Mar 8, 2016

parente commented Mar 8, 2016

jhpedemonte commented Mar 8, 2016

parente commented Mar 8, 2016

jhpedemonte commented Mar 8, 2016

lbustelo commented Mar 9, 2016

jhpedemonte commented Mar 9, 2016

jhpedemonte commented Mar 9, 2016

parente commented Mar 10, 2016

lbustelo commented Mar 10, 2016

lbustelo commented Mar 14, 2016

purdrew commented Mar 14, 2016