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

Add examples for asynchronous display and interactive_output #1794

Conversation

mrled
Copy link
Contributor

@mrled mrled commented Nov 3, 2017

As discussed in #1790

@jasongrout I stole some of your prose and code from comments in that issue. I hope that's ok - if not I can rewrite it.

I'm also not sure how to save the outputs so that they'll render properly on the documentation site. I tried to do the cleanest commit I could, so I just copied the JSON from my running copy of the notebooks into a clean copy from git. Is that OK, or do I need to do something else to make sure that the widgets are displayable on the documentation site?

"\n",
"This is because output widgets are capturing based on the thread in which they were invoked. In other words, in the thread where the output is invoked, there is a context manager which starts and then stops the output capturing. If the threaded displays happen to occur during this time frame, they'll also be captured. However, if the threaded displays happen to occur after the main thread stops capturing output, those displays will not be put in that output widget.\n",
"\n",
"This means that sometimes, trying to display a widget from a background thread will appear to work fine, and sometimes one or more widgets that were supposed to be displayed will be simply missing."
Copy link
Member

@pbugnion pbugnion Nov 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that saying users shouldn't try to display a widget from a background thread is important (I wasn't aware of this).

However, I don't think we should add an example that doesn't work. It just makes the documentation longer, and often, people copy and paste snippets directly from documentation without really reading the context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key isn't so much on displaying a widget, it's that displaying anything may or may not be captured, depending on what's happening in the main thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok, I can delete this section. I was thinking that someone might just try threading and see if it works (which is what I did) and if they had started researching after that didn't work, it would be cool to see an explanation of what's happening. I'll try to refactor that into prose instead so that there is no nonworking code to copy/paste.

" display(\"beta() Two {} at {}\".format(something, time.time()))\n",
" display(\"beta() Thr {} at {}\".format(something, time.time()))\n",
" display(\"beta() Fou {} at {}\".format(something, time.time()))\n",
" display(\"beta() Fiv {} at {}\".format(something, time.time())) \n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not just use print instead of display here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print seems to work for me, whereas display gives the issue we've seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's my experience as well - I wasn't able to trigger the problem with print(), but I was able to trigger it with display().

@jasongrout
Copy link
Member

Here's my minimal example, trimmed down from your example:

import threading
from IPython.display import display
import ipywidgets

def alpha(textvalue):
    def beta(something):
        for i in range(1,10):
            display('%d'%i + '**'*i + something)

    display('Display in main thread: ' + textvalue)

    mapthread = threading.Thread(
        target=beta,
        args=(textvalue,))
    mapthread.start()

ipywidgets.interact(alpha, textvalue='A String')

@jasongrout
Copy link
Member

@jasongrout I stole some of your prose and code from comments in that issue. I hope that's ok - if not I can rewrite it.

That's totally fine.

" \n",
" while ctr < timerlength:\n",
" time.sleep(interval)\n",
" ctr += interval\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just time.sleep(timerlength) -- you then don't need the interval argument.

" button = ipywidgets.Button(description=\"Button #1\")\n",
" container.children += (button, )\n",
"\n",
" display('I am in alpha() at {}'.format(time.time()))\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly saying 'In main thread', rather than 'I am in alpha', seems like it would help illustrate that you can interact with the widget both from the main thread and the child.

@pbugnion
Copy link
Member

pbugnion commented Nov 3, 2017

Thanks for submitting this -- I think it's a useful addition. I made some comments inline.

@jasongrout
Copy link
Member

Yes, thank you very much for submitting this PR to the docs!

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from 9f178e1 to 338eba2 Compare November 3, 2017 16:45
@mrled
Copy link
Contributor Author

mrled commented Nov 3, 2017

Thanks for the feedback ya'll! What do you think of it after these changes?

@pbugnion
Copy link
Member

pbugnion commented Nov 6, 2017

This looks great! Thanks for making changes, and for taking the time to contribute to ipywidgets docs.

I think we should clarify in the title and copy that we are talking about output widgets. How about something like this for the copy. I've made very slight wording changes based on what you'd written:

