Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#1635 closed enhancement (fixed)

Replace PyQt5 with PySide2

Reported by: Tristan Croll Owned by: pett, goddard
Priority: moderate Milestone: 1.2
Component: UI Version:
Keywords: Cc: Greg Couch, Scooter Morris
Blocked By: Blocking:
Notify when closed: Platform: all
Project: ChimeraX

Description (last modified by Tom Goddard)

We currently require a paid commercial license for PyQt5 to be able to sell ChimeraX to companies. With PySide2 this would not be needed. The cost of PyQt5 is not high, I think about $2000 per year for 4 developer licenses. So the real question should be which Qt Python bindings will have better support and be better maintained in the future. PySide2 is an official part of the Qt project, PyQt5 is a one person company. It is unclear which will have a better future.

More an FYI as a suggestion: I understand from meetings over the last few months that PHENIX, CCP4 and CCP-EM are all actively working on migrating to Python 3 and have all decided upon using PySide2 rather than PyQt5 for their GUIs. The two packages appear to be nearly identical in their functionality and APIs, but the former is a free license and apparently has more support from Qt themselves.

Is migration to PySide2 something that might be feasible/desirable? As a plugin developer I'm still trying to find out exactly where I stand with respect to PyQt5 licensing requirements, so that would be one less thing to worry about.

Change History (38)

in reply to:  1 ; comment:1 by tic20@…, 7 years ago

OK, on the license front: I've just had a formal confirmation from 
Riverbank that as long as the plugin can only be used within ChimeraX 
(that is, can't be used as a stand-alone package) then plugin developers 
don't need to buy a license. So never mind that.

On 2019-01-24 10:49, ChimeraX wrote:

comment:2 by Tom Goddard, 7 years ago

Cc: chimera-programmers added

Greg looked at PySide2 last week and got ChimeraX to start and found several problems. We will probably switch to PySide2 in the future but probably will wait until it is in better shape. Currently we pay for PyQt5 commercial licenses for our developers, a step that was deemed necessary for selling ChimeraX to companies.

Begin forwarded message:

From: Greg Couch
Subject: P.S: PyQt 5.12
Date: January 14, 2019 at 3:24:47 PM PST
To: Chimera Staff <chimera-staff@…>

Forgot to add, that while I was waiting for the Qt to compile, I tried using "Python for Qt" instead of PyQt. There are a lot of rough edges, but I got ChimeraX to startup. The main difference is importing from PySide2 instead of PyQt. PySide2 is missing support for web brower history, is not as flexible in some of the function arguments (think C++ implicit casts), and doesn't support some function arguments. As a result I filed 3 bugs. Except for the web history, I am getting notifications about work on the bugs. Christian Tismer (of Stackless Python) is one of the Qt employees working on the bug fixes!

If we want to use PySide2 "soon", it would be worth trying more parts of the ChimeraX GUI to identify missing functionality and file more bugs :-)

-- Greg

comment:3 by Greg Couch, 7 years ago

The web browser history bug is https://bugreports.qt.io/browse/PYSIDE-906. As you can see, it appears to require a change to shiboken, PySide2's C++ wrapper generator, and it unclear when that will happen.

comment:4 by Greg Couch, 7 years ago

The web browser history bug is closed, but the fix was done after Qt 5.12.1 came out. So will have to wait until 5.12.2 to test.

comment:5 by Greg Couch, 5 years ago

Milestone: 1.2

Under consideration for 1.2.

comment:6 by Tom Goddard, 5 years ago

I tried PySide2 in ChimeraX and got it to work without errors in about 2 hours, fixing about 5 issues. I need to more thoroughly test ChimeraX user interfaces with PySide2 to see if there are any big problems. One concern is that in my initial port I got many crashes from PySide2 such as this

2 libsystem_c.dylib 0x00007fff72ffa808 abort + 120
3 org.python.python 0x0000000101322b81 fatal_error + 753 (pylifecycle.c:2157)
4 org.python.python 0x0000000101322883 Py_FatalError + 19
5 libshiboken2.abi3.5.15.dylib 0x000000010b8feaed SetError_Argument + 157
6 QtWidgets.abi3.so 0x0000000117feb7c6 Sbk_QApplicationFunc_event(_object*, _object*) + 150

I have the feeling that some of those crashes were merely bad arguments to PySIde2 calls. But I may be wrong, it could have been a conflict with PyQt5 (which I left installed), or a result of my hacky rename sys.modulesPyQt5 = PySide2 which had some mysterious problems.

If PySide2 does crash with bad arguments instead of giving the expected non-fatal Python traceback, that would be a big problem.

comment:7 by Tom Goddard, 5 years ago

Here are issues I encountered switching from PyQt5 to PySide2. These changes allowed ChimeraX daily build to work with PySide2, opening atomic structures and volumes and doing basic operations with toolbar, model panel, log, commands, open panel, save panel. Still need to test all user interfaces.

1) PyQt5.QtCore.PYQT_VERSION_STR and PyQt5.QtCore.QT_VERSION_STR become PySide2.version and PySide2.QtCore.version used in bug_reporter and ui.widgets.htmlview.py
2) QImage.bits() is a Python memoryview in PySide2 that does not have an asstring() method, used by our qimage_to_numpy(). Has a tobytes() method.
3) qurl.url(qurl.None_) gives an error but qurl.url() works, used by the log panel and ui.widgets.htmlview.py. Need to look at what None_ is doing here.
4) QPushButton.clicked signal handler takes an QEvent in PyQt5, takes no argument in PySide2, effects Model Panel.
5) QObject.findChild() does not have an options argument in PySide2 even though Qt and PyQt5 does, we use option to only look at immediate children. Qt bug report from Jan 2019 describes this, not fixed: https://bugreports.qt.io/browse/PYSIDE-905. Easy to work around by looping over children.
6) QVariant is not in PySide2. We use it in ui.widgets.ItemTable. Used by rotamers gui. Don't know how to fix this.
7) QAction.triggered signal handler takes an argument in PyQt5, but no argument in PySide2, used in ui.widgets.tabbedtoolbar and a hundred other places, easy fix. It is trickier, in PySIde2 if the callback takes one argument then it is called with a bool argument, otherwise it is not called with an argument!
8) QPainter is not a context manager (no enter method) in PySIde2, used in ui.widgets.tabbedtoolbar, easy fix.

Last edited 5 years ago by Tom Goddard (previous) (diff)

comment:8 by Tom Goddard, 5 years ago

More issues.

9) QAction.setShortcut(keyseq) crashes when ChimeraX hands it a Qt.Key instead of a KeySequence. PyQt5 must be doing an implicit cast from Key to KeySequence since in Qt it cannot accept a Qt.Key. Unfortunately this wrong argument causes a crash.

2 libsystem_c.dylib 0x00007fff68f93808 abort + 120
3 org.python.python 0x00000001066deb81 fatal_error + 753 (pylifecycle.c:2157)
4 org.python.python 0x00000001066de883 Py_FatalError + 19
5 libshiboken2.abi3.5.15.dylib 0x000000010b037aed SetError_Argument + 157
6 QtWidgets.abi3.so 0x000000011d01368e Sbk_QActionFunc_setShortcut(_object*, _object*) + 174

comment:9 by Tom Goddard, 5 years ago

It seems the crashes on bad arguments are something we are doing wrong in ChimeraX. Shiboken2 C++ code tries to call a python seterror_argument method and gets a null return value leading to the fatal error. But it should not be returning null. Possibly a problem with PySIde2 initialization related to my hack to allow importing PyQt5 as PySide. The seterror_argument routine is in

lib/python3.8/site-packages/shiboken2/files.dir/shibokensupport/signature/errorhandler.py

comment:10 by Eric Pettersen, 5 years ago

FYI, QPainter as a context manager is also used in the 2D/3D labeling code.

comment:11 by Tom Goddard, 5 years ago

