-
Notifications
You must be signed in to change notification settings - Fork 14
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
Remove demandimport module #121
Remove demandimport module #121
Conversation
The demandimport module relies on the imp module which has been deprecated in Python 3.4[0] and then removed in Python 3.12[1]. Remove it as it is no longer maintained (last commit in 2019), the performance benefit is marginal and optional imports can be managed in alternative ways (such as using try/expect or by utilising a build system). [0] https://bugs.python.org/issue17177 [1] https://docs.python.org/dev/whatsnew/3.12.html#imp [2] bwesterb/py-demandimport@ca9c116
Thanks for the PR. We used I was aware that demandimport is deprecated, however, we have not found a replacement for lazy imports yet. PEP 690 seemed like it might solve our issue – until it was rejected. So for now, I think removing |
I believe, the main issue of the slow startup was |
It's better to have concrete values, so let's see. Before this patch:
After this patch:
The star-up timing values need to be taken with a pinch of salt as they are dependent on so much but what they show is that removing The
Of these only |
Really interesting results, I believe that Let's get rid of |
I think it did that but the point of the lazy import is to do it uniformly. One stray import will regress the import time again. E.g., the only times this PR touches imports of |
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.
The changes seems fine, I will look at the checks as well, before merging (and before Jirka adds his own review).
After the removal of demandimport, the BPF engine of Tracer was being eagerly imported. However, the underlying bcc package is only an optional requirement and its absence should not crash Perun.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #121 +/- ##
==========================================
- Coverage 98.04% 98.02% -0.02%
==========================================
Files 129 129
Lines 8686 8672 -14
==========================================
- Hits 8516 8501 -15
- Misses 170 171 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
I fixed the bcc import not being optional and the tests are passing now. I think we're ready for a merge.
The demandimport module relies on the imp module which has been deprecated in Python 3.4[0] and then removed in Python 3.12[1]. Remove it as it is no longer maintained (last commit in 2019), the performance benefit is marginal and optional imports can be managed in alternative ways (such as using try/expect or by utilising a build system).
[0] https://bugs.python.org/issue17177
[1] https://docs.python.org/dev/whatsnew/3.12.html#imp
[2] bwesterb/py-demandimport@ca9c116