-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Avoid importing buildenv and metadata modules during autocompletion
#13735
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
base: main
Are you sure you want to change the base?
Conversation
| "uninstall", | ||
| ] | ||
| if should_list_installed: | ||
| from pip._internal.metadata import get_default_environment |
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.
It's quite unfortunate to have this import in the middle of the function, but the speedup for autocompletion is quite significant so I think it's worth it. We could also extract the code in this branch into a standalone function, but it seemed like more work than it's worth.
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.
This lazy import should be safe as it is only triggered during tab autocompletion.
| invalid_exc: InvalidRequirement | InvalidVersion, | ||
| ) -> None: | ||
| # packaging.requirements module is slow to import | ||
| from pip._vendor.packaging.requirements import InvalidRequirement |
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.
Not a huge fan of this, but InvalidInstalledPackage should hopefully be a rare code path.
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.
This is more dicey in terms of security, as InvalidInstalledPackage is used in resolvelib. One solution would be to move the definition of this exception down to the resolvelib module itself?
❯ rg InvalidInstalledPackage
exceptions.py
803:class InvalidInstalledPackage(DiagnosticPipError):
resolution/resolvelib/factory.py
26: InvalidInstalledPackage,
287: raise InvalidInstalledPackage(dist=installed_dist, invalid_exc=e)
resolution/resolvelib/candidates.py
16: InvalidInstalledPackage,
421: raise InvalidInstalledPackage(dist=self.dist, invalid_exc=exc) from None|
I've not reviewed this yet but be aware any lazy imports need to be extremely carefully done as there is a potential security issue:
So any lazy import that happens needs to be verified it can not happen after a wheel is installed and before the command finishes executing. |
|
@notatallshaw thanks! Out of the two lazy imports here, one should be safe, the other one I am not so sure. Please see comments inline. |
Speed up tab autocompletion by:
Moving
get_runnable_piptoutils.miscmodule to avoid importingbuild_envmodule. (Per this comment Speedup startup time #4768 (comment))Lazy importing
metadatamodule (which transitively importsemailandurllib.requestwhich are slow)Lazy importing
packaging.requirementsfor another 5% (4ms speedup). I also looked into speeding uppackaging.requirementsitself, but it would be much more work than the simple change in this PR.This also speeds up
pip --helpand possibly other commands.Benchmarks
TODO:
Part of #4768