Add a deprecation warning for unsupported Qt versions and bindings (at least Qt4: PyQt4 and PySide)

[dalthviz]

Maybe could be worthy to release 1.11.3 with a depreciation warning:

• Deprecation warning for Qt4 (removed PyQt4 and PySide support for v2.0.0)
• Deprecation warning for older versions of Qt5: Depending on what minimum Qt5 and bindings versions we will support for v2.0.0. Maybe 5.9 (if we can make it work), but most probably 5.12

[CAM-Gerlach]

Seems like a good idea, and would presumably be a simple if statement in __init__.py checking API and/or initial_api and raising a Warning with an informative message if so.

However, this does raise some important UX questions for us to consider (maybe I'm overthinking this 😆 ):

• Should we issue a warning with sub/class DeprecationWarning or Warning/UserWarning?

  Background: A DeprecationWarning and its subclasses are only raised if Python is started with -W default, -W error, -X dev, etc, or if the error is triggered by code run as __main__ . This makes sense if it is primarily/only intended for application and script developers that use QtPy, and we don't want to nag individual users about the issue. On the other hand, this can also be a user-side issue if the user has installed or set QT_API to a legacy binding, unlike a UserWarning, developers may not see a DeprecationWarning unless they specifically enable and check warnings.

  Suggestion: I'd think few currently maintained packages are likely going to be actively requiring/preferring Qt4 at this point, as opposed to a few individual users still using it, and its untested, difficult to install/aquire and may be broken, so I suggest going straight to a UserWarning/subclass to be very sure any poor souls unfortunate enough to still be stuck on it (but also upgrade to the latest QtPy) are appropriately warned.

• Should we warn if the actually used API is a Qt4 one, the user-selected initial_api is Qt4, or both?

  Background: In many cases, initial_api and API may not be equivalent, and one might be supported in QtPy 2.0.0 but not the other. An unsupported initial_api will not be recognized and cause QtPy 2.x to exist with AssertionError, including if a different API is fallen back to (e.g. if a user/application sets or imports a Qt4 binding but only a Qt5 one is actually installed). Equally, initial_api may be a Qt5 one (the default) but the auto-API selection finds a Qt4 API, which would also not work with QtPy 2.0.0 (e.g. QT_API is unset, so initial_api is the default Qt5 one, but a legacy Qt4 binding is actually imported/installed).

  Suggestion: Given either is likely to cause breakage with QtPy 2.0, I strongly suggest checking both API and initial_api at the end of __init__.py and raising an appropriate warning if either are unsupported.

• Assuming we want to warn on either, do we want to issue a generic warning, or a one specific to the offending binding and whether it was selected and/or actually used?

  Background: A generic warning is simpler and less complex to implement, though longer/less specific and leaning more on users/developers to determine what needs to be done to remedy this. A more specific warning could specify the particular unsupported binding and whether it was selected, actually used or both, but would involve non-trivial complexity.

  Suggestion: I would suggest a simpler best-effort generic warning, along the lines of A deprecated Qt4-based binding was installed, imported or set via the QT_API environment variable, which will no longer be supported in QtPy 2.0.0. Developers should ensure their applications don't depend upon, import or set the QT_API env var to "pyqt", "pyqt4" or "pyside", and users should check that they haven't manually installed, imported or set the same. (the latter half of the last sentence could be elided if only a DeprecationWarning is raised instead of a UserWarning).

[dalthviz]

Thanks @CAM-Gerlach for checking on this and giving some suggestions! From my side I would say:

Point 1 - DeprecationWarning vs UserWarning: I would say DeprecationWarning since when thinking about this I was mostly thinking in devs that use QtPy for their applications.

Point 2 - Tha validation: I will say both since we should cover all usages of Qt4 that will not be supported (either automatic detection or user defined variables).

Point 3 - The message: A generic warning explaining that Qt4 will not be supported anymore (i.e PyQt4 and PySide) should be enough I think

What do you think @ccordoba12 ? Also maybe @andfoy or @steff456 have some ideas regarding approaches to this?

And also @CAM-Gerlach would you like to work on this?

[CAM-Gerlach]

Sure, it should be simple to do, just a if block at the end of __init__() issuing a warning as the above, minus the user portions. Here's what I propose:

LEGACY_APIS = PYQT4_API + PYSIDE_API
if API in LEGACY_APIS or initial_api in LEGACY_APIS:
    warnings.warn(
        "A deprecated Qt4-based binding (PyQt4/PySide) was installed, "
        "imported or set via the QT_API environment variable. "
        "To ensure their applications are still supported in QtPy 2.0, "
        "developers should ensure they don't depend upon, import or "
        "set the QT_API env var to 'pyqt', 'pyqt4' or 'pyside'.",
        DeprecationWarning,
        )

Ideally LEGACY_APIS would be a frozenset rather than a list, but I just made it a list to match the type of the other existing such constants, since it doesn't really matter that much.

[CAM-Gerlach]

We might also want to consider a warning if a version of PyQt (e..g. < 5.9 or at least <5.6), and/or PySide2 (e.g. < 5.12) is used that would similarly be unsupported in QtPy 2.0.0.

[dalthviz]

Sure, it should be simple to do, just a if block at the end of __init__() issuing a warning as the above, minus the user portions. Here's what I propose:

LEGACY_APIS = PYQT4_API + PYSIDE_API
if API in LEGACY_APIS or initial_api in LEGACY_APIS:
    warnings.warn(
        "A deprecated Qt4-based binding (PyQt4/PySide) was installed, "
        "imported or set via the QT_API environment variable. "
        "To ensure their applications are still supported in QtPy 2.0, "
        "developers should ensure they don't depend upon, import or "
        "set the QT_API env var to 'pyqt', 'pyqt4' or 'pyside'.",
        DeprecationWarning,
        )

Ideally LEGACY_APIS would be a frozenset rather than a list, but that matches the type of the other such constants.

I think that sounds good, but lets wait to see what others think.

We might also want to consider a warning if a version of PyQt (e..g. < 5.9 or at least <5.6), and/or PySide2 (e.g. < 5.12) is used that would similarly be unsupported in QtPy 2.0.0.

Thats true, yes, I would say that the acceptable minimum for PyQt could be >=5.9 (since is the lates version in the anaconda channel) but since there are some issues regarding scoped enums with that version for Python 3.6 and 3.7 probably at the end the minimum will be something like >=5.12 as with PySide2. However we need to further discuss this too

[CAM-Gerlach]

since there are some issues regarding scoped enums with that version for Python 3.6 and 3.7 probably at the end the minimum will be something like >=5.12 as with PySide2. However we need to further discuss this too

Yeah, and at least previously that discussion was happening in #233 ; maybe that could be moved here (since my PR will close that one)? Here's the latest comment on there, for reference, my part in the discussion was about adding a run constraint to QtPy 2.0 for the PyQt/Qt versions affected, at least for the conda packages (not sure if there's a way to do so with pip), which would avoid users and applications (e.g. Spyder) pulling in a QtPy version they are incompatible with.

[dalthviz]

maybe that could be moved here (since my PR will close that one)?

Here we are checking the deprecation warning message for Qt4 (which should go only to the 1.x branch) so not related with #233 .

Probably at #233 we will reach a conclusion and then that should be reflected in a new PR and/or could potentially affect the PR you are working on for the CI and test support for PyQt6 and general improvements at #262

That makes sense to you @CAM-Gerlach ?

[CAM-Gerlach]

Here we are checking the deprecation warning message for Qt4 (which should go only to the 1.x branch) so not related with #233 .

Sorry if I was unclear. I assumed this issue was motivated by the fact that Qt4 binding support is removed in QtPy 2.0, untested and deprecated in QtPy 1.x, and EoL upstream. Since all three of these reasons are equally true for Qt5 <5.9/5.12 (the specifics of which are being discussed in #233 ), I thought it might make sense to consider warning for that here as well (considering I'd think we'd want to decide on that before you make a final QtPy 1.x release with this change).

[dalthviz]

Ah yes that makes sense, will reword this issue then 👍

[ccordoba12]

I think that sounds good, but lets wait to see what others think.

I agree with that message but not worded in third person. I mean, I think it's clearer and sounds better in second person:

warnings.warn(
    "A deprecated Qt4-based binding (PyQt4/PySide) was installed, "
    "imported or set via the QT_API environment variable. "
    "To ensure your application is still supported in QtPy 2.0, "
    "please should ensure it doesn't depend upon, import or "
    "set the QT_API env var to 'pyqt', 'pyqt4' or 'pyside'.",
    DeprecationWarning,
)

[dalthviz]

Also @ccordoba12 you think that in the message we should add something regarding Qt5 ? (Like a minimum version)

[CAM-Gerlach]

Yup you're right @ccordoba12 , good point. I only had it that way before because we had different messages for developers and users, and since @dalthviz wanted to mainly target the former, I eliminated that part but didn't reword it.

Fixing an grammar/editing mistake,

warnings.warn(
    "A deprecated Qt4-based binding (PyQt4/PySide) was installed, "
    "imported or set via the QT_API environment variable. "
    "To ensure your application is still supported in QtPy 2.0, "
    "please make sure it doesn't depend upon, import or "
    "set the QT_API env var to 'pyqt', 'pyqt4' or 'pyside'.",
    DeprecationWarning,
)

Also @ccordoba12 you think that in the message we should add something regarding Qt5 ? (Like a minimum version)

☝️ Since Qt bindings < 5.9-5.12 (per the discussion on #233 ) are also equally deprecated and no longer supported in QtPy 2.0, my suggestion was that we should also include those in this warning too.

[dalthviz]

warnings.warn(
"A deprecated Qt4-based binding (PyQt4/PySide) was installed, "
"imported or set via the QT_API environment variable. "
"To ensure your application is still supported in QtPy 2.0, "
"please make sure it doesn't depend upon, import or "
"set the QT_API env var to 'pyqt', 'pyqt4' or 'pyside'.",
DeprecationWarning,
)

I think we can go with this message as it is @CAM-Gerlach

[dalthviz]

Hi @CAM-Gerlach any chance you can work on this with the last message? I think it could be nice to do the 1.11.3 release soon so if you have some time to work on this or if it is ok for someone else to work on this let us know

[CAM-Gerlach]

Sorry, this got lost in my queue somehow. Thanks for the reminder; I'll take care of it now.

One thing, though—this and #281 / #280 were all milestoned on 1.11.3, but #281 also bumped the QtPy 1.x version to 1.12. Was the latter intentional and we should rename the milestone, or is the former still correct? Thanks.

[ccordoba12]

I think we can go with this message as it is @CAM-Gerlach

Agreed.

Also @ccordoba12 you think that in the message we should add something regarding Qt5 ? (Like a minimum version)

I don't think so because qtpy takes care of that for the user.

[CAM-Gerlach]

@ccordoba12 Oh, so we already warn on Qt <5.6 and/or <5.9?

[ccordoba12]

Oh, so we already warn on Qt <5.6 and/or <5.9?

No, we don't. Perhaps we should warn about Qt < 5.9 because the oldest version we can test.

[CAM-Gerlach]

That's what I was suggesting 😏 But we could do that in another PR, since #283 should be ready to go and its a bit less trivial to code.

[dalthviz]

One thing, though—this and #281 / #280 were all milestoned on 1.11.3, but #281 also bumped the QtPy 1.x version to 1.12. Was the latter intentional and we should rename the milestone, or is the former still correct? Thanks.

@CAM-Gerlach see #281 (comment)

[dalthviz]

Checking again the comments, regarding the message for Qt 5 <5.9, that's going to be done by @CAM-Gerlach in a new PR, right? Should we reopen this issue and close it with the merge of that new PR?

[ccordoba12]

regarding the message for Qt 5 <5.9

I think we should leave this message for QtPy 2.0

Should we reopen this issue and close it with the merge of that new PR?

I'm not in favor of that. Instead, let's open a new issue and fix it with a new PR. This issue was about the Qt4 message.

[CAM-Gerlach]

Instead, let's open a new issue and fix it with a new PR.

That also makes the most sense to me 👍 . I can open a new issue.

I think we should leave this message for QtPy 2.0

I'm not sure about that, but we can discuss that in the new issue. I'll try to compile the relevant bits from each of us there to get things started.

[CAM-Gerlach]

Sorry, took a while as I had to take some calls from family members, but opened as #284

[dalthviz]

Thanks @CAM-Gerlach , don't worry and hope everything is going ok

[CAM-Gerlach]

Yeah, thanks are going okay for now at the moment in that department, thanks 👍