10) sip.isdeleted(object) in PyQt5 becomes shiboken2.isValid() in PySide2. Used in html_viewer/tool.py and graphics/opengl.py.

Last edited 5 years ago by Tom Goddard (previous) (diff)

comment:12 by Tom Goddard, 5 years ago

11) matplotlib gives an error showing interfaces plot about "sip" missing because it is trying to configure a PyQt5 backend, apparently because the module name PyQt5 is available. Should be fixed by replacing all PyQt5 imports with PySide2.

comment:13 by Tom Goddard, 5 years ago

12) QMenu.exec() in PyQt5 becomes QMenu.exec_() in PySide2. Used in ui/gui.py.

comment:14 by Tom Goddard, 5 years ago

PySide2 does not have QVariant as described here

https://doc.qt.io/qtforpython-5.12/pysideapi2.html#qvariant

and instead says to use native Python types (e.g string). Perhaps Eric can look at adapting ui.widgets.ItemTable to that.

comment:15 by Tom Goddard, 5 years ago

I made a new ChimeraX branch called pyside2. Others feel free to build it, test it and fix problems. I have built it from scratch on Mac but not Windows or Linux yet.

comment:16 by Tom Goddard, 5 years ago

The crashes when PySide2 routines are called with bad arguments are caused by the PySide2 error message generation code failing. Why it fails is extremely obscure and took a couple hours to track down. The error generation does fancy stuff looking for Qt function signatures. In this process it looks at sys.modules, it finds an entry for "core._serialize" and tries to import "core" and there is no core module. The PySide2 code was not expecting bad entries in sys.modules and results in PySide2 calling Py_FatalError. The bad sys.modules entry is really supposed to be "chimerax.core._serialize". The wrong entry is inserted into sys.modules by _serialize.cpp which is the result of running cython on _serialize.pyx. This is a known problem in cython described in this numpy bug report

https://github.com/numpy/numpy/issues/5680

which was closed with the Cython developers basically saying we don't care, it is a rare problem. I don't have a great way to fix this. But I see two bad ways. One is when we import _serialize, fix sys.modules. The other is to replace "core._serialize" with "chimerax.core._serialize" in _serialize.cpp in the core/Makefile right after cython is run to produce _serialize.cpp and before it gets compiled. Both of these fixes could break _serialize. I tried both solutions, with saving and restoring ChimeraX sessions and both worked. If errors in _serialize were raised I'm not sure what would happen. I like the import fix better than C++ edit so I'll go with that. I really don't like this because I may be replacing one extremely bad hard to track bug of PySide2 crashing on any error, with another bad hard to track bug where session save/restore errors misbehave.

This needs more work.

comment:17 by Tom Goddard, 5 years ago

Description: modified (diff)
Summary: PySide2Replace PyQt5 with PySide2

comment:18 by Tom Goddard, 5 years ago

Reading several 2019 and 2020 PyQt5 vs PySide2 comparisons, most reviewers are split saying there is no clear winner. The main advantage cited for PyQt5 is larger user base an more online material due to is longer history. Main advantage cited for PySide2 is licensing.

comment:19 by Greg Couch, 5 years ago

I would replace "core._serialize" with "chimerax.core._serialize" everywhere in _serialize.cpp. I don't see any downsides from giving it the correct package name. And if I had noticed that problem when I wrote _serialize.pyx, I would have done it.

comment:20 by Eric Pettersen, 5 years ago

The data() and headerData() methods of QCxTableModel (subclass of QAbstractTableModel) need to return an invalid QVariant when they don't handle (or want the default behavior for) the particular role being asked about. In PyQt5 an invalid QVariant is QVariant(). In PySide2 it is None. So, now simply returning None in those places.

comment:21 by Eric Pettersen, 5 years ago

Changed the use of QPainter as a context manager when drawing 2D arrows (not so used for 2D/3D labels, despite my earlier comment). While checking the change, ran across:

13) QPainter.drawPolygon() takes a list of QPoints in PySide2, rather than a series of single QPoint arguments as in PyQt5.

