Embed code generation from Python #1387

vidartf · 2017-05-24T14:43:09Z

Adds functions to the Python side API for generating embeddable javascript/HTML representation of the widgets. Based on code from ipyvolume.

Resolves #1241.

TODOs

Documentation
Tests
Figure out source of error: Could not create model: Model name: DescriptionStyleModel that appears regularly for full saves.

jasongrout · 2017-05-24T15:15:31Z

I just added that description style model. Happy to help if I can.

vidartf · 2017-05-24T15:17:16Z

Ah, then I'm guessing it is simply the fact that it is not available in the version on the CDN yet.

jasongrout · 2017-05-24T15:31:21Z

Right, it isn't in the cdn yet

…

On Wed, May 24, 2017, 11:17 Vidar Tonaas Fauske ***@***.***> wrote: Ah, then I'm guessing it is simply the fact that it is not available in the version on the CDN yet. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1387 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALwZp2F77MSmJewMDmEPML4UsYBlYiiks5r9En8gaJpZM4NlNm4> .

maartenbreddels · 2017-05-24T15:31:37Z

Excellent work, glad to see this going upstream. I recommend being able to override the embed url for these kinds of things. Also, would be nice if the html template can be passed as argument to embed_minimal_html.

vidartf · 2017-05-24T16:21:18Z

@maartenbreddels : I figured if someone wanted more custom HTML, they could simply use the snippet code, and then wrap it themselves (embed_minimal_html is mostly intended as a dev tool).

Having the url overridable sounds like a good idea.

vidartf · 2017-05-24T16:29:53Z

Regarding the javascript embed parser: Does the embedded widget views need to be specified in dependency order, or does the order not matter?

jasongrout · 2017-05-24T16:33:01Z

Does the embedded widget views need to be specified in dependency order, or does the order not matter?

I'm not quite sure what you mean. What dependencies do you see between views?

maartenbreddels · 2017-05-24T16:34:17Z

Order doesn't matter afaik.

vidartf · 2017-05-24T16:52:43Z

What dependencies do you see between views?

@jasongrout Not entirely sure, but I figured there might be some dependencies e.g. with the layout views. It was mostly catalyzed by the warnings I see for the exported minimal HTML: Style not applied because a parent view does not exist (widget_layout.js:116), although that might be related to the DescriptionStyleModel things(?).

Either way I see that the views need a bit further attention, e.g. for the following case:

import ipywidgets
import ipywidgets.widgets.embed as embed

w1 = ipywidgets.IntSlider(min=0, max=100)
w2 = ipywidgets.IntText()
box = ipywidgets.HBox(children=[w1, w2])

embed.embed_minimal_html('test.html', widgets=box)

Here, we definitely want to save the state of w1 and w2, but I don't think it makes sense to add all three views to the export. We should probably only include the views of the widgets specified to the function?

jasongrout · 2017-05-24T17:21:26Z

Are all three views added? I would think that just one view should be added, i.e., the top-level widget that is displayed.

vidartf · 2017-05-24T17:30:41Z

The last two commits (15bfcb1 + 01f7923) have fixed this, but see the note in the commit message of 01f7923.

maartenbreddels · 2017-05-24T17:41:17Z

Should we have some unittest with it, it doesn't have to do really fancy, but to make sure there is code coverage for everything. Like exporting a single widget, two widgets, and check if a complicated dependency graph is handled correctly?

vidartf · 2017-05-24T17:59:39Z

@maartenbreddels Definitly, it is already in the TODO list on top! It won't happen today though, as I am heading home. If you have an interesting dependency graph to test for, feel free to share!

maartenbreddels · 2017-05-24T19:31:05Z

I see, 👍 on the tests and documentation. If you are 'ready', let me know, I can see if I can totally remove embed.py from ipyvolume and rely on this, that would be nice!

maartenbreddels · 2017-05-29T14:16:04Z

@vidartf just want to make you aware of what's happening in #1394 . It would make sense this PR would also include a get_embed_url() function right? Then we can also use that instead of what I suggested https://github.com/jupyter-widgets/jupyter-sphinx/pull/13/files

vidartf · 2017-05-29T14:36:16Z

