Skip to content

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

Closed
vdrhtc opened this issue Jan 27, 2019 · 22 comments
Closed

Incorrect behaviour: plt.show(block=True) with qt5 and jupyter #13302

vdrhtc opened this issue Jan 27, 2019 · 22 comments
Labels
GUI: Qt status: closed as inactive Issues closed by the "Stale" Github Action. Please comment on any you think should still be open. status: inactive Marked by the “Stale” Github Action

Comments

@vdrhtc
Copy link
Contributor

vdrhtc commented Jan 27, 2019

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 in backend_bases.py based on this forum reply (basically, by not calling manager.canvas.figure.show() when block is not None, and instead callingmanager.window.setVisible(True) just before cls.mainloop() since it calls Qt's exec() that should call show() by itself)

The second issue persists, though.

Code for reproduction

%pylab qt5
import numpy as np
import matplotlib.pyplot as plt
from matplotlib.animation import FuncAnimation
fig, ax = plt.subplots()
xdata, ydata = [], []
ln, = plt.plot([], [], 'ro')

def init():
    ax.set_xlim(0, 2*np.pi)
    ax.set_ylim(-1, 1)
    return ln,

def update(frame):
    xdata.append(frame)
    ydata.append(np.sin(frame))
    ln.set_data(xdata, ydata)
    return ln,

ani = FuncAnimation(fig, update, frames=np.linspace(0, 2*np.pi, 128),
                    init_func=init, blit=True, interval = 50)

plt.show(block=True)

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

  • Operating system: 4.18.16-100.fc27.x86_64
  • Matplotlib version: 3.0.2
  • Matplotlib backend: Qt5Agg
  • Python version: 3.6
  • IPython: 7.2.0
  • Jupyter notebook: 5.7.4

Everything installed with pip in a virtualenv.

@tacaswell
Copy link
Member

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).

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

@tacaswell Yes, I understand; however, I can't use %pylab notebook since it won't block with block=True (should I open another issue or is this expected?) and I need to perform several experiments in a sequence. Finally, sometimes having separate GUI windows is more convenient...

Do you know why KeyboardInterrupt is not working? In backend_qt5.py:1127 one can find:

# allow KeyboardInterrupt exceptions to close the plot window.
signal.signal(signal.SIGINT, signal.SIG_DFL)

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!

@joelfrederico
Copy link
Contributor

"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.

@QuLogic
Copy link
Member

QuLogic commented Jan 28, 2019

Ctrl-C sends SIGINT, not SIGTERM.

@joelfrederico
Copy link
Contributor

Ah right my mistake!

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

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 backend_qt5.py to get only the plot window to be closed (based on this):

@_Backend.export
class _BackendQT5(_Backend):
    required_interactive_framework = "qt5"
    FigureCanvas = FigureCanvasQT
    FigureManager = FigureManagerQT

    @staticmethod
    def trigger_manager_draw(manager):
        manager.canvas.draw_idle()

    @staticmethod
    def set_managers(managers):
        _BackendQT5.managers = managers
    
    @staticmethod
    def interrupt_handler(*args):
        for manager in _BackendQT5.managers:
            manager.destroy()
     
    @staticmethod
    def mainloop():
        old_signal = signal.getsignal(signal.SIGINT)
        signal.signal(signal.SIGINT, _BackendQT5.interrupt_handler)
        timer = QtCore.QTimer()
        timer.start(500)  # every 0.5s we are able to catch a SIG_INT in the interpreter and it then will be handled by  _BackendQT5.interrupt_handler
        timer.timeout.connect(lambda: None)
        
        try:
            qApp.exec_()
        finally:
            # reset the SIGINT exception handler
            signal.signal(signal.SIGINT, old_signal)

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?

@anntzer
Copy link
Contributor

anntzer commented Jan 28, 2019

Haven't fully thought about it but at first sight the proposed behavior looks like a reasonable compromise.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

I've created a pull #13306 for this. It involves only 1 file, eventually.

@joelfrederico
Copy link
Contributor

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?

@joelfrederico
Copy link
Contributor

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?

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

@joelfrederico Yes, I think your last suggestion is very nice. I will try to do that now.

@joelfrederico
Copy link
Contributor

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
https://github.com/zeromq/pyzmq/blob/master/zmq/utils/win32.py

Signals are very different on Windows!

@tacaswell
Copy link
Member

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 FuncAnimation as the loop that drives your experiments (which is how I think you are doing it if you rely on the blocking behavior of plt.show to run experiments serially). It is better for you to have your own code block and then explicitly spin the Qt event loop (likely on a clock so it remains responsive, see https://github.com/NSLS-II/bluesky/blob/7eea5f30d5e3a4c386585e23d17048b16cdac776/bluesky/utils.py#L747-L846 for how we do it at my day-job).

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

@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 plt.pause() up to the moment matplotlib got updated above 2.0.0 (actually, something happened with Pyside, I believe) and stopped catching KeyboardInterrupts, and when the plot windows started blinking frantically with no way to stop this =))

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 plt.show(blocking=True) since neither I can sleep in the main thread nor do something useful since the plotting will hang. You can find our code here: https://github.com/vdrhtc/Measurement-automation

How do you wait for the end of the acquisition? I couldn't find this in your code quick enough

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 28, 2019

@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 qApp.exit() as you suggested, but unfortunately I had to create a class field named interrupted to discern between Ctrl+C and window closing. That's unsafe, but I don't yet fully got the architecture to put it elsewhere. Maybe some advice?

@joelfrederico
Copy link
Contributor

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 interrupted. If you have a signal handler, closing the window doesn't raise a signal, so discerning between ctrl-c and close shouldn't be an issue.

@tacaswell
Copy link
Member

@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:

  • arrange to pass an Event into your acquisition code
  • have it set that event when done
  • set up a thread that waits on that event and when it is set kills the GUI main loop
  • start the gui main thread and let it block

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.

@tacaswell
Copy link
Member

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 Event dance (which is what bluesky/bluesky#1125 is implementing).

On a bit more consideration, you should do what we are moving towards, not what we are doing now.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 29, 2019

@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 SIGINTHandler is added; it creates socket.socketpair() for Python to write into with signal.set_wakeup_fd() and makes Qt listen to it (more detailed explanation may be found on StackOverflow, third answer).

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?

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 29, 2019

@tacaswell, thank you for the code! I think that the idea is very similar to what I wanted to do, because plt.show(blocking = True) pretty much just calls qApp.exec_() and blocks on it. So the problem there is only that vanilla implementation doesn't feel SIGINTs.

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 qApp.exec_() Therefore I opened #13315 to fix (partly) plt.pause() with its start_event_loop() which then can be exited without killing everything. And I managed to get it to work, finally, using the hack from here!

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 =)

@tacaswell
Copy link
Member

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).

@github-actions
Copy link

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!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 30, 2023
@github-actions github-actions bot added the status: closed as inactive Issues closed by the "Stale" Github Action. Please comment on any you think should still be open. label Jun 30, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI: Qt status: closed as inactive Issues closed by the "Stale" Github Action. Please comment on any you think should still be open. status: inactive Marked by the “Stale” Github Action
Projects
None yet
Development

No branches or pull requests

6 participants