Feat/qtpy #61
Feat/qtpy #61
Conversation
Codecov Report
@@ Coverage Diff @@
## master #61 +/- ##
==========================================
- Coverage 97.95% 97.95% -0.01%
==========================================
Files 6 6
Lines 539 538 -1
==========================================
- Hits 528 527 -1
Misses 11 11 |
|
Yeah, @GuillaumeFavelier. This can easily be fixed by ensuring that a Qt Python binding is installed on the test machine, but I'm not sure exactly how you want to do that. Switching to qtpy means that it is now up to the user to have a qtpy-compatible Qt Python binding installed. In my opinion, it no longer makes sense to add a specific Qt Python binding in the install requires, since that is now up to the user. I do have opinions on how these types of issues could be solved, but, since they lead to significant changes in how people interact with pyvistaqt, I want sure if you wanted me to make those changes or not. |
…sary QtGui import
|
Historically, In my opinon, a "transition time" (one major version?) where Does it make sense? Also, what are your thoughts about this @pyvista/developers ? |
This makes sense to me. That way, when users |
|
@GuillaumeFavelier , you'll see that I made quite a few changes since your last review. Most notably, updated the docs, and formatting issues, fixed the call to pydoctest in the makefile. The only issue left is that the docs are still not building. I'm not sure why, but maybe you understand the error message... |
Those changes are welcome, thank you @nicobako 🙏
I'll see what I can do |
|
I'm sure I saw this error before somewhere but I can't recall exactly... Is it related to the headless display? Any idea @pyvista/developers |
|
There are a slew of Linux deps that you need to use the builtin xcb (PyQt5 used to bake this into their lib IIRC then they changed), for example see what we install on CircleCI for MNE https://github.com/mne-tools/mne-python/blob/master/.circleci/config.yml#L58-L59 IIRC that big long line is what's needed for this not to complain. These were identified by doing |
|
Many thanks for the swift answer! I will try with that then 👍 |
|
Alright @nicobako, I pushed a few commits and the doc is building now 👍 Useful material for future reference: https://doc.qt.io/qt-5/linux-requirements.html |
|
Thanks @GuillaumeFavelier . I think it's all good on my end. As long as you guys are happy with the changes I made to the documentation (.rst files), then I think this is ready! |
| Historically, ``pyvistaqt`` has used ``pyqt5``, which is subject | ||
| to the GPL license. See details at | ||
| `Riverbank License FAQ <https://www.riverbankcomputing.com/commercial/license-faq>`_. | ||
|
|
||
| ``pyvistaqt`` is transitioning to using ``qtpy`` | ||
|
|
||
| > QtPy is a small abstraction layer that lets you write applications using a single API call to either PyQt or PySide. |
There was a problem hiding this comment.
Why the leading >? I would make this more compact like:
| Historically, ``pyvistaqt`` has used ``pyqt5``, which is subject | |
| to the GPL license. See details at | |
| `Riverbank License FAQ <https://www.riverbankcomputing.com/commercial/license-faq>`_. | |
| ``pyvistaqt`` is transitioning to using ``qtpy`` | |
| > QtPy is a small abstraction layer that lets you write applications using a single API call to either PyQt or PySide. | |
| Historically, ``pyvistaqt`` has used ``pyqt5``, which is subject | |
| to the GPL license. See details at | |
| `Riverbank License FAQ <https://www.riverbankcomputing.com/commercial/license-faq>`_. ``pyvistaqt`` now uses QtPy if present, and will require it after version 0.3. QtPy is a small abstraction layer that lets you write applications using a single API call to either PyQt or PySide (which has a LGPL license). |
The wording about being required or not should be adjusted based on whether or not there is actually going to be a deprecation period.
There was a problem hiding this comment.
The leading > is because it's a quote from the qtpy docs... But I'm open to changing it to something that looks nicer.
| from PyQt5.QtCore import QObject, pyqtSignal, pyqtSlot | ||
| """This module contains a basic Qt-compatible counter class.""" | ||
|
|
||
| from qtpy.QtCore import QObject, Signal, Slot |
There was a problem hiding this comment.
This will immediately break any pyvistaqt code without deprecation. We colud make a small compatibility layer just by making a little ._qtpy file that contains:
try:
from qtpy import QtCore
except ImportError:
warn('qtpy not found; falling back to PyQt5. qtpy will be required in pyvistaqt 0.4')
from PyQt5.QtCore import ...
else:
from qtpy import ...
And then in this file do:
| from qtpy.QtCore import QObject, Signal, Slot | |
| from ._qtpy.QtCore import QObject, Signal, Slot |
And once we cut version 0.3, we can revert the from ._qtpy import ... lines to just be from qtpy import ...
But maybe this is overkill since pyvistaqt is still relatively new. Up to the main pyvista devs I guess...
There was a problem hiding this comment.
If the core developers think there needs to be a deprecation period, then this would be a good solution. I'm open to this.
There was a problem hiding this comment.
I think just going ahead without the depreciation works. Users can always downgrade to an older version, and adding in a depreciation is a bit of a pain. Plus, installing qtpy should be in the setup.py, so it will be included upon install/upgrade.
We can always give a more descriptive error on import if we wish, but it's a bit of extra work making it backwards compatible and adding a depreciation warning as well.
|
Okay, good enough for me then! |
|
Thank you @GuillaumeFavelier and @larsoner for your help! I'm really glad to see this merged into pyvistaqt! |
This is a continuation of @GuillaumeFavelier 's work from #12 . I have updated his work with the latest master branch from pyvista, which had some merge conflicts and required replacing
pyqt5withqtpyin a few additional places.I also updated the test files to use
qtpyinstead ofpyqt5. I'm not exactly sure how your CI works, but I imagine that you will have to set theQT_APIenvironment variable to inform qtpy which Qt Binding you want to use. I'm not sure if you want your CI to test multiple QT bindings, or just one... I can definitely work on this if you let me know which Qt Bindings you want to test.Also, I replaced the
pyqt5requirement in theinstall_requireswithQtPy. This makes the user responsible for installing the Qt bindings of their choice, and makes pyvistaqt agnostic of the specific Qt bindings.Another thing that we must still do is update the documentation by replacing any usage of
pyqt5withqtpy. I wasn't exactly sure how you wanted to treat that, so I haven't done it yet.Users will have to use
qtpyinstead, which has the additional step of setting theQT_APIenvironment variable. From what I understand, this must be set before qtpy is imported. I can easily update the docs if you let me know what kind of guidance you want to give to users.So, in summary, things that still need to be done are:
QT_APIenvironment variableqtpyinstead ofpyqt5usage.rstindex.rst?index.rst