Fixed that.

Last edited 5 years ago by Eric Pettersen (previous) (diff)

comment:22 by Greg Couch, 5 years ago

Fixed the name of chimerax.core._serialize in the daily build, so the non-existent core._serialize no longer appears in sys.modules.

comment:23 by Greg Couch, 5 years ago

An aside, unlike PyQt5, PySide2 ships with the QGamePad shared library. Unfortunately there is no Python interface, only QML access. So to use it, we would need to wrap it.

comment:24 by Tom Goddard, 5 years ago

When a QAction is used for a button or menu callback and has the wrong niumber of arguments (PySide2 wants no args, PyQt5 takes 1 event arg) the error message does not give any traceback, file or line number making it hard to find where the problem is. I tested with menu Actions / Atom / show in both PySide2 and PyQt5 using wrong number of args and both had this poor handling of errors. At least PySide2 is no worse than PyQt5 in this situation.

comment:25 by Tom Goddard, 5 years ago

ISOLDE uses Qt Designer (.ui xml file describing widget layout) which generates Python code for PyQt5. There is info about using Qt Designer with PySide2 here

https://doc.qt.io/qtforpython/tutorials/basictutorial/uifiles.html

comment:26 by Tom Goddard, 5 years ago

Here is a list of all the current Toolshed bundles not developed by the UCSF team and that have a GUI that would be effected by moving to PySide2.

ISOLDE Contact: tic20@…
MolecularDynamicsViewer Contact: kdiller99@…
RMF Contact: ben@…
SEQCROW Contact: tony.schaefer@…
Tempy Contact: me@…, new developer joseph.beton.15@…

The SEQCROW Toolshed bundle has 8 GUI panels many with multiple tabs. It has the most user interface of any tool. I contacted Tony Schaefer so he knows about PySIde2. MolecularDynamicsViewer was last updated May 2019, uses an html gui. Tempy was last updated Feb 2018, although there is a new developer starting on it who I gave access to the ChimeraX github repository. RMF is from Ben who we could likely persuade to update is RMF Viewer gui. I've asked Tristan about whether he thinks his Qt Designer based gui will move easily to PySide2.

If we move to PySide2 I think the best solution would be to put out new PySide2 versions of each of these Toolshed bundles. That seems reasonable for 4 out of the 5, all except MolecularDynamicsViewer where it is unclear if the original developer would do it. It is probably possible to still support PyQt5 imports that will in fact use PySide2. I originally used that method to test PySide2 in ChimeraX. But some differences cannot be avoided (e.g. QAction.triggered does not provide an argument). I would prefer not to support PyQt5 compatibility unless absolutely necessary.

Last edited 5 years ago by Tom Goddard (previous) (diff)

comment:27 by Tom Goddard, 5 years ago

Strictly speaking I think moving from PyQt5 to PySide2 would require a major version change to ChimeraX 2. We have only 5 outside tools on Toolshed right now so whether we go to 1.2 or t 2.0 may not matter much. But I think there are at least another half dozen ChimeraX tools out there that have not yet reached the Toolshed and those developers might be a bit dismayed by the transition to PySide2 so we should think how to keep the outside developers happy.

comment:28 by Tom Goddard, 5 years ago

I contacted all 5 outside developers of current gui toolshed bundles to inquire if they would make a new PySide2 release if we make the move.

comment:29 by Tom Goddard, 5 years ago

Cc: Greg Couch Scooter Morris added; chimera-programmers removed

I tried to investigate the development going on for PySide2 and PyQt5 and think PySide2 is better bet for future support than PyQt5.

The PySide2 project had 6 developers make commits in the past month, with 3 making significant changes, looking at their git repository

https://code.qt.io/cgit/pyside/pyside-setup.git/

The project moved from GitHub to code.qt.io 5 months ago. The GitHub site shows 79 contributors to PySide/2 over the last 10 years with 9 contributors changing more than 1000 lines.