@maartenbreddels Yes, there is a # TODO note for that, so that would be very nice 👍

vidartf · 2017-05-29T15:04:25Z

@maartenbreddels: I removed the code to add referring widgets. The comment in the code mentioned this being useful for layouts, but widgets refer to the layouts, not the other way around, so those should already be included. Instead, this would add widgets to the state that didn't really need to be included. The only exception I could find was links, which I made a special case for (any link between widgets included in the state are also included).

Am I missing any obvious cases where this would make sense to keep?

vidartf · 2017-05-30T11:03:47Z

Alright, it should be ready for a full review now. Note that I made the default behavior to include the state of all widgets in order to match the behavior of the notebook interface (and be conservative).

maartenbreddels · 2017-05-30T11:06:12Z

Yes, I doubt that comment made sense, and indeed it's needed make jslink work, but there could be other widgets that act similarly. Say a 3rd party link-like-widget.

SylvainCorlay · 2017-05-30T18:07:50Z

ipywidgets/widgets/embed.py

+    if include_all:
+        state = Widget.get_manager_state(drop_defaults=drop_defaults, widgets=None)['state']
+    else:
+        state = dependency_state(views, drop_defaults)


I don't think that we should try to walk the widgets dependency graph. We had a similar thing at some point to restrict to widgets that were displayed and spent a lot of time tracking corner cases.

Although I really like your implementation with generators :)

There are some cases where you might miss widgets such as a widget that you are not especially looking at that emits events (e.g. a controller widget).

For ipyvolume it can be the difference between a 100mb file and a 10mb file (simply because you've created 10 widgets with 10mb of data). So for ipyvolume it's a must have, but maybe don't do this by default? Or have a argument 'include_all=True/False' orso?

I think we need a much better story about cleaning up widgets. It's hard to do automatic garbage collection with the object split across frontend and backend, though.

@SylvainCorlay How about this:

Replace include_all=True with an argument state=None.

If state is None, include state of all widgets known to manager.

Else if state is a dict, simply use that. This way API consumers can traverse dependencies for their widgets to generate state.

Possibly add some helpers for consumers to build such a state dict on their own?

I like the state=None strategy.

(The issue with the include_all=False it seems to claim that one would always get a functional set of widgets, which we me have missed some that have e.g. a jsdlink to a widget of interest.)

I'd vote for the inverse of this. state is None should be smart (it seems to work fine right?). Otherwise, try explaining to someone that ALL widgets are by default serialized, when you only want the snippet to show my_widget. If for some reason our code fails, we should fix it, and in the meantime you can bypass it by passing state=Widget.get_manager_state().
I don't like regresssion, let's progress! :)

@SylvainCorlay The current code does that actually, if that jsdlink is in Widgets.widgets (and every widget should right), it will find it.

It seems to only be included if both source and target are in store.

While if only the target of the link is in store, the link should be added to the store, as well as the source of the link...

Then we must start over again with the rexursive wall from the source...

I've committed the outlined change, feel free to play around with it if you want to discover corner cases. Some of the most obvious omissions of the automatic dependency finder are now more explicitly stated in the docstring of the dependency_state function.

vidartf · 2017-06-19T09:50:03Z

Alright, I cleaned up the code after the reviews. Given that there will definitely be corner cases that the dependency finder will not find (not without a lot of effort on our part), I think it makes sense to leave the default as simply including all state (at least for this PR).

For now, those confident that the simple dependency walker is sufficient, are free to use that to generate a smaller state. Alternatively, users can build their own custom state, if they have particularly difficult cases to handle, and want to avoid the full state.

PS: Some of the JS tests are failing CI, no idea why.

jasongrout

What do you think about putting the embed file at the top level, instead of inside the widget directory? So it would be ipywidgets.embed instead of ipywidgets.widgets.embed?

docs/source/embedding.md

ipywidgets/widgets/embed.py

jasongrout · 2017-06-22T13:10:26Z

ipywidgets/widgets/embed.py

+
+    if embed_url is None:
+        # TODO: Get widgets npm version automatically:
+        embed_url = u'https://unpkg.com/jupyter-js-widgets@~3.0.0-alpha.0/dist/embed.js'


