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

Time to remove support for unscopped enums? #364

Open
j9ac9k opened this issue Aug 31, 2022 · 9 comments
Open

Time to remove support for unscopped enums? #364

j9ac9k opened this issue Aug 31, 2022 · 9 comments

Comments

@j9ac9k
Copy link

j9ac9k commented Aug 31, 2022

Hello QtPy devs,

pyqtgraph maintainer here. When QtPy was rolling out support for Qt6, there was the realization that the enum namespace needed to change as PyQt6 no longer supported unscoped enums. In the discussion on #233 I even made a commentabout how scoped enums would not work on Qt 5.9, which is what the default conda channel shipped at the time.

So, turns out I was a bit mistaken, the limiting factor wasn't Qt 5.9, but the sip module that PyQt library depends on; and it looks like as of sip 4.19.9, support is given for fully scoped enums, including in PyQt 5.9.

In Anaconda 2021.05 release, the sip module they ship with was upgraded from 4.19.8 to 4.19.13. The PyQt version they shipped (5.9.2) has remained the same since Anaconda 5.2.0 (released in May, 2018).

If fully scoped enums now work on all Qt bindings that qtpy support, I think there is a discussion to be had with removing the promote_enums function which should simplify the code-base substantially.

Related info that is new to me but may be relevant to some of you.

As of a month ago, Anaconda default channel has PyQt5 5.15.7

@dalthviz
Copy link
Member

Hi @j9ac9k thank you for the feedback and the info! Indeed, the main reason to allow still the unscoped access was to support the default conda channel qt related packages. Besides that the thing was that PyQt6 was the only binding version removing the unscoped access (as far as I know) so adding the function seemed better.

However yes! This is something to check now then (probably for at least a future minor/mayor version release, v2.3.0/v3.0.0).

From my side, I'm open to the idea (remove unscoped access) if we can keep consistency among all the bindings and version (and also somehow guide projects that still use enums in an unscoped manner). What do you think @ccordoba12 @CAM-Gerlach ?

Also, not totally sure if more people have some opinion or idea about this (maybe people that commented on #233 ?) but happy to hear more opinions and go for a concensus on what could be the best approach for the project and dependent projects regarding the enums access :)

@j9ac9k
Copy link
Author

j9ac9k commented Aug 31, 2022

Unscoped enum access as I understand it is in the process of being deprecated. Riverbank computing is just aggressive about removing support for elements of the Qt library that are being deprecated, so this is a change that eventually everyone will have to do at some point.

@ccordoba12
Copy link
Member

What do you think @ccordoba12 @CAM-Gerlach ?

I have a couple of questions:

  1. Why is this important? Code that depends now on unscoped enums through Qtpy will break in the future?
  2. This seems to be a PyQt6 issue only, or are PySide2/6 going to remove them too?

@j9ac9k
Copy link
Author

j9ac9k commented Aug 31, 2022

Why is this important? Code that depends now on unscoped enums through Qtpy will break in the future?

Promoting those enums if I remember right is a significant performance penalty at import time (several seconds); if this can be avoided, I do think the effort should be made to do so. A distant secondary task benefit would be a cleaner code base.

This seems to be a PyQt6 issue only, or are PySide2/6 going to remove them too?

PySide2 is unlikely to undergo any chances in the future, but certainly a possibility for PySide6.

From this website: https://doc.qt.io/qtforpython/PySide6/QtCore/QEnum.html

We are considering to map all builtin enums and flags to Python enums as well in a later release.

I'm not sure if this translates to them being deprecated in their current capacity.

@ccordoba12
Copy link
Member

Promoting those enums if I remember right is a significant performance penalty at import time (several seconds); if this can be avoided, I do think the effort should be made to do so. A distant secondary task benefit would be a cleaner code base.

Very good points to me. Then I'm +1 on this.

@j9ac9k
Copy link
Author

j9ac9k commented Sep 1, 2022

Hmm... I just tried to benchmark the time it took to convert enums, and it seems to run really quick; I'm likely not benchmarking the right thing because in the pyqtgraph codebase, this was a fairly significant time-sink if memory serves.

@CAM-Gerlach
Copy link
Member

@j9ac9k if you're running multiple imports in the same process to benchmark it (e.g. with timeit) #271 all subsequent imports will be cached and be basically instant, which will skew the benchmarks. See this comment: #271 (comment) that avoid this.

@j9ac9k
Copy link
Author

j9ac9k commented Sep 1, 2022

#271 (comment)

Thanks for linking the comment; I tested PyQt6 bindings in a similar fashion as you describe, but my memory of how long it took was either clearly wrong or due to differences in qtpy's implementation, not relevant.

Looking at your linked comment looks like we can get import times down by a factor of 4, which I would argue is still a worthwhile endeavor as ~600ms to import a python module is quite a lot of time.

@dalthviz
Copy link
Member

Quick update, seems like since PySide6 6.4.0 the recommended way to access enum values is using the scoped version, so using Qt.GlobalColor.green instead of Qt.green for example.

For now PySide6 is giving some compatibility when using the unscoped access syntax when getting the enum value from the classes but not from instances of the classes. So, for example, having the following code:

from PySide6.QtGui import QPainter
painter = QPainter()

Doing painter.Antialiasing fails but doing QPainter.Antialiasing or painter.RenderHint.Antialiasing or QPainter.RenderHint.Antialiasingworks:

imagen

See https://www.qt.io/blog/qt-for-python-release-6.4-is-finally-here The new Enum system section.

@dalthviz dalthviz added this to the v3.0.0 milestone Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants