Skip to content
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

Add support for modules that are now available in Qt 6.5 #427

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DaelonSuzuka
Copy link
Contributor

@DaelonSuzuka DaelonSuzuka commented Apr 12, 2023

PySide6 v6.5.0 released last week, allegedly adding the QtTextToSpeech, QtSerialBus, and QtLocation modules.

This PR adds the completely missing QtSerialBus, and adds version-dependent imports to QtTextToSpeech and QtLocation.

This is currently a draft because I haven't gotten all the tests written yet, and I wanted to ask if there's any advice for running the full matrix of tests locally. It's way too much work to shuffle different Qt bindings and versions in and out of my local venv. I've used tox in another project, and even though it worked, it wasn't especially enjoyable.

Edit: Additionally, if there's a reasonable way to run this matrix, then I might be motivated enough to revisit my previous automatic-qt-library-auditor from PR #344, and set that up as a runnable tool. This would be useful in keeping up with further changes in the Qt ecosystem.

@StSav012
Copy link
Contributor

The tests are painful for me, too. Moreover, despite the maintainers claim that they support PyQt5 >= 5.9.0, PySide2 >= 5.12.0, and Qt6 >= 6.2.0, they never actually test the code with the oldest versions. @DaelonSuzuka, would you mind opening a corresponding issue? The PR is not the best place to discuss troubles.

@DaelonSuzuka
Copy link
Contributor Author

would you mind opening a corresponding issue?

Why?

The PR is not the best place to discuss troubles.

Says who?

@StSav012
Copy link
Contributor

I've always thought that the PRs are for proposing changes and discussing them. And the Issues are for general discussions and troubleshooting. Forgive me if I'm wrong. I'd prefer not to flood the PR with such useless talk as I'm having now. Although I'm interested in the test as well, I'll follow the discussion of the maintainers and yours silently.

@CAM-Gerlach
Copy link
Member

This is currently a draft because I haven't gotten all the tests written yet, and I wanted to ask if there's any advice for running the full matrix of tests locally. It's way too much work to shuffle different Qt bindings and versions in and out of my local venv. I've used tox in another project, and even though it worked, it wasn't especially enjoyable.

Unfortunately, I don't have a great answer for you; I at least have generally relied on the CIs to run the full matrix, coupled with a few conda envs with the major Qt/binding versions installed and using the QT_API env var to switch between them when I need to test something locally on a specific version.

The simplest solution to avoid uninstalling and reinstalling stuff would be just having a directory of venvs each with the different Qt/binding versions you want to test. You would need only two venvs total to respectively test the lower and upper minor version bounds for each binding x Qt major version, since you can switch the binding + Qt major version with with the QT_API env variable. You could add additional ones to test the middle bounds of each, if desired.

Beyond that, you could use the https://github.com/spyder-ide/qtpy/blob/master/.github/workflows/test.sh script (probably with a couple tweaks and setting a few env vars) to setup each env and run the tests just as the CIs do.

Converting this to use tox or nox (being the standard tools designed for this purpose) is possible and I looked into it previously when overhauling the CIs last time, but it would require completely re-doing and either throwing out or duplicating a fairly big chunk of configuration and scripting, and then re-integrating that with the existing CI build matrix. Overall, that's a pretty hefty chunk of work. Additionally, we would need to use tox-conda in order to allow use of conda-based Python environments and allow using either pip or conda for package install, which doesn't yet support Tox 4 and on which maintenance and testing seems rather spotty. It would still be worth considering, for sure, and I'd be willing to review it, but it would require someone with the time and know-how to actually do the work.

Finally, if the existing CIs aren't adequate to test automatic-qt-library-auditor, I could potentially help you set up something specific to that, if that would help.

The tests are painful for me, too. Moreover, despite the maintainers claim that they support PyQt5 >= 5.9.0, PySide2 >= 5.12.0, and Qt6 >= 6.2.0, they never actually test the code with the oldest versions.

I'm not sure what you mean there, sorry. Our CI test matrix includes at least one job each for PyQt5/Qt 5.9, PySide2/Qt 5.12, PySide6/Qt 6.2 and PySide6/Qt 6.2, plus a number of jobs for the upper bound of each (5.15/6.4), and many jobs for the other LTS version/middle bound too (5.12 and 6.3).

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general and higher-level comments to start out with.

@@ -8,18 +8,27 @@
"""Provides QtLocation classes and functions."""

from . import (
parse,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works "by accident"/as an internal implementation detail, as parse is in fact imported from packaging, not QtPy; thefore, it should be imported from there instead.

Also, AFAIK our existing tests use the cruder if PYQT_VERSION.startswith("6.5"), but since packaging is required anyway, we may as well use it to be cleaner and more precise, as you do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works "by accident"/as an internal implementation detail, as parse is in fact imported from packaging, not QtPy; thefore, it should be imported from there instead.

I actually did originally have from packaging import parse but I for some reason I can't remember, I removed it and did this instead.

Also, AFAIK our existing tests use the cruder if PYQT_VERSION.startswith("6.5"), but since packaging is required anyway, we may as well use it to be cleaner and more precise, as you do.

I did see that when I was poking around. I can fix all the startswiths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did see that when I was poking around. I can fix all the startswiths.

Yeah, thanks—that's probably worth doing in a separate PR from this one, just to be clear.

if parse(PYQT_VERSION) >= parse('6.5'):
from PyQt6.QtLocation import *
else:
raise QtBindingMissingModuleError(name='QtLocation')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this module was added in Qt 6.5 instead of just in both Python bindings at that same version, then I'm not sure this is the right error, as it is intended for reporting when a particular binding doesn't support a missing module, rather than it not being implemented in Qt itself—not sure how it got overlooked initially, as the current call signature will actually raise an error since it expects binding to be passed. Instead, it would seem to be better to use QtModuleNotInQtVersionError here, with a tweak to either output the full major + minor version automatically (probably best), or pass that version manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this module was added in Qt 6.5 instead of just in both Python bindings at that same version

This is one of the reasons I need to set up a local environment matrix. I believe that PyQt6 had some of these modules the entire time, and it's only PySide6 that just added them to the distribution, but I won't know for sure until I'm able to step through the versions.

)


@pytest.mark.skipif(not PYSIDE6 or parse(PYSIDE_VERSION) >= parse('6.5'), reason='.')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume you're going to fill the reason in later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!


@pytest.mark.skipif(not PYSIDE6 or parse(PYSIDE_VERSION) >= parse('6.5'), reason='.')
def test_qtserialbus_pyside6_below_6_5():
with pytest.raises(QtBindingMissingModuleError) as excinfo:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with pytest.raises(QtBindingMissingModuleError) as excinfo:
with pytest.raises(QtBindingMissingModuleError):

No need for the as if you don't actually use excinfo, no? Or maybe you're planning to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering inspecting the actual error in the test. Do you have an opinion on whether that's worth doing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, yeah, at least the custom attributes of the error object (as opposed to the message, which is considered a user-facing implementation detail subject to change and a test of which would be too fragile).

Comment on lines +3 to +4
import pytest
from qtpy import (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split third party imports from local imports (same with the others)

qtpy/QtTextToSpeech.py Outdated Show resolved Hide resolved
Comment on lines +16 to +17
PYQT_VERSION,
PYSIDE_VERSION,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of importing both of these separately, and particularly if this is related to the Qt version rather than the binding version, you could just import QT_VERSION and check that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried that first, and my editor insisted that QT_VERSION didn't exist.

This is also one of the modules that I need the environment matrix for. I think that PyQt6 had this module earlier than PySide6 did.

Comment on lines +1 to +6
# -----------------------------------------------------------------------------
# Copyright © 2009- The Spyder Development Team
#
# Licensed under the terms of the MIT License
# (see LICENSE.txt for details)
# -----------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least be consistent about whether we're adding this header to the tests—either to all newly added ones (ideal), or else none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct copyright header? Various files have slightly different headers and some of them also list specific individuals, so I've never been sure what header I should be adding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to comment on that, but I didn't want to be too nitpicky, heh.

The up to date version of the canonical header (for new files, at least) is in the License Guidelines on the our governance repo, i.e.:

Suggested change
# -----------------------------------------------------------------------------
# Copyright © 2009- The Spyder Development Team
#
# Licensed under the terms of the MIT License
# (see LICENSE.txt for details)
# -----------------------------------------------------------------------------
# -----------------------------------------------------------------------------
# Copyright (c) 2023- QtPy contributors
#
# Released under the terms of the MIT License
# (see LICENSE.txt in the project root directory for details)
# -----------------------------------------------------------------------------

The reason a number of files list different individuals is because a lot of the code was combined from multiple predecessor projects (see the Readme section that touches on a bit of this history), plus various other historical contributions. New files should just use the above, but it is important to retain the existing copyright notices in files that have them for both legal and ethical purposes.

Co-authored-by: C.A.M. Gerlach <[email protected]>
@DaelonSuzuka
Copy link
Contributor Author

Unfortunately, I don't have a great answer for you;

Actually, your comment helps a lot. Thanks for the input(as always!).

I at least have generally relied on the CIs to run the full matrix, coupled with a few conda envs with the major Qt/binding versions installed and using the QT_API env var to switch between them when I need to test something locally on a specific version.

I've tried to use QT_API before and couldn't get it to work; I suspect this is a skill issue.

The simplest solution to avoid uninstalling and reinstalling stuff would be just having a directory of venvs each with the different Qt/binding versions you want to test. You would need only two venvs total to respectively test the lower and upper minor version bounds for each binding x Qt major version, since you can switch the binding + Qt major version with with the QT_API env variable. You could add additional ones to test the middle bounds of each, if desired.

At least as an experiment, I'm considering building a big 'ol matrix with all the major versions of each binding, and then run an audit of the entire history of Qt. I think I will start with tox and see how far that takes me.

Converting this to use tox or nox

I definitely wouldn't advocate rebuilding the existing CI system, and I don't believe the audit tool needs to be a part of the standard CI workflow.

@StSav012
Copy link
Contributor

Our CI test matrix includes at least one job each for PyQt5/Qt 5.9, PySide2/Qt 5.12, PySide6/Qt 6.2 and PySide6/Qt 6.2, plus a number of jobs for the upper bound of each (5.15/6.4), and many jobs for the other LTS version/middle bound too (5.12 and 6.3).

I'm sorry, I haven't noticed that PyQt5 5.9 is tested only on Windows. I've looked through Ubuntu tests and assumed that the versions used are identical for all the platforms.

I must have had a bad day yesterday, and I apologize for the noise I made here.

@CAM-Gerlach
Copy link
Member

I've tried to use QT_API before and couldn't get it to work; I suspect this is a skill issue.

I actually used it successfully just earlier today; you can check if it is by running the mypy-args command via the QtPy CLI (qtpy mypy-args) which will tell you what binding would be selected importing qtpy normally. There are some reasons why it might be overridden, if the application you're testing sets the env var first before importing QtPy, if the selected binding cannot be imported, or if one of the modules is already imported prior to qtpy being imported. You can set FORCE_QT_API to override this last bit.

Just to note, the binding selection logic is currently kinda jankily structured and could really use a refactoring, which is something I'm planning to do if and when the next major version rolls around (since it would result in some non-trivial behavior changes in certain edge cases).

At least as an experiment, I'm considering building a big 'ol matrix with all the major versions of each binding, and then run an audit of the entire history of Qt. I think I will start with tox and see how far that takes me.

That would be super cool, at least IMO! Would love to see that. At least, you could run the supported versions from 5.9 - 5.15 and 6.2-6.5.

I'm sorry, I haven't noticed that PyQt5 5.9 is tested only on Windows. I've looked through Ubuntu tests and assumed that the versions used are identical for all the platforms.

Yeah, the matrix is pretty darn complex, even if it is mostly single-sourced now in one declarative config file.

I must have had a bad day yesterday, and I apologize for the noise I made here.

No worries on my side and hope you're having a better day today; I did get a little worried it might put either you or @DaelonSuzuka off from contributing as you've both been very helpful in recent QtPy releases but hopefully that has smoothed out. Cheers!

@dalthviz dalthviz added this to the v2.5.0 milestone Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants