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

Output widget context manager prints to wrong cell (if used in a Thread) #2358

Closed
soerenwolfers opened this issue Apr 4, 2019 · 17 comments · Fixed by ipython/ipykernel#1186
Milestone

Comments

@soerenwolfers
Copy link

If I wrap this code from source/docs/examples/Output Widget.ipynb,

import ipywidgets as widgets
out = widgets.Output(layout={'border': '1px solid black'})
with out:
    for i in range(10):
        print(i, 'Hello world!')

in a Thread,

import ipywidgets as widgets
out = widgets.Output(layout={'border': '1px solid black'})
def foo():
    with out:
        for i in range(10):
            print(i,'Hello world!')
IPython.display.display(out)
threading.Thread(target=foo).start()

then the output ends up in a wrong cell as soon as I execute another cell.

I would consider this a bug, as I cannot imagine anyone wanting the Output widget to change cells. To convince you of the awesomeness of updating the Output widget in the cell where it was started, imagine how easy it would be to make a simple

%%run_cell_in_background
for i in range(10):
        print(i, 'Hello world!')

out of this (indeed, that's what I did, and it works beautifully except for the problem above. In my application, I need a simple job monitor, but I am sure there would be dozens of different uses for this)

@soerenwolfers soerenwolfers changed the title Output context manager prints to wrong cell Output context manager prints to wrong cell (if used in a Thread) Apr 4, 2019
@soerenwolfers soerenwolfers changed the title Output context manager prints to wrong cell (if used in a Thread) Output widget context manager prints to wrong cell (if used in a Thread) Apr 4, 2019
@jasongrout
Copy link
Member

Can you read https://ipywidgets.readthedocs.io/en/stable/examples/Output%20Widget.html#Interacting-with-output-widgets-from-background-threads carefully, which talks about using an output widget from a thread, and then post your thoughts about it?

@jasongrout jasongrout added this to the Reference milestone Apr 4, 2019
@maartenbreddels
Copy link
Member

This ties into the discussion with @andymcarter on gitter.
We probably also want to have https://github.com/jupyter-widgets/ipywidgets/blob/master/ipywidgets/widgets/widget_output.py#L63 directly manipulate the outputs traits, instead of relying on the front end to do so.

@soerenwolfers
Copy link
Author

soerenwolfers commented Apr 4, 2019

@jasongrout That exactly describes what I saw and disliked. I also thought about the solution to pass around widgets that is presented there but didn't like it because it can't be used to create non intrusive cell magic. It would be awesome if you guys found a way around jupyter's shortcomings somehow. I realize that at this point it's really not about actual widgets anymore but maybe you still feel like it's.a useful thing to have

@beasteers
Copy link

+1. Having some fix for this would be great. I'm basically trying to start up a bunch of jobs that print to separate Output widgets inside tabs, but I don't have the ability to go into the job code I'm running and change the output statements to use out.append_stdout or out.append_display_data.

Do you see it being possible to rework the Output widget so that they can capture outputs from different threads concurrently and place them in their own widget?

I see that output widgets get streamed their outputs using a msg_id, and I don't totally understand how it works. But would it be possible to assign a different msg_id per thread/process and have each thread/process write to that id (and their respective outputs)? I see that it uses ip.kernel._parent_header['header']['msg_id'] which looks like a global id so I'm not sure.

@jasongrout
Copy link
Member

jasongrout commented May 21, 2019

I see that it uses ip.kernel._parent_header['header']['msg_id'] which looks like a global id so I'm not sure.

That's the key problem in the current IPython kernel - the parent header (which is how things are routed) is a global shared across all threads.

@beasteers
Copy link

Gotcha. Are there plans to improve that situation? Or is it something that's too difficult to address without completely uprooting things

@jasongrout
Copy link
Member

Are there plans to improve that situation? Or is it something that's too difficult to address without completely uprooting things

I don't know of anyone working on this in IPython itself (though that repo would be the place to ask). @SylvainCorlay and some others from quantstack are experimenting with a new python kernel based on the C++ backend xeus that experiments with other concurrency models.

@maartenbreddels
Copy link
Member

It wouldn't be such a bad idea to refactor the output widget. We could have the same logic in the frontend and backend (so that the Output widget behaves the same without a front-end connected). And if we send the outputs using a custom msg, we could send the msg_id with it.
It is tricky work, and requires ipywidgets, the labextension, the notebook extension, nbconvert and voila to all be in sync.

@beasteers
Copy link

Hmmm. I haven't really looked into this or tested it, but from a cursory glance, I wonder if something like this could be a bandaid? It would have to be extended for multiprocessing as well and I'm not quite sure how to make it work with IPython.display.display

https://stackoverflow.com/a/43667367

Basically, it adds a proxy around sys.stdout and redirects to a specific stringIO based on the thread name. My main reservation is that now Outputs would have to be thread/process aware which really doesn't seem like their job. Maybe having this as a separate package would work better.

@mgeier
Copy link
Contributor

mgeier commented Jan 5, 2020

I don't know if this is the same issue, but the output also shows up at the wrong place when using nbconvert --execute, even if there are no additional threads created by the user. And as a consequence, this also happens with nbsphinx, which leads to this wrong output in the ipywidgets docs themselves: https://ipywidgets.readthedocs.io/en/latest/examples/Output%20Widget.html.

@jasongrout
Copy link
Member

I don't know if this is the same issue, but the output also shows up at the wrong place when using nbconvert --execute, even if there are no additional threads created by the user. And as a consequence, this also happens with nbsphinx, which leads to this wrong output in the ipywidgets docs themselves: https://ipywidgets.readthedocs.io/en/latest/examples/Output%20Widget.html.

This was a separate problem with nbconvert not supporting output widgets.

@jasongrout
Copy link
Member

The slider widget works fine. The togglebuttons widget doesn't.

There was no slider or toggle widget in the original description, it was about output widgets capturing standard output in threads. Is that the issue you are commenting about? If not, can you please open a new issue with steps to reproduce what you are seeing and a full description of the problem and what you expect?

@StefanBrand
Copy link
Contributor

For reference: The proposed workaround does not work with clear_output(): #2584 (comment)

@modaresimr
Copy link

Is it possible to have a similar thing in a multiprocess environment?

@krassowski
Copy link

@jasongrout was there ever a discussion about modifying kernel-frontend protocol to include cell ID in messaging (and routing) to ensure that the output goes to the correct cell?

@krassowski
Copy link

ipython/ipykernel#1186 proposes a solution on ipykernel side using Thread.identity to associate thread emitting outputs with the appropriate execution request.

@maartenbreddels
Copy link
Member

Note that #3759 is a different solution, one that we adopt in solara.
Note that both are slightly overlapping, ideally I'd like to see both PR in, since I do not like that widgets need a frontend for a functional output widget.

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 a pull request may close this issue.

8 participants