-
Notifications
You must be signed in to change notification settings - Fork 949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: save additional mime bundles in notebook #3107
base: main
Are you sure you want to change the base?
Feature: save additional mime bundles in notebook #3107
Conversation
As a concrete example: import ipywidgets as widgets
widgets.Image.from_url('http://localhost:8891/static/base/images/logo.png') Which will end up in the notebook JSON file as:
|
Of course, matplotlib/ipympl#16 and matplotlib/ipympl#294 and matplotlib/ipympl#150 are related as well, and big motivation for this from @martinRenou 's side I think. |
packages/base/src/widget.ts
Outdated
return Promise.resolve({ | ||
'application/vnd.jupyter.widget-view+json': { | ||
model_id: this.model_id, | ||
version_major: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we take the version from what the model defines instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to mention that this should be removed, actually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok :) 👍🏼
this.notebook.events.on('before_save.Notebook', async () => { | ||
var cells = Jupyter.notebook.get_cells(); | ||
// notebook.js save_notebook doesn't want for this promise, we are simply lucky when this | ||
// finishes before saving. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very weak :S
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this means it will be a lab feature only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about not making it a Promise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we to await the model, and making `generateMimeBundle async also makes sense, since it will involve async cases in bqplot at least.
Thanks a lot @maartenbreddels for trying this. I will give it a try. |
I cleaned it up a bit, and implemented this for HTML and Image. For instance, GitHub renders the HTML: |
Bad news, it seems we cannot do this in Lab either. You can connect to the save event, like we do with saving the widget state:
But again, this is sync only https://github.com/jupyterlab/jupyterlab/blob/b4db7f03d2ad0b91a9c8c252c56ae5e4a9408fbb/packages/docregistry/src/context.ts#L506 Maybe we can make it sync only for now, and it will work in classic and lab. |
@MSeal fyi |
Adding support for this would be quite helpful! It would also be really nice if we made this work across UIs (remember there's a dozen of so proprietary UIs as well). Working in classic, lab, and maybe nteract -- I think those are the 3 most popular open source UIs -- would be ideal to ensure it works in other environments. In lieu of that support, we should at least consider documenting changes needed by a UI that partially or completely supports widgets currently. e.g. 8.0 Widget UI Interaction changes section in the changelog? |
This will also require changes on all the frontends? My understanding from conversation on gitter awhile ago https://gitter.im/jupyter-widgets/Lobby?at=5f0ea2e30d37916fda770757 is that the frontend chooses the precedence in the mimebundle and currently lab et al always give the widget mimetype higher precedence. |
It seemed from comments that UIs would need to adjust their rendering code some to look for mimetypes within the bundle and that saving those mimetypes in the bundle had UI specific limitations. |
How lab/classic/nbconvert uses the mime bundle is something we can disagree about. I think we should have a dropdown to choose/override this default, in lab, classic and nbconvert, but we don't need to address that now I think. The main point of this PR is to have an API to ask for an extra mime bundle, an example use case (HTML and Image widget), and an implementation example for Lab and/or classic notebook on how to use that API (inject it on save). On that last point, it seems technically not possible, for classic and lab jupyterlab/jupyterlab#9764 if generating the mime bundle needs to be async. |
I think I agree with you? I was/am just concerned is that ipywidgets doesn't have the ability to do that overriding. So this may require work outside of ipywidgets.
👍 |
How would this play with #3114? |
They should play nice together and are independent (although I'd like both of them in 8.0), I'm working on this branch now, and I'm reusing some of the code from there to make it possible to do this in a sync way. |
widgetsnbextension/src/manager.js
Outdated
// finishes before saving. | ||
await Promise.all( | ||
cells.map(async cell => { | ||
var widget_output = cell.output_area.outputs.find(output => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting the following error from this line:
manager.js:228 Uncaught (in promise) TypeError: Cannot read property 'outputs' of undefined
at manager.js:228
at Array.map (<anonymous>)
at window._Events.<anonymous> (manager.js:227)
at window._Events.dispatch (jquery.min.js:2)
at window._Events.v.handle (jquery.min.js:2)
at Object.trigger (jquery.min.js:2)
at window._Events.<anonymous> (jquery.min.js:2)
at Function.each (jquery.min.js:2)
at S.fn.init.each (jquery.min.js:2)
at S.fn.init.trigger (jquery.min.js:2)
(anonymous) @ manager.js:228
(anonymous) @ manager.js:227
dispatch @ jquery.min.js:2
v.handle @ jquery.min.js:2
trigger @ jquery.min.js:2
(anonymous) @ jquery.min.js:2
each @ jquery.min.js:2
each @ jquery.min.js:2
trigger @ jquery.min.js:2
events.trigger @ events.js:31
Notebook.save_notebook @ notebook.js:2735
Notebook.save_checkpoint @ notebook.js:3333
handler @ actions.js:885
final_actions.<computed>.handler @ actions.js:934
ActionHandler.call @ actions.js:1019
ShortcutManager.call_handler @ keyboard.js:554
KeyboardManager.handle_keydown @ keyboardmanager.js:215
(anonymous) @ keyboardmanager.js:181
dispatch @ jquery.min.js:2
v.handle @ jquery.min.js:2
async function (async)
(anonymous) @ manager.js:226
dispatch @ jquery.min.js:2
v.handle @ jquery.min.js:2
trigger @ jquery.min.js:2
(anonymous) @ jquery.min.js:2
each @ jquery.min.js:2
each @ jquery.min.js:2
trigger @ jquery.min.js:2
events.trigger @ events.js:31
Notebook.save_notebook @ notebook.js:2735
Notebook.save_checkpoint @ notebook.js:3333
handler @ actions.js:885
final_actions.<computed>.handler @ actions.js:934
ActionHandler.call @ actions.js:1019
ShortcutManager.call_handler @ keyboard.js:554
KeyboardManager.handle_keydown @ keyboardmanager.js:215
(anonymous) @ keyboardmanager.js:181
dispatch @ jquery.min.js:2
v.handle @ jquery.min.js:2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's normal that cell.output_area
is undefined. We might want to add a check for undefined before this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you tested with only one cell containing an output. When there is no output under one cell in the Notebook, cell.output_area
seems to be undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that should be fixed now
74f0763
to
cdfcec9
Compare
Yes, I think that is still expected behaviour / separate issue. I think we should have a dropdown to select which mime entry to show. |
I've done a refactor of this. We now ask the view to generate the mimebundle. Jupyter notebook will call on notebook saving, the A new action (and menu item) "'Add extra to output cell mime bundle'" is added, which will call This approach makes it such that injection of mime bundles for common cases can be done sync, and thus be done on save in classic notebook and jupyter lab, while at the same moment allow for more expensive and async generation of mime bundles on user request. |
We should maybe brainstorm a bit about this. IMO a dropdown would not be a great UX. Edit: If we go for the dropdown idea, maybe we should provide a way for custom widget libraries to provide a sensible default repr. |
It's good to consider a few ideas on how to build on top, but indeed a dropdown is not the best UX, we could take a look at https://zeppelin.apache.org/docs/0.8.0/usage/display_system/basic.html for inspiration, but for sure a separate issue/PR. Let's first see if we can agree on populating the mime bundle in a user-friendly way. What do you think of the latest idea (auto save cheap mime bundles, expensive ones on user request)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think having two approaches like that is good :)
I also agree it's better to have it in the view.
jupyterlab_widgets/src/manager.ts
Outdated
/** | ||
* Register a widget model. | ||
*/ | ||
register_model(model_id: string, modelPromise: Promise<WidgetModel>): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it intended to remove those two methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they went up a base class (base manager), because we need to same feature for the notebook manager.
How confident are you that we could make a JupyterLab implementation? I've tried something very similar here: matplotlib/ipympl#294 but it didn't work for some reason. It seems to me that ipywidgets was overwriting whatever I was putting in the Notebook instance, but I might be wrong. |
I am totally lost in Lab-land, I cannot find an easy way to match the WidgetView to the output area (in classic we have access to the DOM, so we test with element.contains), if you have ideas on that, I can help you with the rest. |
Maybe @jtpio would have some ideas? |
c98e2d0
to
e1c5ab1
Compare
Rebased |
@maartenbreddels if you don't mind I'll iterate on this and will try to implement the same for JupyterLab. |
I'd also like to add a way for widgets to completely overwrite the mimebundle if they want. This would be useful for ipympl, because it has been asked many times to save a static image of the plot when saving the Notebook, which I believe make sense for ipympl. But this should not be the default for all widgets. |
This will be useful for widgets like ipympl or ipycanvas, where the model does not contain enough information to recover the widget (the kernel only contains the information about the state of the plot) so we need to store a static version of the plot (static image) for convenience, and we need to dump the widget model.
async generateMimeBundle() { | ||
return {}; | ||
} | ||
|
||
/** | ||
* Whether the `generateMimeBundle` output should | ||
* overwrite the mimeBundle or extend it | ||
*/ | ||
shouldOverwriteMimeBundle(): boolean { | ||
return false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about generateMimeBundle making the default mimebundle (with the widget in it).
subclases can then do:
return {'text/html': view.el.outerHTM, ... await super.generateMimeBundle()}
// or
return {'text/html': view.el.outerHTM}
depending on if they want to overwrite it. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
@maartenbreddels this is becoming less critical for ipympl, we're looking into always putting a static version of the plot in matplotlib/ipympl#343 |
Rather than a hook on save would it be possible to have an API like The other side of this is allowing widgets to access this data- ipympl should be able to reach back to the display item to get the image data when re-rendering. |
This is a proof of concept for allowing embedding of additional entries in the mime bundle, such as static images, svg, video etc from the front-end.
The basic idea is to have an API
Widget.generateMimeBundle
which will generate additional mime entries, which can be overridden in widget subclasses. Bqplot can inject svg, core ipywidgets can inject HTML for the HTML widget, an image for the Image widget etc.TODO:
text/html
mime entry.This PR differs from #2282 in just having an API for providing the API, instead of specific methods for particular representation (although the two PR's can be seen as orthogonal).
Related: #314 #2280 #2282 higlass/higlass-python#49 glue-viz/glue-jupyter#207