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

Can't interrupt start_event_loop() at backend_qt5.py::FigureCanvasQt #13315

Closed
vdrhtc opened this issue Jan 29, 2019 · 13 comments
Closed

Can't interrupt start_event_loop() at backend_qt5.py::FigureCanvasQt #13315

vdrhtc opened this issue Jan 29, 2019 · 13 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 29, 2019

Bug report

Bug summary (suggested fix is in the very bottom)

This internal method is used in plt.pause() and BlockingInput class (I don't know what the latter does).

The problem is approximately since Matplotlib 2.0.0 (but I don't know what exactly has changed). In the current implementation, the method execution can't be interrupted easily since it is based on event_loop.exec_() (internally C++) function that locks out the Python interpreter and prevents it from processing SIGINT as KeyboardInterrupt.

The problem is clearly visible when you run some kind of animation (see example below), since the SIGINT is getting understood as KeyboardInterrupt by the target function of the Timer (which is in Python and thus the interpreter gets to run from time to time in the event loop). However, this exception in the timer target can't stop the even loop, and it continues.

In overall, this problem actually renders plt.pause() practically unusable (this, and the window focusing issues). So either we should deprecate plt.pause() or fix it, as I suggest below.

Code for reproduction

%pylab qt5
import numpy as np
import matplotlib.pyplot as plt
from matplotlib.animation import FuncAnimation
from matplotlib._pylab_helpers import Gcf

stopped = False
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)
    if frame == 2*np.pi:
        stopped = True
    return ln,

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

# wait for the plotting/underlying process to end
try:
    while not stopped:
        manager = Gcf.get_fig_manager(fig.number)
        manager.canvas.start_event_loop(1)  
except KeyboardInterrupt:
    plt.close(fig)
    print("KeyboardInterrupt caught")
except AttributeError:
    print("Figure closed from GUI")

Actual outcome

On pressing stop button in Jupyter:

And then the waiting cycle continues with no error. The exceptions on the Qt event loop get caught somewhere inside?

Expected outcome

Clean stop and closed figure on the except KeyboardInterrupt clause.

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.

Suggested fix

This problem has annoyed me for quite some time, and I am very glad that it seems that there is the solution. It is very closely related to #13302, and is solved similarly (using signal.set_wakeup_fd() and socket.createsocketpair()).

The problematic method should be rewritten as follows:

def start_event_loop(self, timeout=0):
    if hasattr(self, "_event_loop") and self._event_loop.isRunning():
        raise RuntimeError("Event loop already running")
    self._event_loop = event_loop = QtCore.QEventLoop()
    if timeout:
        timer = QtCore.QTimer.singleShot(timeout * 1000, event_loop.quit)

    SIGINTHandler(qApp)
    interrupted = False

    def sigint_handler():
        nonlocal interrupted
        event_loop.quit()
        interrupted = True

    old_sigint_handler = signal.getsignal(signal.SIGINT)
    signal.signal(signal.SIGINT, lambda sig, _: sigint_handler())

    try:
        event_loop.exec_()
    finally:
        signal.signal(signal.SIGINT, old_sigint_handler)
        if interrupted:
            raise KeyboardInterrupt

SIGINTHandler class may be found in #13306

@vdrhtc vdrhtc changed the title Can't interrupt start_event_loop() at backend_qt5.py::FigureManagerQt Can't interrupt start_event_loop() at backend_qt5.py::FigureCanvasQt Jan 29, 2019
@tacaswell
Copy link
Member

I can reproduce some of these issues from a terminal

import numpy as np
from matplotlib.animation import FuncAnimation
from matplotlib._pylab_helpers import Gcf

fig, ax = plt.subplots()
xdata, ydata = [], []
ln, = plt.plot([], [], 'ro')
stopped = False

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

def update(frame):
    global stopped
    xdata.append(frame)
    ydata.append(np.sin(frame))
    ln.set_data(xdata, ydata)
    if frame == 2*np.pi:
        stopped = True
    return ln,

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

# wait for the plotting/underlying process to end
try:
    while not stopped:
        plt.show(block=False)
        manager = Gcf.get_fig_manager(fig.number)
        manager.canvas.start_event_loop(1)  
except KeyboardInterrupt:
    plt.close(fig)
    print("KeyboardInterrupt caught")
except AttributeError:
    print("Figure closed from GUI")

although, oddly when I put the global in sigint stops doing anything.

I it is possible that this is due to a change in default signal registration by pyqt rather than an intentional change in Matplotlib.

One change that may be relevant here is 872675c which switched to using a stand-alone event loop in plt.pause (rather than starting the qApp event loop), but this means

qApp.lastWindowClosed.connect(qApp.quit)
no longer helps us abort early. It may also be that qApp.exec_() sets up the requisite signal handling but the QEventLoop does not.

In overall, this problem actually renders plt.pause() practically unusable (this, and the window focusing issues). So either we should deprecate plt.pause() or fix it, as I suggest below.

While this is very annoying for your use case, it still is plenty useful. pause is a pretty core part of the API and can not be deprecated.

I don't understand why we need the complexity of SIGINTHandler(qApp)? It seems the behavior we want is to kill the event loop and return control to the terminal. We can then go back to relying on whatever is doing the event loop integration between the kernel and Qt normally to keep doing it's job. If the user has a full Qt app then they never should be calling plt.pause because the Qt event loop is always running in that case.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 30, 2019

Yes, I agree that it is somewhere in the transition from PyQt4 to PyQt5. You probably should try the example from Jupyter to understand more precisely what I mean and my motivation.

I am not sure what is the purpose of plt.pause() except than to wait until something happens AND do not hang the Qt windows (cause otherwise one would use sleep()). This implies that one is not running the code on the Qt event loop and probably is in the interactive mode. For example, you are running several simulations and dynamically plot the results: you may want to skip one of them and continue with the others (and do not close the previous plot windows). In Jupyter you can do this with a button press, in the terminal with a single Ctrl+C.

I think the problem with the approach of sending SIGTERM and killing Qt (taking down the Python process as well) is that a matplotlib window is treated like a standalone Qt application -- and it isn't. As @joelfrederico said, the end user doesn't usually even know what an event loop is! In my opinion, SIGINT should always return control to Python and raise there the pythonic KeyboardInterrupt that may be caught and processed. To do this it seems that, unfortunately, we need that complex class (may be it's possible to simplify it).

You have mentioned qApp.lastWindowClosed.connect(qApp.quit). You mean that this thing does not abort the additional QEventLoop when the window is closed? That's weird if so. I want also to complain that on plt.show(block=True) it stops working from time to time on my Linux computer, and I've just also noticed it in Windows: the lastWindowClosed signal does not always emit? I've even said about this problem in the first issue #13302 (process does not exit on window closing), but now I can't reproduce this. I tried to look at the window list from Qt with qApp.allWindows() and found out that it's not clearing after windows.close(). This may be changed by self.window.setAttribute(QtCore.Qt.WA_DeleteOnClose, True) in FigureManagerQt, but does not help, of course.

I caught it:
https://www.youtube.com/watch?v=79pLM04NayI

@tacaswell
Copy link
Member

You probably should try the example from Jupyter to understand more precisely what I mean and my motivation.

I do not think we should officially support running GUI backends from a jupyter notebook. It only works if and only if you are running the kernel on the same machine that you are running the web browser on. This means you lose the biggest benefit of notebooks (being able to run them remotely).

We should focus on getting the behavior in the IPython terminal correct (it will be easier to debug without one more server involved). That will hopefully carry over to the notebook case.

I think the problem with the approach of sending SIGTERM and killing Qt (taking down the Python process as well)

This is a misunderstanding of what quitting qApp does, it does not kill the whole process, it stops the Qt event loop and returns control to the code that called exec_

is that a matplotlib window is treated like a standalone Qt application

The Qapplication object is a singleton at the c++ level, (if you have more than one instance of QApplication in your process you will get segfaults 😉 ). All of the plot windows (and all other Qt windows you pop up in the same process) are part of the same application from Qt's point of view.

As @joelfrederico said, the end user doesn't usually even know what an event loop is!

fair, the edge we need to walk here is to both make it so if users do not need to know about event loops they do not have to, but also to not re-invent a lot of semantics / functionality so that a user that does know about event loops will be surprised / frustrated.

SIGINT should always return control to Python and raise there the pythonic KeyboardInterrupt that may be caught and processed.

I agree with the first part, but am 75/25 on the second part. Has it historically worked that way? A reasonable way to get the right behavior is to have the custom signal handler first interrupt Qt event loop and then call the original handler (basically super() but for signal handlers).

You mean that this thing does not abort the additional QEventLoop when the window is closed?

It does if you pause and then close all of the windows it does not exit the event loop we are running, it should to be consistent with plt.show(block=True) when in non-interactive mode (and my vague memory of how it used to work)

I am not sure what is the purpose of plt.pause()

The purpose of plt.pause is to run the GUI event loop for some period of time, the issue here is that early exit is not behaving correctly anymore (either due to closing all the open windows or sigint).

I tried to look at the window list from Qt with qApp.allWindows() and found out that it's not clearing after windows.close().

Non-visible windows may still existing in memory. If you still have a hard-reference to the Figure it will have a hard reference to the Qt objects which means they are still alive (if made non-visible and removed from pyplot's state).

@tacaswell
Copy link
Member

@vdrhtc Sorry if I am reading as hostile, there is a lot of detail / complexity in this space and I am trying to be direct and efficient in this conversation. I am very excited someone wants to talk about this stuff 😄 .

@joelfrederico
Copy link
Contributor

I need to double-check how signals are handled in Qt, but I believe the behavior on SIGINT is currently to return from exec_()without raising or calling anything. Changing that would change the API's behavior, which then gets into the decision of whether that's good or bad. That's the reason I only restored the previous signal handler instead of doing all of this myself. Changing the handler within show() was pretty clearly an unintended side effect, but the full contract for SIGINT behavior is unclear to me.

Should it call the SIGINT handler on a ctrl-c during show()? This is tricky so I left it alone. Does a user expect a SIGINT to call the Python signal handler the next time any Python code is called? This means you have no idea which Python will be next. It could be any one of your or matplotlib's Python signals or slots. (The default behavior after that happening is a KeyboardInterrupt, and the default Python behavior is to terminate on an unhandled exception... I get fuzzy about what happens after Qt has called back into a Python signal or slot function that returns with an uncaught exception...) Should it interrupt the Qt event loop even if no Python is encountered or not? (Technically there are very limited things a signal handler can do, so this would be the self-socket trick or something similar.) I don't know.

I moved away from running plots while other Python code is running. The real issue is that Python is not multithreaded. So the way interactive matplotlib works is to flicker between terminal and Qt execution. That's a hit in performance no matter how you slice it, so I opted to take a different approach and all this became moot to me.

I agree though, if it should do anything, it should call the original SIGINT handler and not just raise a KeyboardInterrupt. We stored the original handler in order to reset it later, so that's easy enough to do. If it's the default handler, calling it should raise a KeyboardInterrupt.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 30, 2019

@tacaswell, it's all right, we are just getting to know each other =)

I do not think we should officially support running GUI backends from a jupyter notebook ... (being able to run them remotely)

Well, the notebooks do not survive a simple page update (regarding standard output, the [*] mark and %pylab notebook plots) so dropping an SSH tunnel ruins the experience.

Conversely, I would want to have on my local PC an environment like Matlab, or Mathematica, or RStudio, if you wish. JupyterLab is being developed for this, and I am sure it still has %pylab qt5 magic in it. What is the alternative? I know Spyder, but it looks the same to me with the same problems.

I agree that we should test in the terminal first, but I can't agree with dropping %matplotlib qt5 support.

This is a misunderstanding of what quitting qApp does, it does not kill the whole process, it stops the Qt event loop and returns control to the code that called exec_...

No-no, I meant

signal.signal(signal.SIGINT, signal.SIG_DFL)
which indeed leads to killing Python.

The Qapplication object is a singleton...

Yes, I understand otherwise I wouldn't be talking about lastWindowClosed (btw, did you watch my video??). I am only saying that same line prioritizes Qt over my script which has called it, and my program dies inside the qApp.exec_() if I want to interrupt it/or I can't interrupt it at all like in case.

...call the original handler...

Oh, yes, of course, that's right, I'll do that.

It does if you pause and then close all of the windows...

I am a bit lost in this sentence. I have tried to plt.pause() for 10 seconds and close the single window -- plt.pause() does not exit until full 10 seconds elapse. And it's not that bug I'm pursuing, plt.close(block=True) exits on immediately on close.

The purpose of plt.pause is to run...

Sorry, but than probably it should be called plt.run()! You are telling me what it's doing and not why would it do it!

Non-visible windows may still existing in memory.

I have a PR ready to deal with that as I said, should I post it?.. I feel that's not the intended behaviour.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 30, 2019

@joelfrederico

I need to double-check how signals are handled in Qt

May be it is important when we are developing a standalone application using matplotlib, but then it probably won't use plt.show() like @tacaswell's bluesky -- it just calls the Qt internals. But if I call plt.show() it probably means I haven't ever heard about Qt and do not know how it works, I only want to see some plot (I am probably in the interactive mode, or at least I am used to it). This leads us to second thought:

Should it call the SIGINT handler on a ctrl-c during show()? ...

I think when it comes to SIGINT during an interactive session, the default behaviour should be to stop at all costs and return to the shell so the user can issue other commands. If it is not interactive, it still should behave familiar. The users should feel as they were interrupting, for example, a script with a simple for-loop: may be an exception, if they do not explicitly catch it, but no delays.

PyQt5 can't handle (fatal, what does this mean?) exceptions, as it seems from here:

try:
self.draw()
except Exception:
# Uncaught exceptions are fatal for PyQt5, so catch them instead.
traceback.print_exc()
finally:
self._draw_pending = False

I opted to take a different approach

And what's that, if that's not a secret?

I agree though, if it should do anything, it should call the original SIGINT handler

Yes, I will do that at least

@joelfrederico
Copy link
Contributor

Unfortunately, my different approach is proprietary. ;)

If you're going to call matplotlib.pyplot.show(), you may have actually implemented a full app using only matplotlib API's. See https://matplotlib.org/users/event_handling.html. So right off the bat, actually yes, I have heard of people "developing a standalone application using matplotlib". Kind of surprising to me personally, but it happens.

That's exactly what I mean, what does "can't handle" and "fatal" mean? I don't know. Presumably this isn't undefined behavior, I just haven't done the research to know what the well-defined behavior is. Probably involves terminating the app somehow?

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Jan 31, 2019

Unfortunately, my different approach is proprietary. ;)

Okay... But are you still on Python and mpl?

Probably involves terminating the app somehow?

I tested with this:

from PyQt5 import QtCore, QtWidgets
qApp = QtWidgets.QApplication(["lol"])
window = QtWidgets.QWidget()
def raise_error():
    raise ValueError("lol")
window.show()
timer = QtCore.QTimer.singleShot(1000, raise_error)
qApp.exec()

In Jupyter with no magic it spits out exception but does not stop, window is still closeable and qApp.exec() returns 0 after closing. Far from fatal!

When run as a script in Pycharm, it prints the traceback and gets killed:
Process finished with exit code 134 (interrupted by signal 6: SIGABRT). Pretty fatal

@joelfrederico
Copy link
Contributor

joelfrederico commented Jan 31, 2019

Ah, "proprietary" unfortunately means we've reached the end of the discussion.

What happens in a vanilla Python shell...? Ooo badness... it's an unhandled abort. My hunch is that Python calls abort() when it gets to the point of returning to Qt.

@tacaswell
Copy link
Member

which indeed leads to killing Python.

This may be a change in PyQt under us. The recently have recently gone through a process of making it much less forgiving of exceptions.

And it's not that bug I'm pursuing, plt.close(block=True) exits on immediately on close.

I think it is closely related.

Sorry, but than probably it should be called plt.run()! You are telling me what it's doing and not why would it do it!

It pauses your code and gives the user a chance to poke around.

Kind of surprising to me personally, but it happens.

If you use the pan/zoom functionality you are using that functionality. You can build pretty cool stuff that is cross-platform and cross-framework without too much extra hassle.

I agree that we should test in the terminal first, but I can't agree with dropping %matplotlib qt5 support.

I think this is critical for use in IPython (although, the most important thing it does (registering the inputhook) should happen auto-magically...

Sorry, don't have time for more in-depth comments tonight.

@vdrhtc
Copy link
Contributor Author

vdrhtc commented Feb 1, 2019

@joelfrederico

Ah, "proprietary" unfortunately means we've reached the end of the discussion.

Okay (x2) 😅

What happens in a vanilla Python shell...?

Aborted (core dumped)

I like this error, that's oldschool

@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

4 participants