Perhaps this default should be defined at the top of the module so it's easy to change. Also, it should be updated now that we split the packages up.

Yes, there is a TODO here. Previously there were talk about making a function somewhere to automatically get the right URL. I can put it as a module global if we're not doing that.

Perhaps _version.py is the right place to put the frontend version(s). Right now, we store the widget spec version there and the protocol version. Adding another attribute there for the package versions of @jupyter-widgets/base and @jupyter-widgets/controls could be the natural place.

based on this, I'd say:

embed_url_default = u'https://unpkg.com/jupyter-js-widgets@~%s-%s.%s/dist/embed.js' % (ipywidgets.__jupyter_widget_version__, ...)

Where the ... would need to include alpha.0 somehow, so I guess that also needs to go int _version.py?

Also: What is the current URL to use? I haven't been able to track those changes perfectly.

And how would the two packages work vs an overridden URL? Would an override replace both, or just one of them? Or would we need it to be a dict/sequence now?

(see #1428, and in particular, see the comment #1428 (comment))

(and see #1410)

And how would the two packages work vs an overridden URL? Would an override replace both, or just one of them? Or would we need it to be a dict/sequence now?

Right now there is just the html manager package, which defines the /base and /controls versions, so there is just one include. In the future, I imagine that maybe the html manager does not include the /controls, and that would be specified separately.

I'll wait with this until package renaming is done #1447.

ipywidgets/widgets/embed.py

docs/source/embedding.md

Adds functions for generating embeddable HTML/javascript from the python API.

Note: This should be a pure move, except changes to relative imports.

recommonmark rst eval is not enabled, so fix the formatting

A change was left behind in the editor

jasongrout · 2017-06-22T15:39:49Z

@maartenbreddels - just to keep the license situation simpler, and since you are also a Jupyter widgets contributor - I notice that we quote your entire MIT license in one of the files for some code of yours that we are using. Would it be possible for you to consider the code we are using from ipyvolume as a separate contribution to Jupyter Widgets under the jupyter bsd license, as a Jupyter contributor, and thus just consider it under standard Jupyter contributors copyright?

(It's fine either way, but I'm just trying to simplify the license situation where convenient...)

maartenbreddels · 2017-06-22T16:31:35Z

Yes, I agree, consider all code in this PR as a new contribution or consider the code to be relicensed under bsd.

jupyter-widgets#1387 (comment) "consider all code in this PR as a new contribution or consider the code to be relicensed under bsd."

maartenbreddels · 2017-06-23T11:13:47Z

I'm not sure if there are pieces of the code that are very similar or exact copies of my code, if that is the case, maybe it's better to explicitly put this in the header (# Part of the code is based on ipyvolume, but relicensed by the author (Maarten Breddels) under BSD).

jasongrout · 2017-06-23T11:19:07Z

I'm not sure if there are pieces of the code that are very similar or exact copies of my code, if that is the case, maybe it's better to explicitly put this in the header (# Part of the code is based on ipyvolume, but relicensed by the author (Maarten Breddels) under BSD).

I think it's also important to consider it copyright by Project Jupyter contributors (as stated at the top of the file) - otherwise the attribution requirements require us to cite your BSD license in full. Personally, I think all we need is your permission as documented here to just use the code as-is in these commits. But of course, happy to do as you wish.

maartenbreddels · 2017-06-23T12:01:23Z

I'm fine with that, just that if someone says 'hey, this code is copied!' they don't need to worry.

vidartf · 2017-06-23T14:36:06Z

Is the current notice OK? As far as I can tell that is the only remaining issue here + the CDN url, and I'd love to get this to purple soon :)

maartenbreddels · 2017-06-23T14:41:54Z

I'm fine with it 👍

jasongrout · 2017-06-23T14:59:46Z

@maartenbreddels - I tweaked it to say:

+# Parts of this code is from IPyVolume (24.05.2017), used here under
+# this copyright and license with permission from the author
+# (see #1387)

to make it clearer that the copyright and the license matches the rest of the file. Is that all right with you?

Also copy over the TODO

vidartf · 2017-06-29T13:42:12Z

I've just updated the embed CDN url to the same as what the JS code uses (https://unpkg.com/@jupyter-widgets/html-manager@*/dist/index.js), with the same TODO pointing out that this can be made more robust. With this, I think all outstanding issues commented above should be addressed.

jasongrout · 2017-06-29T20:34:50Z

Thanks!

maartenbreddels mentioned this pull request May 29, 2017

[WIP] Split out embed manager #1394

Merged

5 tasks

vidartf force-pushed the embed branch from 6702524 to ea0554f Compare May 29, 2017 14:57

vidartf changed the title ~~[WIP] Embed code generation from Python~~ Embed code generation from Python May 30, 2017

jasongrout added this to the 7.0 milestone May 30, 2017

SylvainCorlay reviewed May 30, 2017

View reviewed changes

jasongrout mentioned this pull request Jun 2, 2017

table_beautifier in widget #1319

Closed

vidartf force-pushed the embed branch from 8b1ae9c to 6534d5d Compare June 19, 2017 09:20

jasongrout requested changes Jun 22, 2017

View reviewed changes

vidartf added 2 commits June 22, 2017 17:13

Fix uneven indent

dd18105

Add python embedding code

73d0f99

Adds functions for generating embeddable HTML/javascript from the python API.

vidartf added 9 commits June 22, 2017 17:13

Clean up embed interface and docstrings

c71c978

Document python embed functions

bc2d040

Refactor python side embed state specification

41e25d8

Improve embed docs readability

e380596

Adjust docstring language

5b3ed6d

Fix embed code

99cc261

Embed: Minor adjustments according to review

b62cac3

Move embed py module to ipywidgets root

faf8af2

Note: This should be a pure move, except changes to relative imports.

Cleanup embed docs

a1d9033

recommonmark rst eval is not enabled, so fix the formatting

vidartf force-pushed the embed branch from 934a115 to a1d9033 Compare June 22, 2017 15:14

Fix

ea2c563

A change was left behind in the editor

Embed-py: Include import example in docs

5c4d3ee

Relicense code from ipyvolume

0e14be4

jupyter-widgets#1387 (comment) "consider all code in this PR as a new contribution or consider the code to be relicensed under bsd."

Clarify license

09b4da8

Update permission statement.

287eac0

Update the python embed URL to match JS one

ce13271

Also copy over the TODO

jasongrout merged commit 6a28bdf into jupyter-widgets:master Jun 29, 2017

vidartf deleted the embed branch June 30, 2017 10:00

github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 13, 2021

github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2021

Embed code generation from Python #1387

Embed code generation from Python #1387

Conversation

vidartf commented May 24, 2017 • edited Loading

TODOs

jasongrout commented May 24, 2017

vidartf commented May 24, 2017

jasongrout commented May 24, 2017 via email

maartenbreddels commented May 24, 2017 • edited Loading

vidartf commented May 24, 2017 • edited Loading

vidartf commented May 24, 2017

jasongrout commented May 24, 2017

maartenbreddels commented May 24, 2017

vidartf commented May 24, 2017

jasongrout commented May 24, 2017

vidartf commented May 24, 2017

maartenbreddels commented May 24, 2017

vidartf commented May 24, 2017

maartenbreddels commented May 24, 2017

maartenbreddels commented May 29, 2017

vidartf commented May 29, 2017

vidartf commented May 29, 2017

vidartf commented May 30, 2017

maartenbreddels commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vidartf commented Jun 19, 2017

jasongrout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongrout Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasongrout commented Jun 22, 2017

maartenbreddels commented Jun 22, 2017

maartenbreddels commented Jun 23, 2017

jasongrout commented Jun 23, 2017

maartenbreddels commented Jun 23, 2017

vidartf commented Jun 23, 2017

maartenbreddels commented Jun 23, 2017

jasongrout commented Jun 23, 2017 • edited Loading

vidartf commented Jun 29, 2017

jasongrout commented Jun 29, 2017

vidartf commented May 24, 2017 •

edited

Loading

maartenbreddels commented May 24, 2017 •

edited

Loading

vidartf commented May 24, 2017 •

edited

Loading

jasongrout Jun 22, 2017 •

edited

Loading

jasongrout commented Jun 23, 2017 •

edited

Loading