-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Incorrect behaviour: plt.show(block=True) with qt5 and jupyter #13302
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
Comments
This is related to #4779. I advise against using the gui backends inside of jupyter notebooks. It will not work if you ever push the notebook to a remote server (as it will try to open the widows on the server, not your local machine). |
@tacaswell Yes, I understand; however, I can't use Do you know why KeyboardInterrupt is not working? In
so it is expected to react correctly. Looking at the history, @joelfrederico and @anntzer were working on this; may be they could shed some light? I would very much appreciate that! |
"Crashing" is by design (IIRC from what I read... Somewhere...). When writing a GUI app, the most logical behavior for ctrl-c is to send an the usual SIGTERM to the Qt loop as if the whole thing were written in C. You're using a backend in a way that wasn't intended. I think you'll have to decide: are you writing an app or are you working on a notebook? You're trying to combine functionality from two distinct modes of operation and that's usually a recipe for glitches and odd behavior. |
Ctrl-C sends SIGINT, not SIGTERM. |
Ah right my mistake! |
Well, I think I get it -- so you send SIG_DFL to C code and it kills Qt and takes down CPython with it as well? Considering the question about how one should use matplotlib, I think that using it from shell or from IPython is extremely wide-used at least in the academic environment. I personally wouldn't want to write a GUI application that will work exactly the same way as Jupyter. So, I did some search and here is what I came up with for
Well but now one would need to send Ctrl+C twice to firstly kill the matplotlib window, and then to send Python a KeyboardInterrupt. Is this a bad idea? |
Haven't fully thought about it but at first sight the proposed behavior looks like a reasonable compromise. |
I've created a pull #13306 for this. It involves only 1 file, eventually. |
I'd disagree. Timers kill performance, they're an exceptionally bad idea. If you really want Python to handle a signal, you should register a handler with Qt: http://doc.qt.io/qt-5/unix-signals.html. (There is an analogue Windows method to this as well.) Also, we should realize that there are multiple use cases for matplotlib. Changing the default behavior is only likely fix some people's problems at the expense of other people. We should offer a way to get different behaviors. I've always felt matplotlib forcing a specific handler to be a bit heavy-handed. I think a better solution would be to allow people to set their own handler. Then the question would be, what should the default behavior be? |
I think the average user doesn't understand Qt event loops. The backend is hidden from them. I'd be okay with a ctrl-c sending a SIGINT to Qt, which then gets handled in Qt and calls a Qt exit, and also later causes a KeyboardInterrupt to be raised once Qt is closed. I think that makes the most sense- it allows the Qt GUI to close in the same way clicking the close button would, and then causes matplotlib.pyplot.show to raise a KeyboardInterrupt, which should make sense in the Python world- that's the command that was running when the ctrl-c was pressed. Thoughts? |
@joelfrederico Yes, I think your last suggestion is very nice. I will try to do that now. |
For reference, on Windows you'll want to set things up similarly to how PyZMQ does things: https://pyzmq.readthedocs.io/en/latest/api/zmq.utils.win32.html Signals are very different on Windows! |
Writing data acquisition code in python is a major part of my day-job (see https://github.com/nsls-ii/bluesky) so I am going to claim a bit of domain expertise here. I am not sure how your system is architected, but I do not think you want to relying |
@tacaswell, thank you for the feedback! I've looked through your lib, and I see you have taken the backend apart piece by piece. Maybe I will as well come to this some day, when I'm ready... I can relate (considering the daily data acquisition). We have been used I don't use the plotting loop as the main loop (the acquisition runs in a different thread), but the only way I found to wait until the measurement ends was to rely on How do you wait for the end of the acquisition? I couldn't find this in your code quick enough |
@joelfrederico Concerning performance: I am not a C++ and Qt expert, so correct me if I am wrong, but it seems to me that the timer here will never be the bottleneck (for example, see here). I am 100% sure that calling one lambda function with no arguments with half a second period will never compare to the plotting part in any case. Actually, running the animation from my example with 1ms step only yields 150-200 fps which is 5 times slower than requested. Of course, this does not depend on whether I launch timer or comment it out of the code. I did not yet test on Windows; hope that it will work at least with no surprises. I would say that the timer workaround is just ugly and, for sure, I don't like it as well. Conversely, dealing with Windows-specific signals and Qt internals in your link looks a bit intimidating to me and I fear even more ugly code. May be, the clues are in this thread and everything's not that bad? Or maybe we should leave signals this as TODO for the future generations? By the way, I've implemented and tested raising KeyboardInterrupt after |
I think this should really be up to @tacaswell or somebody of his standing. I'm not a senior contributor here, so my opinion should hold minimal weight compared to his. :) Timers certainly aren't best-practices for Qt, because they start to get layered onto other things that might not be best-practices. It's technical debt- how much do you want to incur before you say enough is enough? I'm not a high-level contributor so I don't have an answer to that question. As for how to raise the KeyboardInterrupt, if you put the handler that accepts the socket comm in the right spot, it can be a closure in the same scope where exec_() is called. Then, the handler has access to a variable in that scope and can set it. You can test that variable right after exec_() to decide whether or not to throw. I'm not sure what you did so I can't comment. I don't know how/why you need your |
@vdrhtc We use asyncio for concurency so everything runs on the main thread. I would suggest doing something like (and I am very fast-and-loose with the pseudo code): import threading
import time
def do_experiment_on_other_thread(ev):
def real_worker():
time.sleep(5)
ev.set()
threading.Thread(target=real_worker).start()
def do_my_experiment(...):
done_event = threading.Event()
def gui_killer():
done_event.wait()
qApp.exit()
do_experiment_on_other_thread(done_event)
th = threading.Thread(target=gui_killer)
th.start()
qApp.exec_() A more complete example is at https://github.com/NSLS-II/bluesky/pull/1125/files#diff-1292145f2de491d852c62c40962538dcR1279 which uses a single-shot timer to avoid a race condition. Basically:
Definitely read #4779 (and please provide feedback if it is not actually helpful). Also, consider adopting bluesky 😈, we already have solved a bunch of these problems. |
a bit more details: Currently we are using a single thread which runs the asyncio event loop and a blocking process, have a task scheduled that occasionally kicks qt main loop (or the notebook backend) and when we are done with the scan stop the asyncio event loop. This is less than great, we want to move to a model where we start an asyncio event loop on a background thread, pass the scan instructions back to the always running event loop, and then block the calling thread via the On a bit more consideration, you should do what we are moving towards, not what we are doing now. |
@joelfrederico, oh, I'm sorry, I forgot to mention that was included into the PR already. It was like two lines of code (commit a6274). But now we can forget about it, I've got rid of the QTimer and the interrupted field, please look here: commit 6eea8. New class Above Python 3.5 (compliant with the current matplotlib requirements) that should work on Windows as well but I haven't tested yet. What do you think? |
@tacaswell, thank you for the code! I think that the idea is very similar to what I wanted to do, because Today I did some testing and found that I don't actually want the qApp to quit upon completion of the measurement! Sometimes I want old plot windows to hang around so I can monitor the experiment history, but they get killed if qApp exits. This means that I actually don't want to block on I will certainly look at #4779 and will try to contribute. Considering bluesky: I will definitely study the code, maybe it's time for me to learn asyncio. I am not sure if you have already advertised your lib to people, but for me it was a bit hard to find where to start (I am still a bit confused, as the examples.py is stated to be old and plans.py is very large). Maybe you should put a Mock-based Demo.ipynb for users to play around with different measurements! And, well, full adoption would be a pain I guess, so at least I will try to adopt some ideas =) |
Start at https://nsls-ii.github.io/bluesky/tutorial.html (which has a link to a bnder repo that has the full stack already installed and set up). |
This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help! |
Bug report
Bug summary
I'm implementing live plotting for our data acquisition system using Jupyter and FuncAnimation, and have just found an interesting defect in
plt.show(block=True)
behaviour.Precisely, it does not unblock when the plot window is closed, and sending KeyboardInterrupt crashes the kernel.
I was able to accidentally fix the first part by modifying the
show()
function inbackend_bases.py
based on this forum reply (basically, by not callingmanager.canvas.figure.show()
when block is not None, and instead callingmanager.window.setVisible(True)
just beforecls.mainloop()
since it calls Qt'sexec()
that should callshow()
by itself)The second issue persists, though.
Code for reproduction
Actual outcome
Plot window that can be closed by either pressing the corresponding window button or by sending KeyboardInterrupt. The kernel then should be freed.
Expected outcome
The kernel is not freed upon closing (without my fix) and dies on KeyboardInterrupt.
Matplotlib version
Everything installed with pip in a virtualenv.
The text was updated successfully, but these errors were encountered: