mottosso / Qt.py

Minimal Python 2 & 3 shim around all Qt bindings - PySide, PySide2, PyQt4 and PyQt5.
MIT License
914 stars 254 forks source link

Added PySide6 Support (squash) #394

Closed zoshua closed 5 months ago

zoshua commented 7 months ago

New branch for the "squashing"

mottosso commented 7 months ago

Excellent, nice and clean. But, somehow, tests are still not running. 😭 @martin-chatterjee can you spot what we could do to make tests run on PRs?

I expect tests will pass, as they appear to have done locally. So it's likely we can merge this, without officially announcing support for PySide6, and work our way to introducing tests for it in a separate PR. Although ideally, we'd do that here as well.

One of the reasons for using such a complex Dockerfile as we have today is because the world was young, and PyQt and PySide was just starting to live side by side, so it wasn't clear whether there were differences between official PySide/PyQt on PyPI versus PySide embedded in Maya. At this point however, I expect it would be very safe to test against what exists on PyPI. Especially considering this project no longer runs exclusively on Maya.

I remember seeing someone post their fork with an alternative test suite, but I'm struggling to find it :S It would be sensible to keep a separate GitHub action script or runner per Python version and PySide/PyQt combination.

Would you be up for this @zoshua or @martin-chatterjee?

martin-chatterjee commented 7 months ago

"Excellent, nice and clean. But, somehow, tests are still not running. 😭 @martin-chatterjee can you spot what we could do to make tests run on PRs?"

@mottosso, I just found the time to have a quick look, and I can not reproduce the CI issues on my side – which is good news, I guess? 🙂

I've created a PR for further debugging – so maybe you could try to merge my PR into some temp branch and push on your end? This should show us if the CI still fails on mottosso/Qt.py.


For reference, here's what I did:

1. Add @zoshua's Qt.py fork as a remote, and fetch:

git remote add zoshua git@github.com:zoshua/Qt.py.git
git remote set-url zoshua --push no-push
git fetch zoshua

2. Create new branch review-zoshua-pyside6 off of master, and push:

git checkout master
git checkout -b review-zoshua-pyside6
git push origin review-zoshua-pyside6

→ This establishes my base line, proofing that CI is (still) running as expected. ✅

3. Bring in @zoshua's work out of branch squash, and un-stage it:

git merge --squash zoshua/squash
git restore --staged .

4. Commit changes bit by bit, and push on every commit:

CI completes without errors on every commit. ✅

mottosso commented 7 months ago

so maybe you could try to merge my PR into some temp branch and push on your end?

But actions should run without merging. A big part of the actions is to make sure merging is safe to begin with, but if we have to merge before running the tests..? Am I missing something? 🤔 Your PR also is not triggering any tests. I expect there must be a configuration option to make GitHub Actions run on PRs too?

martin-chatterjee commented 7 months ago

@mottosso: Ah I see, I might have misunderstood the issue in the first place.

I was under the impression that the CI test jobs do not succeed on your end (→ just like this last failed GH action).

But what you would like to have is automatic CI test runs being executed on mottosso/Qt.py for every new PR, right?

So I might have barked up the wrong tree here for while... 🙂🤦‍♂️

martin-chatterjee commented 7 months ago

Off the top of my head, I'm not sure if running CI on the targeted repo of a PR is possible on Github – I've never thought about that before...

However, before any contributor is able to open a PR, they need to push to their fork – and this will already trigger and execute the CI test jobs over there.

So in a way, every PR already has executed CI test jobs associated with it – just not on mottosso/Qt.py, but on the source fork.

@mottosso, is this what you're after?

mottosso commented 7 months ago

But what you would like to have is automatic CI test runs being executed on mottosso/Qt.py for every new PR, right?

That's right, yeah. Sorry for not being clear about this! I appreciate you spent time putting together this alternative PR and painstakingly ran through each commit.

mottosso commented 7 months ago

@mottosso, is this what you're after?

No. In every other project I maintain on GitHub, CI is always run on PRs. There should be no need to visit a fork in order to see the results of tests.

For example:

martin-chatterjee commented 7 months ago

Ah nice, thanks for the example. So this actually is possible within Github. I'll take a look. 👍

martin-chatterjee commented 7 months ago

Well, that should be easy... 🙂👍

According to the example repo you provided:

name: cmdx-workflow

on:
  push:
  pull_request:
    branches: [ master ]

That seems to be all that's needed. 🙂

I'll take a look and test this.

martin-chatterjee commented 7 months ago

@mottosso:

PR #399

mottosso commented 7 months ago

Excellent, thanks @martin-chatterjee. At this point, I expect any updates to this PR to also run all tests. @zoshua would you mind merging with latest master, so we can see this thing in action?

mottosso commented 7 months ago

Great work. Now all we need is a GitHub workflow to test the PySide6-specifics of it. I expect we can start by adding a pyside6.yml here with the steps (1) install Python, (2) install PySide6 and (3) run the tests. Would you like to give it a spin @zoshua?

zoshua commented 7 months ago

Looks like the nose2 update for ./entrypoint.sh has created issues with the job running run-tests.yml

Thoughts on how to proceed?

martin-chatterjee commented 7 months ago

@zoshua, we can't really modify entrypoint.sh, as it's also being used in the Docker container, in run-tests.yml.

I'll let @mottosso chime in with his thoughts and preferences, but I can see three solutions:

  1. Stick to using nose. (Despite it being oldschool... 🙂)
  2. Continue to use nose2 in pyside6.yml, and create a nose symlink that points to nose2, before executing entrypoint.sh.
  3. Continue to use nose2 in pyside6.yml, and execute an alternative run-tests.sh, instead of entrypoint.sh.

Personally, I'd probably do option 2.

And either way, we should probably roll back all modifications to entrypoint.sh.

@mottosso, thoughts?

mottosso commented 7 months ago

It's safe to make a new entrypoint.sh, e.g. entrypointPySide6.sh. It's also fine to move the contents of that file into the yml, as it was only really meant for use with Docker since Docker struggles with lengthy commands like this. GitHub actions does not.

zoshua commented 7 months ago

@mottosso @martin-chatterjee I am new to github actions and don't really know the preferred way to define the env for running steps.

It appears that the step Testing examples.. is not able to import Qt

I am happy to continue to tinker but wanted to check if you might have any insight in this area.

mottosso commented 7 months ago

The "Testing Implementation" is also unable to test PySide6:

https://github.com/mottosso/Qt.py/actions/runs/8113970481/job/22178534199?pr=394#step:4:12

Have a look inside, and see what makes them tick.

martin-chatterjee commented 7 months ago

@zoshua, I think you might need to explicitly pip install the checked out code.

I think you should just be able to run:

python3.10 -m pip install .

(This is assuming, that the "Checkout code" step checked out the repo into the current working directory.)

zoshua commented 7 months ago

@mottosso Added with binding PySide6 to run_tests.py It appears to run without errors in pyside6.yml It appears to produce errors for the other Docker tests

@martin-chatterjee Added step pip install . after code is checked out in pyside6.yml It appears to run without errors, but we are still erroring out at the step Testing examples for the same reason as before

mottosso commented 7 months ago

Those EGL errors is because Qt requires a display, and these headless runners have none. There's a way to "fake" a display, which is what's happening here.

I know iterating by pushing is tedious and takes a long time. I know it all too well. 😅 But there's an alternative you can try, either if you have Docker on your own machine or you can use a cloud based (free) Docker here.

It'll put you in a virtual machine, much like the one on GitHub, where you can load the same OS and do all of your testing. It would be easier locally, as you can mount your local folder into the virtual machine, but at least on the cloud version you can install and run things interactively.

zoshua commented 7 months ago

Happy to iterate to make this thing sing!

Will try out the Docker reccos, hope the test error spam'ing is not too annoying.

mottosso commented 7 months ago

Not annoying at all, go right ahead. If anyone is getting too many messages, there's an Unsubscribe button at the top.

martin-chatterjee commented 7 months ago

@zoshua, absolutely not annoying at all – please go ahead. 🙂👍

May I recommend to maybe even start one step "earlier" than within Docker, and first try to get the test suite to run on your local machine, in a virtual environment.

You know, something along those lines:

# → In bash...

# Create and enter virtualenv.
python3.10 -m venv ./venv-3.10-qt.py
source ./venv-3.10-qt.py/bin/activate
pip install --upgrade pip setuptools

# Install dependencies.
pip install .........

# Install Qt.py.
pip install .

# Run test suite.
export QT_PREFERRED_BINDING="PySide6"
nosepipe --verbose --with-process-isolation --exe
# 👆 Replace with the equivalent call for `nose2`, if you're using that.

I haven't had the chance to do so myself – but I've got a hunch that this might already surface some of root issue(s).

zoshua commented 7 months ago

Got to tinkering with Docker in the cloud as well as running the tests locally on macOs.

Couple of key findings:

  1. tests.py is assuming we are using nose, I have a workaround that uses unittest and defines our own assert_raises() function at the moment, seems to do the trick
  2. tests.test_membership is reporting back a pretty large list of members that did not make it to PySide6
  3. found my way to the rabbit hole that is build_membership.sh, seems like this might need to be rerun w/ PySide6 now in the mix
  4. couple of other oddities in tests.py that might need some tweaks as well

Would love to know your takes on best path forward given these notes.

mottosso commented 6 months ago

Would love to know your takes on best path forward given these notes.

Sounds like you're on the right track to me, please do proceed!

mottosso commented 6 months ago

With Maya 2025 being released, and having PySide6 on-board, this merge request is now very relevant. What's the latest on your end @zoshua? Anything holding you up?

zoshua commented 6 months ago

Hi @mottosso! Just saw the Maya 2025 thing a few days back as well.

Apologies for the radio silence, job stuff picked up quite seriously over the past few weeks on my end.

I believe I pressed pause here at a stage where I had just gone through the process of generating the _common_members locally.

After generating the _common_members and updating Qt.py I remember wondering if we had or needed the ability to procedurally generate the _misplaced_members as well.

From what I can tell PySide6 seems to have quite a few fundamental changes that will reduce the _common_members significantly.

(Side Note: I had not encountered any of this in my original commit as I had just updated Qt.py with PySide6 bits until it stopped erroring and my code would run.)

Still pushing on bringing it all together for a real release but will definitely be looking to all the pros here for insight on making the update be as fast and clean as possible.

zoshua commented 6 months ago

For this Assert "AttributeError: module 'PySide2.QtCore' has no attribute 'QStringListModel'"

I am able to run locally non issue and the documentation seems to align https://doc.qt.io/qtforpython-5/PySide2/QtCore/QStringListModel.html

Thoughts?

mottosso commented 6 months ago

https://github.com/mottosso/Qt.py/pull/394/checks#step:7:94

Only one missing member! That's great.

This seems to me like it's been renamed/slightly edited. We could either (1) wrap the Qt 6 class in our own to emulate the one from Qt 4/5. Or we could deprecate it entirely. Wrapping it seems straightforward, but I haven't looked into whether it's used in the same way. And it is useful, I expect we will need it.

Thoughts?

mottosso commented 6 months ago

"AttributeError: module 'PySide2.QtCore' has no attribute 'QStringListModel'"

Did you overcome this? Because it does exist in Qt 4/5/6.

zoshua commented 6 months ago

"AttributeError: module 'PySide2.QtCore' has no attribute 'QStringListModel'"

Did you overcome this? Because it does exist in Qt 4/5/6.

I just reverted to what previously existed/worked here in the github Run Tests.

Do you think its related to running our tests in the highly customized docker container?

zoshua commented 6 months ago

Just checked and it appears that the function signatures match..

https://doc.qt.io/qtforpython-5/PySide2/QtGui/QRegExpValidator.html#PySide2.QtGui.PySide2.QtGui.QRegExpValidator

https://doc.qt.io/qtforpython-6/PySide6/QtGui/QRegularExpressionValidator.html#PySide6.QtGui.PySide6.QtGui.QRegularExpressionValidator

So we should be good to map PySide6 to QRegExpValidator with _misplaced_members

zoshua commented 6 months ago

@mottosso This one appears to be PyQt4 Centric File "/home/runner/work/Qt.py/Qt.py/examples/loadUi/baseinstance2.py", line 7, in

You think I am good to remove from Pyside6.yml?

mottosso commented 5 months ago

Yes, definitely. Based on explicitly making this only run for PyQt4, https://github.com/mottosso/Qt.py/blob/master/examples/loadUi/baseinstance2.py that seems safe.

mottosso commented 5 months ago

Had a look at this, and we've got a bit more work before we can call this drop-in ready for Qt 4 and Qt 5.

  1. QFont.setWeight doesn't take a number anymore, but an enum
  2. QtCore.Qt.MidButton no longer exists for mousePressEvent

Here's one way to solve (1).

diff --git a/Qt.py b/Qt.py
index 08b5c03..00d3571 100644
--- a/Qt.py
+++ b/Qt.py
@@ -1195,6 +1195,9 @@ _compatibility_members = {
             "getOpenFileNames": "QtWidgets.QFileDialog.getOpenFileNames",
             "getSaveFileName": "QtWidgets.QFileDialog.getSaveFileName",
         },
+        "QFont": {
+            "setWeight": "QtGui.QFont.setWeight",
+        },
     },
     "PySide2": {
         "QWidget": {
@@ -1514,8 +1517,38 @@ def _pyside6():
         Qt.QtCompat.setSectionResizeMode = \
             Qt._QtWidgets.QHeaderView.setSectionResizeMode

+    def setWeight(func):
+        def wrapper(self, weight):
+            weight = {
+                100: Qt._QtGui.QFont.Thin,
+                200: Qt._QtGui.QFont.ExtraLight,
+                300: Qt._QtGui.QFont.Light,
+                400: Qt._QtGui.QFont.Normal,
+                500: Qt._QtGui.QFont.Medium,
+                600: Qt._QtGui.QFont.DemiBold,
+                700: Qt._QtGui.QFont.Bold,
+                800: Qt._QtGui.QFont.ExtraBold,
+                900: Qt._QtGui.QFont.Black,
+            }.get(weight, Qt._QtGui.QFont.Normal)
+
+            return func(self, weight)
+
+        wrapper.__doc__ = func.__doc__
+        wrapper.__name__ = func.__name__
+
+        return wrapper
+
+    decorators = {
+        "QFont": {
+            "setWeight": setWeight,
+        }
+    }
+
     _reassign_misplaced_members("PySide6")
-    _build_compatibility_members("PySide6")
+    _build_compatibility_members("PySide6", decorators)

 def _pyside2():

Namely, we provide a new member via QtCompat which requires updates to legacy software.

font = self._window.font()
font.setFamily("Open Sans")
font.setPointSize(8)
Qt.QtCompat.QFont.setWeight(font, 400)

Not ideal, the ideal would be for existing code to require 0 changes. But, we cannot do:

Qt.QtGui.QFont.setWeight = some_wrapper_function

As this would modify the original binding, which is one of the things that sets Qt.py apart from other wrappers. We cannot risk the stability of code that does not use Qt.py.

What options do we have? If we go with the above, then we must provide this member for all bindings, and we should also provide the new enums, e.g. QtCompat.QtGui.QFont.Bold, such that new code can run on old bindings.

Here's one project you can try with.

python -m pip install pyblish-lite

It bundles Qt.py inside of vendor/Qt.py. Replace that with this version and you'll run into the above issues. Can we test with a few project projects to find what else there is? We'll need an upgrade guide in the README with what users are expected to do in order to make their projects compatible with Qt 6. If indeed we cannot find a way to make it transparent.

zoshua commented 5 months ago

Thank you for the feedback and insight.

Will be picking this up soon 👍

zoshua commented 5 months ago

Investigations...

  1. I made the "Here's one way to solve (1)." tweaks locally to Qt.py and ran our current PySide6 tests without issue.
  2. I created a new space/venv to test pyblish-lite w/ PySide6 and the latest tweaks made from step 1, updating the local vendor/Qt.py.
  3. I encounter and fix some PySide6 _misplaced_members issues
  4. I confirm the QtCore.Qt.MidButton button issue
  5. I hacked a bit with _misplaced_members to see if we could reroute "QtCore.Qt.MiddleButton": "QtCore.Qt.MidButton" with no luck
  6. I hacked a bit further directly tweaking _pyside6() by adding Qt._QtCore.Qt.MidButton = module.QtCore.Qt.MiddleButton, which seems to have made the error go away, however I am sure this is not the appropriate solution. Also, I have not used pyblish before so I am not familiar w/ how it is supposed to function.

Thoughts on next steps?

mottosso commented 5 months ago

I hacked a bit further directly tweaking _pyside6() by adding Qt._QtCore.Qt.MidButton = module.QtCore.Qt.MiddleButton, which seems to have made the error go away

This would be the easiest route, but we can't risk affecting applications that use PyQt/PySide without Qt.py, and this approach would affect those.

Thoughts on next steps?

Let's stick with the first approach for now and put it in a "Qt 6 Transition Guide". We need the find a similar solution to MidButton and put that in the guide too. Our goal then should be to make this guide as small as possible.

Qt 6 Transition Guide

zoshua commented 5 months ago

Made the suggested updates and added a possible solution for the Mid/Middle Button change.

Confirmed error removal in pyblish-lite by making the following changes in view.py from .vendor.Qt import QtCore, QtWidgets, QtCompat

view.py - Item.mousePressEvent() if event.button() == QtCompat.Qt.MidButton:

mottosso commented 5 months ago

Great, thanks for this.

Considering that this PR does not affect existing use of Qt.py, I'll merge this now despite suspecting that there will be a number of bullet points needed for an upgrade guide. I'll add a section to the README now with what we now, then we can continue adding and updating as we continue our discovery. A new release will follow shortly.