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

Allow Output.clear_output() to be thread-safe. #3261

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

Conversation

eriknw
Copy link

@eriknw eriknw commented Aug 29, 2021

Fixes #3260.

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch eriknw/ipywidgets/threadsafe_clear_output

@vidartf
Copy link
Member

vidartf commented Nov 30, 2021

There are some intricacies with this change that makes it not so straight-forward. First of all, I think a clear alternative to this change exists: Document that users can set the outputs trait to empty as an alternative way of clearing outputs, and explain in which situations the two ways of clearing the outputs makes the most sense.

Getting more into details, I note the following points:

  1. The content of an output widget can be modified in two ways:
    a. By manipulating the outputs trait, either directly, or via the convenience append_* methods on the output widgets.
    b. By entering its context manager, and using standard Jupyter protocol messages (e.g. via IPython.display.display() or IPython.display.clear_output() etc).
  2. I think that mixing the two patterns of modifying the outputs can lead to some race-conditions, even without threads. I qualify this with a "I think that" as I do not have any repros to demonstrate it.
  3. The context manager only works on the main thread.
  4. The current proposal in this PR would have the implementation of the clear_output method change from one way of modifying the outputs to another depending on the signature which it is called by, i.e. out.clear_output() differs from out.clear_output(wait=False). It would also change the default way used in most cases.
  5. The @out.capture(clear_output=True) decorator uses clear_output(), which would make it likely to mix the two patterns, increasing the risk of races.

In sum, I think this change would be likely to make the behavior more mysterious/confusing. Instead I think we should more clearly document the current pattern and suggestions. An example for of a documentation paragraph could be:

There are two different ways of changing the outputs of an Output widget:
- By manipulating the `outputs` trait of the widget. This includes calling the `Output.append_*` methods.
- By using it as a context manager (in a `with:` clause), and using standard methods for manipulating outputs, i.e. `IPython.display` etc.

Mixing the two ways is not recommended, as it is likely to cause race conditions, giving unpredictable behavior.

Note: For multi-threaded scenarios, only the first method is going to be effective, as the context manager requires that the code is running on the main thread.

We can then also add a note in the docstring of clear_output saying that it will not work from another thread than the main thread. Ideally we would also add a similar note for the context manager (__enter__ method? __init__? class docstring?).

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.

Output.clear_output() in a thread does not block until the output is cleared
2 participants