-
Notifications
You must be signed in to change notification settings - Fork 572
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
Embed widget state in notebook on execute #900
Conversation
b161eb0
to
3784814
Compare
Ok, I don't think these failures are caused by this PR, although the Python 2.7 failure is a big strange. All the other are caused by a different in the output for the notebook with the KeyboardException, which now does not include the parenthesis |
Will this be available in the next release? It seems to work fine, I also tried it programmatically with |
I really hope so! |
Yes exactly, that's the idea! |
a6b9e2b
to
d3f60ac
Compare
I think this branch is ready for review/merge |
Can someone please review this? |
Will take a close look this next week and try it out |
@MSeal Did you have a chance to have a look? |
Not yet. I got busy and then vacation hit. I will prioritize digging into
this when I get back.
…On Tue, 12 Mar 2019, 10:19 AM Matthias Geier, ***@***.***> wrote:
@MSeal <https://github.com/MSeal> Did you have a chance to have a look?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#900 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABxw1OUDvksnpmJQV143T3Otgm9xr7ciks5vV9OkgaJpZM4XelQM>
.
|
Currently default values are not removed and binary buffers are not saved. (updated to conform to widget state schema)
abb2e49
to
9877c9e
Compare
Added a flag to disable the storing of the widget state, e.g.:
|
@maartenbreddels Finally got around to looking at this PR. The code looks good, but I'm struggling to reproduce the UI display despite the metadata being present.
produces despite the fact that the metadata is getting populated: I tried upgrading jupyter classic, jupyter lab, ipython, etc but I'm not getting the rendering that others were posted in related threads to this PR. Ideas for what might be wrong in my setup to reproduce or what might be going wrong with widget state? |
Do you get a rendering if you simply execute the cells in the notebook? If not, there may be an issue with the installation of ipywidgets, for instance pre notebook 5.3 you had to enable the extension manually. |
Yes. It renders fine if I execute the cells from classic or lab. I upgraded to latest widgets, ipython, and lab (classic hadn't updated in a while). Maybe I should try some different browser / OS combinations. I am on Ubuntu 17.10 latest Chrome |
I had to click File->'Trust this notebook' |
Ahh yes, I figured I was missing something silly. Each run of the notebook is requiring me to re-trust the outcome, but that's a separate conversation / thread. |
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.
So this solution is working very well -- I even tested custom %%javascript cell logic and it handled it appropriately as well as a variety and combination of widgets interacting and independently operating.
The one thing we should add is documentation for the widget support (including the trust notebook step) and the scope of the support. For example this makes ipynb -> ipynb translation work for standard widgets. It doesn't support other export formats (we can talk about #901 separately and update docs there-in). And it will default behavior for widgets that rely on user input to widget default states or require python programmatic manipulation of the widget to update on nbconvert runs.
Would you prefer to merge this PR first and then add docs, or add some documentation to this PR before merging? I can probably help, just might be delayed before I can write it and you'll probably do better justice to outlining the level of support.
Thanks, and yes indeed, I'd rather see this merged first. |
Thanks, really happy to see this in master, looking forward to the next release! |
Thanks for making the PRs. I'll try reading through 901 later this week. Plan is currently to do a release early to mid April after some other changes get finalized. |
@maartenbreddels Any chance you could add those document requests before we release? It'd help with making the release notes. |
I propose the following at the end of execute_api.rst:
Feel free to copy/paste and/or modify if you want (I think it is less work than opening a PR for this, unless you disagree) |
I'll probably open a PR anyway so it's more visible -- but that helps. The only additions I think we should include might be:
Thoughts or rewording? |
sounds good!
Op do 4 apr. 2019 om 19:32 schreef Matthew Seal <[email protected]>:
… I'll probably open a PR anyway so it's more visible -- but that helps. The
only additions I think we should include might be:
This widget rendering is not performed against a browser during execution, so only widget default states or states manipulated via user code will be calculated during execution. `%%javascript` cells will execute upon notebook rendering, enabling complex interactions to function as expected when viewed by a UI.
If you can't view widget results after execution, you may need to select `Trust Notebook` under the `File` menu.
Thoughts or rewording?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#900 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABryPcCM6U9gRO9PwizzJ4NTOZLaJwCcks5vdjdBgaJpZM4XelQM>
.
|
Rebase of #779 which is a followup of #760