### Interacting with output widgets from background threads

Output widgets capture output based on the thread in which they were invoked.
In other words, in the thread where the output is invoked, there is a context
manager which starts and then stops the output capturing. If you call `display`
in a thread other than the thread containing the context manager, you cannot
rely on those `display` calls being captured by the context manager.

Instead, we can define and display a box widget in the main thread, and add
output widgets to it from a secondary worker thread. This does not break the
context manager that handles communication between the kernel and the frontend,
and it will work reliably every time. It operates on a similar concept to the
previously discussed task of modifying a widget's `value` in the background.

The code example is really good. I have the following suggestions:

  1. Could we remove the outer alpha function, and make everything happen at the top level of the notebook? Instead of having the call to interact, the VBox would be the top level widget. Maybe I'm missing something, but it doesn't feel like interact matters in this case.

  2. We should probably use import ipywidgets as widgets for consistency.

  3. We should call the thread thread, rather than mapthread, for consistency with the previous example.

  4. I think string interpolation is better than concatenation. Ie. I think writing '{} {} {}'.format(i, '**'*i, something) is clearer than '%d'%i + '**'*i + something.

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from 338eba2 to 6ad2d37 Compare November 7, 2017 18:42
@mrled
Copy link
Contributor Author

mrled commented Nov 7, 2017

All those changes make sense to me - updated. Anything else need work?

@@ -898,6 +898,96 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"### `interactive_output`\n",
"\n",
"The `interactive_output` function provides maximum flexibility at the cost of a bit more code.\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word 'flexibility' is a bit vague. How about:

interactive_output provides additional flexibility: you can control how the UI elements are laid out.

"\n",
"The `interactive_output` function provides maximum flexibility at the cost of a bit more code.\n",
"\n",
"It does not generate a user interface for the widgets, like `interact`, `interactive`, and `interact_manual` do. This is powerful, because it means you can create a widget, put it in box, and then pass the widget to `interactive_output`, and have control over the widget and its layout."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe like ... do -> unlike (to make the opposition even clearer).

put it in box -> put it in a box

@pbugnion
Copy link
Member

pbugnion commented Nov 8, 2017

The asynchronous widgets bit looks good!

