-
-
Notifications
You must be signed in to change notification settings - Fork 18.9k
BUG: upgrade to PyQt6 to fix arm64 docker build #62176
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
BUG: upgrade to PyQt6 to fix arm64 docker build #62176
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Hi @mroeschke and @rhshadrach, can any of you review this PR when you have the time? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you're dropping PyQt5 support here, is that right?
======================================================================================================================================== ================== =============== ============== | ||
Dependency Minimum Version pip extra Notes | ||
======================================================================================================================================== ================== =============== ============== | ||
`PyQt4 <https://pypi.org/project/PyQt4/>`__/`PyQt5 <https://pypi.org/project/PyQt5/>`__/`PyQt6 <https://pypi.org/project/PyQt6/>`__ 5.15.9 clipboard Clipboard I/O |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're listing the minimum version as 5.15.9 while changing the minimum-version action to PyQt6. In addition, it seems like PyQt4 should no longer be listed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change just extends the support to PyQt6
, while keeping PyQt4
and PyQt5
compatibility intact.
it seems like PyQt4 should no longer be listed?
It still supports PyQt4
, so I think it should remain, but if you want me to de-list it I will do it.
You're listing the minimum version as 5.15.9 while changing the minimum-version action to PyQt6
The goal of this change was just to replace the PyQt5
entry in requirements-dev.txt
, the changes in the CI dependencies were unintentional, but required to test the PyQt6
changes in CI.
Pandas remains compatible with PyQt4
and PyQt5
. The problem is that it is not being tested in CI anymore. Maybe we can list PyQt4
, PyQt5
and PyQt6
in the CI dependencies and parametrize the qt
clipboard tests to test the different PyQt
versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that PyQt4 is not available on conda-forge and I also can't download it from pypi. I will remove it from install.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal of this change was just to replace the PyQt5 entry in requirements-dev.txt, the changes in the CI dependencies were unintentional, but required to test the PyQt6 changes in CI.
We have multiple setup actions to support this - actions-311-minimum_versions.yaml
could test with PyQt5 whereas actions-311.yaml
could test with PyQt6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
I was avoiding creating a big discrepancy between the CI jobs and ended up installing qtpy
, PyQt5
and PyQt6
in all environments and parametrized the tests to use each of the Qt bindings. The problem with this approach is that the qapp
fixture from pytest-qt
doesn't handle the clipboard properly when the test is run with PyQt5
, because qapp
uses PyQt6
to create the clipboard, and, for some reason, it can't read what's written with PyQt5
.
Anyway, the parametrization approach works but creates complexity. The advantage of this is that every CI job tests PyQt6
and PyQt5
.
Splitting the CI is simpler and doesn't complicate the clipboard tests. I will leave it up to you the choice of:
- Test
PyQt5
inactions-311-minimum_versions.yaml
and testPyQt6
in the rest. - Leave the clipboard tests parametrized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see the downside of having some CI jobs test PyQt5 and others PyQt6. cc @mroeschke for any thoughts here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the test parametrization and restored the actions-311-minimum_versions.yaml
file to its original state, where it tests PyQt5
while the other CI jobs test PyQt6
.
When PyQt6 is uninstalled, it is still importable, but `PyQt6.QtWidgets` is no longer available.
This comment was marked as resolved.
This comment was marked as resolved.
The CI for `311-minimum_versions` tests the clipboard with `PyQt5`.
Pyside2 support wasn't added because `pytest-qt>=4.5` doesn't support it. Reference: <https://github.com/pytest-dev/pytest-qt/blob/master/CHANGELOG.rst#450-2025-07-01>
Set explicit system dependencies on Linux. Make it explicit that Qt bindings support are limited on Linux and states that it works best on Windows and MacOS. The Qt support on Linux isn't reliable because it depends on an event loop and user-input events to modify the clipboard. [1] [1]: <https://doc.qt.io/qt-6/qclipboard.html#notes-for-x11-users>
sudo apt-get install xsel | ||
|
||
Alternatively, you can install one of these Python packages, | ||
but their support is limited on Linux and work more reliably |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Qt clipboard documentation states that:
the X11 clipboard is event driven, i.e. the clipboard will not function properly if the event loop is not running. Similarly, it is recommended that the contents of the clipboard are stored or retrieved in direct response to user-input events, e.g. mouse button or key presses and releases. You should not store or retrieve the clipboard contents in response to timer or non-user-input events.
The way that pandas currently uses the clipboard on Linux doesn't trigger any event, which doesn't trigger any clipboard modification.
Since I am on Linux (Wayland), here is an example of how it is not reliable:
import sys
from PyQt6.QtWidgets import QApplication
from PyQt6.QtCore import QTimer, QEventLoop
app = QApplication(sys.argv)
app.clipboard().setText("Hello World!")
# CI tests this
print("clipboard before:", app.clipboard().text())
# clipboard before: Hello World!
# creates event loop and process events
loop = QEventLoop()
QTimer.singleShot(100, loop.quit)
loop.exec()
print("clipboard after :", app.clipboard().text())
# clipboard after :
Although the Qt bindings aren't reliable, the subprocess calling wl-copy
works fine.
Hi @mroeschke, since this PR introduces a lot of changes in CI:
I want to know what do you think of these changes and if this PR needs any further modifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this so far, but I am hesitant on supporting pyqt6 just for the Dockerfile, which unfortunately hasn't seen consistent maintenance over the years.
Additionally, our clipboard support was vendored from pyperclip, which I suppose didn't have much consideration for pyqt6 (maybe it didn't exist yet?)
If we were to make any improvements here, I would like to see pandas instead migrate to a different, non-vendored dependency for clipboard support instead, xref #39834, or just deprecate support for this "IO" method if it's causing installation issues. I don't think we receive many bug reports or feature requests for this method either.
Thanks for the review.
I completely agree with you. Just to add pyqt6 support, I had to introduce several dependency changes in CI which I didn't mean to.
Both approaches seem valid, I would be a little hesitant in using spyoungtech/pyclip because its last update was 2 years ago. I am particularly fond of the deprecation approach, since it's not difficult to use # t.py
import pandas as pd
import io
import pyclip
df = pd.DataFrame(dict(a = [2, 5], b=[3, 4]))
with io.StringIO() as buf:
df.to_csv(buf, sep="\t")
pyclip.copy(buf.getvalue()) $ python t.py
$ wl-paste
a b
0 2 3
1 5 4 Thanks for the review again! Closing. |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.docker build --platform linux/arm64 -t pandas-dev .
if you are on anx86_64
system, but be warned that the build takes very long._import_module
auxiliary function.