conda-forge / pyside2-feedstock

A conda-smithy repository for pyside2.
BSD 3-Clause "New" or "Revised" License
17 stars 19 forks source link

Upstream bug breaks interactive plotting in Matplotlib, what to do? #226

Closed hoechenberger closed 3 months ago

hoechenberger commented 4 months ago

Hello! So there's an upstream bug in PySide6 (which has been present since the PySide2 days) that prevents interactive Matplotlib plots from showing up:

https://github.com/matplotlib/matplotlib/issues/15359

Importantly, PyQt6 doesn't have this issue.

The problem is due to PySide6's not implementing the PyOS_InputHook API. We'd opened a bug report upstream many months ago, but nothing's really happening.

Since PyQt6 is not available for conda-forge and we have to stick with PySide6, interactive plotting is now broken for us.

A fix was proposed but never merged, since apparently the CI runs wouldn't finish on Windows.

I was wondering if we could apply this patch at least for non-Windows platforms here, since there's no activity anymore upstream? Of course, the easiest for me (as a user) would be to just use PyQt6 instead, but there are no conda-forge packages. This is a really bad situation to be in now, as it means I'm stuck with either a broken PySide6 or an (old) PyQt5 for the time being...

hmaarrfk commented 4 months ago

Is there a way to recreate a reproducer without a matplotlib dependency?

I know it’s a lot of work, but typically getting to a common knowledge base with upstream developers is important. I feel like matplotlob might be too big of an ask to help them troubleshoot it.

A follow up, would this be introducing a new function to the public API? If so. That is a really big ask for us to do and generally not very wise unless it is merged into upstream.

hoechenberger commented 4 months ago

@hmaarrfk This is the patch that was proposed:

https://codereview.qt-project.org/c/pyside/pyside-setup/+/454254/2/sources/pyside6/libpyside/pyside.cpp

hmaarrfk commented 4 months ago

Even I, expert as I am in my usage of matplotlib and python, cannot understand the system wide implications of that patch at a glance.

I am somewhat sympathetic to the qt developers since the minimum reproducible error code doesn’t really translate down to their software directly.

Without that, I can see even more delays in getting your issue resolved and ultimately your patch merged in. They could be trying to go in a different direction. One cannot know for sure.

I’ve hit a few bugs like this in Qt and Pyside myself. I wish there was a way to “live patch” things in python to avoid this exact problem you are facing

hoechenberger commented 4 months ago

Thanks @hmaarrfk, I appreciate your sympathy!

This means that the only workaround for us and our users for now is to use PyQt5 (as PyQt doesn't have this problem), since PyQt6 isn't available from conda-forge. This is really disappointing 😔 Not blaming anyone here, mind you, but it's just not a nice place to be in

hmaarrfk commented 4 months ago

I guess it would just be good if these calls:

import matplotlib
import matplotlib.pyplot as plt

matplotlib.use("QtAgg")  # will use PySide6
plt.ion()  # turn on interactive mode
plt.plot(1)  # should open a figure, but this does not work

were not import matplotlib but rather import PySide6.

Is there anyway you can provide the "interactive part" of the following code:

from PySide6.QtWidgets import QApplication, QLabel, QMainWindow

app = QApplication([])
window = QMainWindow()
label = QLabel("Hello World", window)
window.setCentralWidget(label)
window.show()

I use ipython when I do these things, so I use %gui qt but I understand that this is different than the codepath you wish to use for non-ipython codepaths.

tacaswell commented 4 months ago

See https://matplotlib.org/stable/users/explain/figure/interactive_guide.html#input-hook-integration for more details.

To be clear, interactive figures should still work with plain python (well, readline...I suspect (but have not tested) that if you use the older IPython + readline prompt pyside will also be missing the input hook) if you use plt.show(block=True) (that is a window will come up and you can interact with it). What does not work is what mpl calls "interactive" via plt.ion() where we expect the shell to be running an input hook so you can type new commands at the terminal + have a live figure. As noted plt.ion() does work with IPython because they provide their own input hook.

I think that @hmaarrfk 's second example is enough to show the problem (that example works with PyQt6 do not expect it to work with pyside6). To report upstream would be better to add a signal/slot hookup to be very sure that the eventloop is/is not running.


I'm not actually sure what the story with input hooks and the new prompt in py313 as it is a c-level thing but upstream adopted the pypy prompt which is written in Python....

hmaarrfk commented 4 months ago

ps. i did take some time to look at proposed upstream patch, but it is quite invasive in my mind as a "conda-forge" patch.

Please do take the "demo" i wrote and expand on it. Fingers crossed it helps Qt upstream developers!

larsoner commented 3 months ago

Please do take the "demo" i wrote and expand on it. Fingers crossed it helps Qt upstream developers!

Your snippet is already enough actually. On Linux for me it produces:

image

You can see the display is whatever was on the screen before the window popped up (Chrome). Adding an app.exec() you see the display update properly (but block the interpreter):

image

If you replace the PySide6 import with PyQt6:

from PyQt6.QtWidgets import QApplication, QLabel, QMainWindow

it just works even without the app.exec:

image

hmaarrfk commented 3 months ago

Your snippet is already enough actually. On Linux for me it produces:

Great. I just tried to simplify things with my kno2ledge. But I feel like I open enough issues on PySides bug tracker. And since you are all capable I'll let you take the lead.

My fear is that i won't be fast enough to respond to their followup questions if I take the lead.

tacaswell commented 3 months ago

https://github.com/python/cpython/pull/119843 <- the new python repl does respect the inputhook (if it gets implemented).

larsoner commented 3 months ago

Now that the 40-line upstream PR has landed in 6.8 and 6.7.3 branches, we can just wait for 6.7.3 to come out. Alternatively, I'm happy to add this as a patch for another 6.7.2 build... a bit weird perhaps but would allow us to get the fix used (and tested) in the wild sooner rather than later!

hmaarrfk commented 3 months ago

Amazing work working with them to get it fixed for windows too!!!

hmaarrfk commented 3 months ago

I'm much more ok backporting merged fixed for issues we have brought up.

Do make the PRs. The CIs are somewhat broken today though ;)

hmaarrfk commented 3 months ago

Am I reading the patch correctly. It seems to be simply overwriting any existing PyOS_inputhook. Is that correct? What if some other event system is trying to use this?

hmaarrfk commented 3 months ago

In cpython 3.10 the following conditions are used in the TK example

IMG_9923

larsoner commented 3 months ago

Am I reading the patch correctly. It seems to be simply overwriting any existing PyOS_inputhook. Is that correct? What if some other event system is trying to use this?

Yep kind of surprising on the face of it but I think PyQt all the way back to at least PyQt5 and PyQt4 also did it this way for years (and PyQt6 still does) without people complaining much AFAIK, so in that sense it's probably safe enough.

hmaarrfk commented 3 months ago

Done in https://github.com/conda-forge/pyside2-feedstock/pull/230

tacaswell commented 2 months ago

Using more than one GUI framework at a time is generally not safe (a fun detail is that pyqt and pyside both wrap the same underlying qt so so long as they are built against the same libqt you can use them both in the same Python process as long as you do not cross the streams on the Python side 🤯 ) so that they may stomp on each other is probably OK.

That said, Python is careful about it: https://github.com/python/cpython/blob/d69529d31ccd1510843cfac1ab53bb8cb027541f/Modules/_tkinter.c#L3371-L3390

hmaarrfk commented 2 months ago

Using more than one GUI framework at a time is generally not safe

The general problem with python is that some application that can target multiple frameworks try to "import" things as a test that they exist.

But I think the code is triggered on the "application running"