Skip to content
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

feat: Thread safe kernel side Output widget #3759

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Member

Background

Working on support the output widget in solara:

I found out about the hooks in ipykernel ipython/ipykernel#115

This approach will probably be better way forward.

The issue

Without a frontend, the output widget does not work. We had to work around that for Voila voila-dashboards/voila#91 (now ported to nbclient jupyter/nbclient#68 ).

Futhermore, the frontend is not aware of threads, causing all kinds of issues, e.g.:

This does require ipython/ipykernel#1110, so even if we go forward with this, the old code will have to stay in.

Testing and ipykernel

This solution only works with ipykernel/ipython, and I am not sure how to test this. Also, this goes against the direction of begin independent of a particular implementation. For instance, I am not sure xeus-kernel could support this (cc @martinRenou ).

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch maartenbreddels/ipywidgets/feat_output_widget_kernel_side

@maartenbreddels
Copy link
Member Author

Example code that works in the notebook:

import threading
import time
import ipywidgets as widgets


def run1():
    with out1:
        for i in range(10):
            display("output to 1")
            time.sleep(0.2)

def run2():
    with out2:
        for i in range(10):
            display("output to 2")
            time.sleep(0.2)
        

out1 = widgets.Output()
out2 = widgets.Output()
threading.Thread(target=run1).start()
threading.Thread(target=run2).start()
widgets.HBox([out1, out2])

This would totally break before this PR, all output seems to go to 1 of the widgets.

@jasongrout
Copy link
Member

This solution only works with ipykernel/ipython, and I am not sure how to test this. Also, this goes against the direction of begin independent of a particular implementation. For instance, I am not sure xeus-kernel could support this (cc @martinRenou ).

This hits on the fundamental decision we had to make when originally implementing output widgets and evaluating the experiments at ipython/ipykernel#115: Do we make the kernel more complicated (with an output redirection mechanism) and the frontend simple (which arguably is a better architecture), or do we make the frontend more complicated and need no changes to any kernel? We decided there were likely to be far more kernels than frontends, so the balance shifted to the somewhat awkward architecture we have today, which is kernel agnostic.

In practice, though, there have been fewer kernels implementing ipywidgets support and more frontends implementing ipywidgets support than (at least I) anticipated. So I'm glad you are reevaluating the original tradeoff.

maartenbreddels added a commit to maartenbreddels/ipykernel that referenced this pull request Jul 20, 2023
A display hook can handle a publish message, but not yet a clear_output

Upstreaming of: widgetti/solara#132
Follow up of: ipython#1110
Related: https://github.com/ipython/ipykernel/pull/115/files

Would enable jupyter-widgets/ipywidgets#3759
to fully work kernel side.
@maartenbreddels
Copy link
Member Author

Thank you for the context Jason!

Note that this also requires ipython/ipykernel#1135 to be able to intercept the clear_output

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

Successfully merging this pull request may close these issues.

2 participants