mottosso / Qt.py

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

Change of types-PySide2 breaks Python 2 compatibility #383

Closed friedererdmann closed 1 year ago

friedererdmann commented 1 year ago

Hi,

We just started seeing none of our pipelines on Python 2 working anymore, after the latest release tag.

types-PySide2 is not available as Python 2 package and is now a required part of the installation of Qt.py (https://github.com/mottosso/Qt.py/blame/master/setup.py#L38).

I believe rolling back the change to the previous line would fix the issue (by making types-PySide2 an extras require not a install requirement).

    extras_require={
        "stubs": ["types-PySide2"],
    },

For now we've pinned our Python2 version to 1.3.7, but ideally we'd continue to get latest packages if it would be possible.

cc @chadrik for visibility.

Thanks!

mottosso commented 1 year ago

Thanks for pinging this, we literally merged and released this version 2 hours ago.

Also leaning on Chadrik for this, would it resolve by making a version of types-PySide2 for Python 2?

Till then, the last version 1.3.7 will work. Sorry about this, we've lost our test suite a while back due to the CI service Travis becoming a paid platform, so this release was not fully tested on all platforms and versions of Python.

friedererdmann commented 1 year ago

Thanks for the quick reply - pinning to 1.3.7 for now unblocked us!

chadrik commented 1 year ago

Sorry! I just uploaded a wheel that supports python 2 and 3: types-PySide2 5.15.2.1.3. I didn't have anything in my pyproject limiting the version and I just missed the fact that the wheel defaulted to python3-only.

mottosso commented 1 year ago

Can you confirm this works for you @friedererdmann?

Given we haven't pinned the specific version of types-PySide2 this should already get picked up. However, that also means there's a chance Qt.py may break again sometime in the future due to updates to types-PySide2. I'd prefer to pin the version in our setup.py, and explicitly update it when necessary. What do you think @chadrik?

friedererdmann commented 1 year ago

Yes, I can confirm it works now. Thanks both of you!

MHendricks commented 1 year ago

I'd prefer to pin the version in our setup.py, and explicitly update it when necessary.

@mottosso , I disagree with pinning external dependencies in setup.py/cfg's main requirements. Sure, it makes it so Qt.py will work with types-PySide2, but what if another package requires a different version of types-PySide2? I'm mostly concerned about explicit style pinning like types-PySide2==5.15.2.1.3, using types-PySide2>=5.15.2.1.3 or types-PySide2!=5.15.2.1.2 is generally acceptable.

I would be ok with pinning it as an optional dependency(extras_require), that gives the flexibility to easily cover the "may break" case.

setup(
    name="Qt.py",
    ...,
    install_requires=["types-PySide2>=5.15.2.1.3"],
    extras_require={
        "pinned": ["types-PySide2==5.15.2.1.3"],
    },
)

This lets you use pip install Qt.py and it just requires a version of types-PySide2. If you use pip install Qt.py[pinned] you know that its an blessed version of the requirements.

The reason I'm concerned about this is more to do with pkg_resources(I assume importlib.metadata is similar) raising exceptions if there are version conflicts with the current pip setup. This breaks pip exe's launching and resolving plugins with pkg_resources.

Note: I couldn't come up with a better name than pinned, it can have any name and I'm not really a fan of the name pinned.

mottosso commented 1 year ago

Oof, yes you are right. The >= requirement doesn't solve for when a future update to types-PySide2 breaks Qt.py.

In that case, can we somehow embed types-PySide2 into Qt.py? Similar to how we already embed the stubs?

https://github.com/mottosso/Qt.py/blob/4a9abaec163f28739cd7639605ba5dbc871f797e/setup.py#L36

MHendricks commented 1 year ago

Why is types-PySide2 a hard requirement? I don't use pycharm so why do I need a collection of .pyi files I'm not using and aren't being used to make Qt.py work? Also, why do I need it for Qt.py to work in maya/nuke/houdini/3ds Max/etc I'm using pip to install Qt.py? It seems to me like the definition of a optional requirement.

I like the original suggestion of just making it an extras_require(In my last comment, I didn't actually read the first comment suggesting it 😃I was just responding to you suggesting pinning it). I won't object to hard pinning the requirement if its only in an extras_require.

Looking at the original PR the reason to make it a hard requirement seems to be:

the reality is that 99% of people who experience broken code completion for Qt.py will not read this documentation, they'll simply be frustrated that it doesn't work out of the box.

This seems to me like its making it hard on everyone just so users don't have to read the readme/install instructions.

In general pinning requirements does not belong in install_requires. https://stackoverflow.com/a/44938662

chadrik commented 1 year ago

the reality is that 99% of people who experience broken code completion for Qt.py will not read this documentation, they'll simply be frustrated that it doesn't work out of the box.

This seems to me like its making it hard on everyone just so users don't have to read the readme/install instructions.

It's not really hard on everyone, though is it? types-PySide2 has zero affect on python runtime, so it can't break your code. The primary thing that could go wrong is that it doesn't support the version of python that you're using with Qt.py, which we just experienced. Sorry for that mistake, but it's now fixed, and it's hard to imagine a scenario where it could become unfixed.

My recommendation is to leave it as it is, and if lots of people complain about it, make it an extra. I will be very surprised if anyone complains at all, let alone more than they've complained on various threads here about completion support.

chadrik commented 1 year ago

In general pinning requirements does not belong in install_requires

I agree with this. We should leave it unpinned. End users can pin it if they need to.