Skip to content
This repository has been archived by the owner on Oct 26, 2019. It is now read-only.

Move to ipywidgets 5.0.0 / jupyter-js-widgets 1.0 #194

Closed
parente opened this issue Apr 20, 2016 · 25 comments · Fixed by #200
Closed

Move to ipywidgets 5.0.0 / jupyter-js-widgets 1.0 #194

parente opened this issue Apr 20, 2016 · 25 comments · Fixed by #200
Assignees
Labels
Milestone

Comments

@parente
Copy link
Member

parente commented Apr 20, 2016

Maybe move to 5.0.0b4 now so that the jump to final is easy. Or wait for the final releases of both.
At least one recent change that we might need to deal with:

@parente parente added this to the 0.5.0 milestone Apr 20, 2016
@parente
Copy link
Member Author

parente commented Apr 20, 2016

@parente
Copy link
Member Author

parente commented Apr 20, 2016

And ... final is out so, time to move.

@parente
Copy link
Member Author

parente commented Apr 20, 2016

Bumping Dockerfile.kernel to install ipywidgets 5.0.0 final and package.json to install 1.0.0 final most things work, but a few oddities:

  1. Labels on sliders in all demos are gone. Need to try this in notebook 4.2 with ipywidgets 5.

screen shot 2016-04-19 at 10 26 52 pm

1. A simple hello world decl widgets demo that prints whatever you type into a text box when you click invoke logs a Comm close error when loaded and does not function. 2. Taxi demo throws the following Python exception. It also happens when I bump the decl widgets install in the Dockerfile.kernel to 0.5.0.dev1
dashboard.js:239 AttributeError : 'NoneType' object has no attribute 'set' 
 ---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-6-0545d7ccebf2> in <module>()
      1 from urth.widgets.widget_channels import channel
----> 2 channel("trips").set("tripOptions", ["trips_1", "trips_2", "trips_3", "trips_4", "trips_5"])

/opt/conda/lib/python3.4/site-packages/urth/widgets/widget_channels.py in set(self, key, value, **kwargs)
     68     def set(self, key, value, **kwargs):
     69         global the_channels
---> 70         the_channels.set(key, value, self.chan, **kwargs)
     71 
     72     def watch(self, key, handler):

AttributeError: 'NoneType' object has no attribute 'set'

@parente
Copy link
Member Author

parente commented Apr 20, 2016

Fixed 2 and 3 by addressing the parameter alignment change in the PR and issue noted in the description. The labels on the sliders are still missing, however.

@parente parente self-assigned this Apr 20, 2016
@parente
Copy link
Member Author

parente commented Apr 20, 2016

And the sliders are not taking the proper default values according to those specified in @interact in the notebook.

@parente
Copy link
Member Author

parente commented Apr 20, 2016

@jdfreder Off-hand, is there anything that changed recently (last 2-3 weeks) that requires us to do something in the widget manager or otherwise to get labels / defaults working from interactive? I vaguely remember some discussion about the defaults. It's a bit odd (but good!) that these appear to be the only things broken.

@parente
Copy link
Member Author

parente commented Apr 20, 2016

@jhpedemonte Feel free to start from the WIP PR as part of #193 and #108 . If you figure out the sliders, we can update my PR or you can just send in your own. I won't be getting back to it for a bit.

@jhpedemonte
Copy link
Collaborator

jhpedemonte commented Apr 20, 2016

Missing labels is due to this change in utils.js. Not sure why though. Still investigating.

@jhpedemonte
Copy link
Collaborator

jhpedemonte commented Apr 21, 2016

My working hypothesis is that the utils.js change is exposing a timing issue.

In the old code, when module_name was 'jupyter-js-widgets', the Promise was immediately resolved synchronously (old line 70). The new code, though, resolves to true at new line 71 and eventually does an asynchronous require (new line 73).

@jhpedemonte
Copy link
Collaborator

I'm stumped. @jdfreder any idea why the require.js change to utils.js would cause ipywidgets to not properly show labels for us?

