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

Change Platform importing to not be dynamic #65

Closed
wants to merge 0 commits into from

Conversation

Giddius
Copy link

@Giddius Giddius commented Feb 11, 2022

I added an class variable to the API (_platform) and now _set_platform_dir_class works by building a dict from all subclasses of PlatformDirsABC where the key is the classvar _platform and the value is the actual implementation class.

The change is that new implementation need to be imported into the __init__.py, but nothing else needs to be changed.

fixes #55

*Did not run the tests as I don't have experience with tox.

@Julian
Copy link
Member

Julian commented Feb 11, 2022

I believe this was done this way for performance reasons (but @ofek will know for sure). What motivations are there for undoing it, in case there are any specific reasons you have in mind?

@ofek
Copy link
Contributor

ofek commented Feb 11, 2022

I believe this was done this way for performance reasons

Correct.

@Giddius Can you try keeping the conditionals?

@Giddius
Copy link
Author

Giddius commented Feb 12, 2022

I believe this was done this way for performance reasons

Correct.

@Giddius Can you try keeping the conditionals?

Sorry messed up trying to reset my fork, to not make more unecessary format changes. (still new to working with other setups).

My general thought is that, even if this library would support absolutely all platforms, it would be at most about 6? (or how many sys.platform could actually detect) so having the dynamic import system (via importlib) is kind of overkill, especially considering the drawbacks.

I can keep the conditionals and just have the imports as normal import statements but inside conditionals, do not know if this fixes the nuitka problem though. I ran the main file with import profiling (python -X importtime platformdirs/__main__.py) now for the version from this pull request (global-top-imports), the new version (conditional-imports) and the original (dynamic-imports). The Times are for the cumulative of importing platformdirs.

version cumulative us difference us
dynamic-imports 48559 -
global-top-imports 59359 10800
conditional-imports 53647 5088
pick_win_folder as property 50364 1805

Last row shows an implementation where the get_win_folder gets invoked as property and cached by the class itself.

CAVE: I just ran each once and only on my machine(windows) as a quick test, also could be that the dynamic imports do not get profiled correctly.

So even with conditional import it has at least half of the performance problem that the top imports version has.

So before redoing it, are the performance issues for conditional imports to much anyway?

@gaborbernat
Copy link
Contributor

I'm personally -1 on this change and prefer the current system. Not sure what actual problem this is trying to solve besides reducing some perceived complexity 🤔

@Giddius
Copy link
Author

Giddius commented Feb 12, 2022

Only for info, the current system can not be compiled with nuitka because of the dynamic imports. see #55

@RonnyPfannschmidt
Copy link

Nukita has special options to manage this

@ofek
Copy link
Contributor

ofek commented Feb 19, 2022

Fixed by #68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with Nuitka
5 participants