I'd forgotten to review the interactive_output bit. I've made some minor comments on the text inline. For the code example, I have a few style comments:

  • we should stick to import ipywidgets as widgets style imports (I know that we use `from ipywidgets import FloatSlider earlier, but the majority of the notebook just uses the former style)
  • I think PEP 8 dictates a space around assignment operators, a new line before functions and a space after the commas in lists, tuples and argument lists (again, I know this notebook doesn't follow PEP 8 consistently):
a = IntSlider()
b = IntSlider()
c = IntSlider()
ui = HBox([a, b, c])

def f(a, b, c):
    print((a, b, c))
  • I think the display call should be display(ui, out).

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from 6ad2d37 to cfa7bf4 Compare November 8, 2017 19:14
@mrled
Copy link
Contributor Author

mrled commented Nov 8, 2017

Ha yes that makes sense, I shoulda made the changes to both examples. Updated with your suggestions - what do you think of it now?

@jasongrout
Copy link
Member

Thanks - you've done some great work here. There is one more subtle race condition. Because the output capturing happens in the browser, and works with a stack of capturers, things could get messed up if you have two threads both producing output. For example, if this is the order of events:

Thread 1 creates an output widget and starts the output context
Thread 1 prints "T1:A"
Thread 2 creates an output widget and starts the output context
Thread 2 prints "T2:A"
Thread 1 prints "T1:B"

Then thread 2's output capturing will get "T1:B".

The reliable way to do things here is to not use output capturing, but to directly append values to the output widgets, with out.append_stdout, out.append_stderr, or out.display_data (these were added in #1752, released in ipywidgets 7.0.3).

@jasongrout
Copy link
Member

I should add that these issues are pretty fundamental with how the python kernel handles output. IIRC, there is a single datastructure containing metadata about where the output should go (e.g., the parent message, etc.), and that is shared across threads. So there's not an easy way to send a single output message with a piece of metadata saying "I'm from thread X" without constructing and sending the message yourself (in which case I suppose you could put in some metadata).

CC @minrk for verification of details about how output messages are sent from threads.

Dave Willmer did do some work on handling output from different threads at ipython/ipykernel#115, but in the end we decided to do output capturing in the browser side to support more kernels better.

@mrled
Copy link
Contributor Author

mrled commented Nov 9, 2017

@jasongrout Oh wow interesting, ok. To make sure I understand you correctly, are you saying that I should remove the with output: statement, and instead do something like output.display_data += [display(whatever)] ? (Where output is a widgets.Output().) Do I have that right?

@jasongrout
Copy link
Member

do something like output.append_stdout('{} {} {}'.format(i, '**'*i, something)) for normal output, or something like output.append_display_data(displayable_object) for an object with rich display (like output.append_display_data(HTML('<b>hello world!</b>'))

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from cfa7bf4 to 169de93 Compare November 9, 2017 22:42
@mrled
Copy link
Contributor Author

mrled commented Nov 9, 2017

Aight, pushed ✨ what do you think?

@jasongrout
Copy link
Member

jasongrout commented Nov 9, 2017

Thanks, I think that looks much better. Can you update the text as well, which still refers to a box and output widgets in the thread? Then I think this is ready to be merged.

(here's the text that I think should be updated to reflect the new state of the code)

"Instead, we can define and display a box widget in the main thread, and add\n",
"output widgets to it from a secondary worker thread. This does not break the\n",
"context manager that handles communication between the kernel and the frontend,\n",

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from 169de93 to 7540fa5 Compare November 10, 2017 16:22
@mrled
Copy link
Contributor Author

mrled commented Nov 10, 2017

Alright, updated the text, how does it look now?

Also, I had a question about append_display_data(). I was playing with it in my notebooks, and looks like it can take some display()able things like the return value from IPython.display.HTML(), but not other things like an ipywidgets.Box or an ipyleaflet.Map. Is that a bug (which I should file a report for)? Or is there some other way to asynchronously display whole new widgets?

@jasongrout
Copy link
Member

I was playing with it in my notebooks, and looks like it can take some display()able things like the return value from IPython.display.HTML(), but not other things like an ipywidgets.Box or an ipyleaflet.Map. Is that a bug (which I should file a report for)? Or is there some other way to asynchronously display whole new widgets?

Hehe - I found the same issue yesterday playing with it: #1811

@jasongrout
Copy link
Member

jasongrout commented Nov 10, 2017

Alright, updated the text, how does it look now?

I like it! I would delete this sentence:

"These methods are designed to work properly with the context manager,\n",
"and they will work reliably every time."

as I'm not sure what "work properly with the context manager" means, and we wouldn't suggest it if it didn't work reliably or properly.

Other than that, I think this is good to go!

@jasongrout
Copy link
Member

I would delete this sentence:

(but if you disagree, I think it's fine as well - this is a minor point)

@mrled mrled force-pushed the add-async-and-interactive_output-examples branch from 7540fa5 to e7f8fe6 Compare November 10, 2017 17:06
@mrled
Copy link
Contributor Author

mrled commented Nov 10, 2017

Done - I was trying to tie it back to the previous paragraph but if it doesn't make sense then it's not going to help, heh.

@jasongrout
Copy link
Member

Done - I was trying to tie it back to the previous paragraph but if it doesn't make sense then it's not going to help, heh.

I think "Instead" is a sufficient connector :)

@jasongrout jasongrout merged commit dad1af2 into jupyter-widgets:master Nov 10, 2017
@jasongrout
Copy link
Member

Thanks again!

@mrled
Copy link
Contributor Author

mrled commented Nov 10, 2017

Thank you for your patience in answering my questions, and guiding me through the PR process!

@jasongrout jasongrout added this to the 7.0.4 milestone Nov 17, 2017
@github-actions github-actions bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants