Opened 7 years ago

Last modified 5 years ago

#1635 closed enhancement

Replace PyQt5 with PySide2 — at Version 17

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 (17)

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
Note: See TracTickets for help on using tickets.