Our widget manager is very similar to the EmbedManager (I've added some missing bits locally, but that made no difference). Our use of the notebook's OutputArea is based on the (now deprecated) jupyter-js-output-area example, but I don't see anything that needs to majorly change.

Any tips would be greatly appreciated.

@jhpedemonte
Copy link
Collaborator

@SylvainCorlay Any insight on comment above?

@parente
Copy link
Member Author

parente commented Apr 25, 2016

Things we tried on Friday:

  • Upgrading to the latest jupyter-js-notebook OutputArea. Same bad behavior.
  • Forcing the code down the path where requirejsDefined in utils.js to confirm that the original synchronous resolve behavior still works. The old code path works as it did before.
  • Testing the behavior in Jupyter Notebook 4.2 where it works properly. (I forget what we found with respect to load timing here. Need to re-run.)

@parente
Copy link
Member Author

parente commented Apr 26, 2016

@jhpedemonte both dove into this today. We tore everything down to be like the web3 demo in ipywidgets. We got the missing labels to appear by using webpack to build one big monolithic JS file like widgetsextension does for notebook 4.2 and like the web3 demo does. Present theory is that support for async requirejs loads are busted, even though the code is written to handle them.

@jdfreder @SylvainCorlay requirejs support hasn't been completely dropped from ipywidgets, has it?

@jdfreder
Copy link

Hey a @parente and @jhpedemonte,

Sorry for the late response, I'm no longer working on Jupyter full time and my plate is full with other work. Require.js should still work, but it needs to be loaded into the environment globally for ipywidgets to detect it. Have you verified it's not a conflict with the Label widget in Julyter-js-widgets? We renamed latex to label.

@jpedemonte you are right, the behavior is asynchronous once the jupyter-js-widgets package is defined in requirejs.

@parente support for requirejs isn't dropped-far from that actually. The beefed up logic was necessary for static widgets with third party dependencies and the ability to override builtin widgets in other environments, like nteract.

(Sent from phone, sorry if there are typos)

@parente
Copy link
Member Author

parente commented Apr 26, 2016

@jdfreder No problem on the late response. The only solution we've found so far is webpack everything into a single JS file, just like what web3 demo and widgetextension do. Otherwise, it appears that the requirejs calls resolve too late for models to instantiate and receive receive update events from the backend.

We're going to switch to building a single JS file like those projects do to see if we can proceed down that path for now.

@parente parente modified the milestones: 0.5.0, 0.6.0 Apr 26, 2016
@SylvainCorlay
Copy link
Member

SylvainCorlay commented Apr 26, 2016

@parente @jhpedemonte sorry for the late response. I know exactly what you need to do to:

After you have checked that requirejs is on the page, you can do

define('jupyter-js-widgets', function() {
      return require('jupyter-js-widgets');
});

See e.g. : https://github.com/ipython/ipywidgets/blob/master/jupyter-js-widgets/src/embed-webpack.js#L45

@jhpedemonte
Copy link
Collaborator

jhpedemonte commented Apr 26, 2016

@SylvainCorlay Doesn't work for me.

I added that bit of code to the top of dashboard.js. I tried within the existing requirejs block (since we need to request jupyter-js-widgets.js first), as well as above it (with it's own call to fetch the JS file). Neither worked.

Keep in mind that we are building jupyter-js-widgets.js as its own AMD module using webpack. And the jupyter-js-widgets name is already defined as a require.js path.

@SylvainCorlay
Copy link
Member

Sorry, reading the issue again... Do you only have issues with the Label widget?

@parente
Copy link
Member Author

parente commented Apr 26, 2016

@SylvainCorlay Without doing anything special, default values are also ignored since it looks like the model is loading / instantiating too late to handle the update messages about them (see screenshot up above of the lorenz demo).

Putting a breakpoint in loadClass in utils.js to step through what's happening, we're able to affect the timing so that sometimes entire widgets don't even appear on the page.

@jhpedemonte
Copy link
Collaborator

In the end, was able to fix this by using webpack to create a single monolithic file with all of the code (see PR #200).

@parente parente modified the milestones: 0.6.0, 0.5.0 Apr 26, 2016
@parente parente assigned jhpedemonte and unassigned parente Apr 27, 2016
@SylvainCorlay
Copy link
Member

I am still confused about what is going on here - and why you cannot require jupyter-js-widgets dynamically and need to webpack it.

Since there seems to be multiple unrelated issues in this github issue, that would be great if you guys could open an issue upstream in ipywidgets.

During the BayesHack hackathon last week-end, I had some weird race conditions when trying to make an embed manager with a kernel backend (and custom widgets) which I could not figure out during the hackathon, and it might be related to what you are seeing here.

@SylvainCorlay
Copy link
Member

PS @jdfreder @jhpedemonte @parente @lbustelo, I am ok with making a 5.0.1 or 5.1.0 release with the fix in jupyter-widgets/ipywidgets#530. Is there anything else that you think we should fix before doing this.

@parente
Copy link
Member Author

parente commented Apr 27, 2016

would be great if you guys could open an issue upstream in ipywidgets.

We wanted to get something working in order to cut a release compatible with ipywidgets 5, so we went the webpack route. In the end, I think it had the added benefit of cleaning up some of our frontend code. But, yea, we should revisit the requirejs problem with a small example that reproduces the race conditions. Maybe starting with web3 and forking it so that it uses requirejs / AMD?

During the BayesHack hackathon last week-end, I had some weird race conditions when trying to make an embed manager with a kernel backend (and custom widgets) which I could not figure out during the hackathon, and it might be related to what you are seeing here.

Weird race conditions sounds similar. We'll have to see.

I am ok with making a 5.0.1 or 5.1.0 release with the fix in jupyter-widgets/ipywidgets#530.

I think it's fine to cut a release with that fix alone for declarative widgets. No reason to hold it up for a possible async bug that we don't have a solid test case for or know how to fix yet.

@SylvainCorlay
Copy link
Member

I am ok with making a 5.0.1 or 5.1.0 release with the fix in jupyter-widgets/ipywidgets#530.

I think it's fine to cut a release with that fix alone for declarative widgets. No reason to hold it up for a possible async bug that we don't have a solid test case for or know how to fix yet.

OK, I am jumping on a plane, which gives some time to others to react and will push a release when I am in NYC!

@lbustelo
Copy link

So far jupyter-widgets/ipywidgets#530 was the only issue that I could not handle on my end alone. So I'm +1 for a release.

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

Successfully merging a pull request may close this issue.

5 participants