I searched for 15 minutes for the PyQt5 code repository and did not find it. I guess it is not public. Riverbank Computing is reported to be a one person company, Phil Thompson. The PyQt5 source code is available as a tar ball and there is PyQt6 work going on and and a tar ball for that. It is hard to tell but if the project is almost all the work of one person I would not be sure of it will be maintained in the future.

The PyQt mailing list has good activity about 50-100 messages per month. I was surprised there is no discussion of PySide2. There was a 2015 discussion of PySide including specifically asking Phil Thompson to comment on making the PyQt repository public but Phil never joined the discussion, and it appears there was never follow-up discussion (no more recent messages with PySide in the subject). It seems it is a taboo topic on the list. I would love to know what Phil Thompson thinks about PySide2.

comment:30 by Tom Goddard, 5 years ago

SEQCROW developer Tony Schaefer and MolecularDynamicsViewer developer Kyle Diller both were fine going to PySide2 and can provide new Toolshed bundles for that.

comment:31 by Tom Goddard, 5 years ago

Tested all Toolbar icons of all tabs, fixed about 5 problems.

comment:32 by Tom Goddard, 5 years ago

I tested all the ChimeraX menus and GUIs and fixed several PySide2 problems and reported about as many tool bugs in both the PySIde2 and PyQt5. I was not able to run Modeller. Not every button of every tool was pressed.

Segment Map and Fit to Segments tools (ie Segger) are still using PyQt5 and don't work. Should be pretty easy to fix.

Next step is to put in PyQt5 backwards compatiblity modules in sys.modules that use PySide2 and see if the 5 toolshed gui tools will work. Skip ISOLDE since no ChimeraX 1.2 version is available. I think that breaks matplotlib which tries to import PyQt5 when using Qt5 backend then PySide2 and decides incorrectly it is using PyQt5. Try interfaces command with PyQt5 modules to test that. If toolshed tools are not working due to PyQt5 / PySIde2 differences then just make the developers update the toolshed tools, or bump ChimeraX version to 2.0.

I compiled PySIde2 ChimeraX on Windows and Ubuntu 18.04 with no problems and they ran simple tests showing molecules and maps. Don't expect any platform specific issues but should test on Windows and Linux a bit more.

Looks like another days works then it should be ready to merge pyside2 branch into the daily build develop branch.

comment:33 by Tom Goddard, 5 years ago

I ported Segger from PyQt5 to PySide2 and tested. This was trivial, just needed a few dozen imports changed and all worked.

I tried all 5 Toolshed GUI bundles from outside developers: ISOLDE, SEQROW, RMF, MolecularDynamicsViewer, TEMPy. ISOLDE and RMF current bundles are compiled against Python 3.7 so cannot be installed with ChimeraX 1.2 which uses Python 3.8. TEMPy is very old and does not work with ChimeraX 1 or 1.1, reported to last work with ChimeraX 0.5. I fails to start trying to import chimerax.core.ui which is now chimera.ui.

By adding PyQt5 to sys.modules set to PySide2 and also adding some submodules (QtWidgets, QtCore) and some PyQt5 specials (pyqtSignal, Qt.QIcon, Qt.QStyle, Qt.QClipboard deprecated locations) and importing matplotlib early so it doesn't get confused by PyQt5 and fail I got MolecularDynamicsViewer to work and tested it, and got SEQCROW to display all its GUIs (about 7 panels with many subtabs) but did not test SEQCROW function since I don't know how. I think we should leave out this PyQt5 backward compatibility since all 5 developers of the Toolshed GUI packages have agreed to migrate to PySIde2. I left the compatibility code in chimerax.ui.gui but commented out.

comment:34 by Tom Goddard, 5 years ago

I've done sufficient testing that it is time to switch the daily builds to use PySide2. I'll give others a chance to object before I do it.

comment:35 by Tom Goddard, 5 years ago

Resolution: fixed
Status: assignedclosed

Done.

The switch to PySide2 was done a weeks ago in daily builds and no bug reports so far. Looking good.

in reply to:  36 ; comment:36 by Tristan Croll, 5 years ago

Was just pointed out to me that PySide has just released a new major version (confusingly named PySide6): https://pypi.org/project/PySide6/. Considering that it's only 4 days old at 6.0.0 it would probably be premature to update - but worth knowing about!
[https://pypi.org/static/images/twitter.90915068.jpg]<https://pypi.org/project/PySide6/>
PySide6<https://pypi.org/project/PySide6/>
Python bindings for the Qt cross-platform application and UI framework
pypi.org

________________________________
From: ChimeraX <ChimeraX-bugs-admin@cgl.ucsf.edu>
Sent: 11 December 2020 04:33
Cc: goddard@cgl.ucsf.edu <goddard@cgl.ucsf.edu>; gregc@cgl.ucsf.edu <gregc@cgl.ucsf.edu>; scooter@cgl.ucsf.edu <scooter@cgl.ucsf.edu>; Tristan Croll <tic20@cam.ac.uk>
Subject: Re: [ChimeraX] #1635: Replace PyQt5 with PySide2

#1635: Replace PyQt5 with PySide2
------------------------------------+---------------------------
          Reporter:  Tristan Croll  |      Owner:  pett, goddard
              Type:  enhancement    |     Status:  closed
          Priority:  moderate       |  Milestone:  1.2
         Component:  UI             |    Version:
        Resolution:  fixed          |   Keywords:
        Blocked By:                 |   Blocking:
Notify when closed:                 |   Platform:  all
           Project:  ChimeraX       |
------------------------------------+---------------------------
Changes (by Tom Goddard):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 Done.

 The switch to PySide2 was done a weeks ago in daily builds and no bug
 reports so far.  Looking good.

--
Ticket URL: <https://plato.cgl.ucsf.edu/trac/ChimeraX/ticket/1635#comment:35>
ChimeraX <http://www.rbvi.ucsf.edu/chimerax/>
ChimeraX Issue Tracker

comment:37 by Eric Pettersen, 5 years ago

Though it works with Qt 5.12+, PySide6 is targeted for Qt6, so I think we can hold off for now. Seems like we would have to bump the ChimeraX major version number when we go to Qt6/PySide6. PySide6 has some nice features, like being able to use properties instead of getters/setters. See https://www.qt.io/blog/qt-for-python-6-released

comment:38 by Tom Goddard, 5 years ago

A few problems with PySide2 have come up. The ChimeraX webcam command does not work and cannot be fixed because PySide2 QCamera.setViewFinder() is broken, bug #4085. This PySide2 bug was reported more than a year ago but ignored. I debugged it and found the exact error in shiboken2 generated C++ code -- and added it to the PySide2 ticket -- still no response a month later. Then I was looking into replacing PyOpenGL with Qt opengl API. It is available in PyQt5 but apparently not in PySide2. I posted a question about OpenGL support to the PySide2 mailing list more than a month ago, dead silence. On top of this lack of response from PySide2 developers, almost all mention of PySide2 is gone on the Qt site and it only talks about PySide6 now.

So I think PyQt5 currently works better and has better support. Down the road I suspect PySide6 is a safer bet as PyQt5/6 relies on one guy who has been working a long time.

Also Qt 6.0 was released in December 2020 and now Qt Company says they only provide patch releases beyond Qt 5.15.2 to commercial customers and they want all us free-loaders to move to Qt 6. Qt 6 currently lacks QtWebEngine, due in Qt 6.2 in September 2021. That is the soonest we would be able to move to Qt 6. The Qt 6 APIs look quite compatible with Qt 5 so possibly a shim module that can handle PyQt5, PySide2 or PyQt6 and PySIde6 may be possible. We probably won't know until we try. But it seems reasonable to go with a shim module now. So that is what we are doing, described in ticket #4120.

Since we are going to Qt shim module to hide the use of PySide2 or PyQt5 we may as well also switch back to the better working and better supported PyQt5.

Note: See TracTickets for help on using tickets.