-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
PR: Unscoped enums access for PyQt6 and other missing PyQt6 compatibility changes #271
Conversation
@dalthviz, I was thinking that in order to avoid the penalty hit involved in loading all those symbols at startup, we could use lazy loading instead. In other words, if the symbol is requested then we load it automatically. If not, it won't be loaded at all. These are some resources that could help you with that:
I think the third one is the most promising. Unfortunately, that requires Python 3.7, but we could make Qtpy depend on that version given that 3.6 is about to reach end of life. |
Given we've supported 3.6 up to this point with the QtPy 2.0 release, 3.6 still sees some significant use (e.g. its still the default Python on many CIs, distros, etc), and the bindings we support all still support it (particularly the Qt6 ones, which would be impossible to use with any version of QtPy on Python 3.6 otherwise, and in fact Qt6 + Py36 would actually see QtPy downgraded to a mutually incompatible version), if we did go ahead with this approach, it would make sense to guard the lazy imports behind a version check, or try-except, IMO. |
Then we will need to precalculate a map of possible import calls without the use of enumeration classes? I'm not totally sure to understand here the use of lazy loading. The penalty involved with what is here is more related to the automatic detection of the enums and setting the values at parent class level than the import it self I think (that's why I was experimenting with |
@dalthviz Just in case you aren't already aware and it would be helpful, |
Just in case doing some benchmarking tests using the
|
This is really cool! I can't see any problem using the |
Thanks for the detailed testing and writeup, @dalthviz ! Important to note, using $ python -m timeit "from qtpy import QtCore, QtWidgets, QtGui"
1 loop, best of 5: 3.42 usec per loop
$ python -m timeit -n 1 -r 1 "from qtpy import QtCore, QtWidgets, QtGui"
1 loop, best of 1: 163 msec per loop That's 5 orders of magnitude off the true value without The
Neglecting that, it looks like promotion has no effect on base |
I pulled this PR and conducted the same tests as @dalthviz under Python 3.9.7 and PyQt6 on my machine (Windows x64, somewhat older) with and without this PR, with the identified issues fixed. Keep in mind that the TL;DR: Total time to import Full Results
|
03563f5
to
e9be297
Compare
e9be297
to
943c622
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me @dalthviz , mostly just some trivial nitpicks and a few more substantive comments/suggestions, though I of course defer to you and @ccordoba12 's on matters of specific subject-matter expertise.
That does requires spyder-ide/qtpy#271
That does requires spyder-ide/qtpy#271
d91d757
to
8ef3b5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dalthviz ; at least from the aspects I am qualified to evaluate, this this LGTM so far; the rest I leave to @ccordoba12 .
My one concern is the modest performance hit, but its not too bad and only affects PyQt5. I thought about suggesting ameliorating this by adding an env variable (unset by default) to disable it, so that applications only using scoped enums, or not using them at all, could avoid the import-time cost, but as that would introduce side effects for other packages using QtPy, on second though I don't think that's a great idea.
Looks like this also fixes everything requested by @tlambert in #274 and added in PR #275 , aside from aliasing |
Co-authored-by: Talley Lambert <[email protected]>
@tlambert03 Looks like @dalthviz incorporated your change, thanks! It also allowed us to unskip a test. |
That does requires spyder-ide/qtpy#271
That does requires spyder-ide/qtpy#271
Hey @ccordoba12 , this should be ready to go unless you have any last concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, I've been testing this at superqt, and it works for all of our Qt6 tests. looking forward to having this released :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dalthviz for this! I left some small comments and suggestions, otherwise looks good to me.
Thanks for the feedback @tlambert03! We're pretty close to release Qtpy 2.0 |
One late thought here... haven't looked to closely at the code, but it would be nice if there were a way to disable unscoped enum access somehow (via env var or otherwise). For instance for internal testing (as opposed to having to lint for it) |
@tlambert03 I'd considered suggesting adding an env variable for this, at the time for performance reasons, but reasoned the extra global state, complexity and interaction with other packages importing QtPy ultimately dissuaded me from that idea. The use case here does sound relatively compelling, so long as the env var was treated as a dev-focused option so as to not break other packages, but the one concern here is that if other packages that yours depends on/interacts with also use QtPy, and don't expect unscoped enums, this will break them too (and probably before yours does, so you likely won't get to see the errors you're looking for if you're trying to test your application). So if we offered this option, we'd want to document this concern and make clear it should be used for local development only (which I can do as part of #61 / #85 that I'm working on for 2.0). |
yeah, this is certainly the concern. I get that it's kind of asking for bug reports on your end 😂 ... so i definitely understand if you want to guard that option carefully.
not sure I followed that. are you saying, if my package did use the "disallow unscoped enums" env var in production? Or just generally? And if generally, isn't that only for Qt < 5.12? As an alternative, I suppose I could just monkeypatch
yep, that would be the idea. Could even put that in the env var name: " |
Hey, sorry for any ambiguity. Basically, if your library/script/application depended upon/imported/invoked etc. another library/script/application that also used QtPy, and that library/script/application expected unscoped enums, it would trigger a crash (regardless of whether your own code used them, and before it was fully exercised to determine that it didn't). If you have relatively tight control over your dep stack or you're interested in patching any upstream issues as well (as it sounds like you are), then this is fine, but there isn't a way to turn it off for just your own code that you control (like a warning filter). If that's fine and expected in your use case, then there's no issue, and as a dev-only best-effort option its an acceptable tradeoff, but it is something we'd want to make sure developers using it are aware of (unless I misunderstand something; my Qt-fu is almost certainly much less than yours). |
if the proposed "disallowed unscoped access" env var was set globally and the user had PyQt6 right? That part makes sense to me ... and why I fully agree that it should be a hidden switch, never used in production. If you're worried about it at all (even just having that possibility around), I don't mind if you'd prefer not to add it. |
Okay, thanks! It seems to make sense to me, with appropriately dire warnings in the docs (and before that, it'll be completely undocumented), but its ultimately up to @ccordoba12 's and @dalthviz 's better judgement. |
I'm fine with adding it if you think it's going to be useful and it's well documented. But let's leave that for another PR if @tlambert03 is willing to give us a hand with it (since he's the one requesting for it). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, great work here @dalthviz!
Part of #233