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

Move code out of pip._internal.__init__ #7061

Merged
merged 3 commits into from
Sep 26, 2019

Conversation

chrahunt
Copy link
Member

Related to #4497.

Previously our main function and its associated imports were in pip._internal.__init__ - this makes them unconditional for any access of pip internals. Now main is in pip._internal.main.

This is relevant to:

  1. Improving test execution time - some of our longest tests are in tests.unit.test_build_env, which starts subprocesses and import internals which may not require everything that was in __init__.
  2. Checking module import time - when we import so much in __init__ it is difficult to use -X importtime to isolate slow-loading modules or modules that cause a lot of other transitive imports

A test on my machine shows that import of pip._internal previously took around 120ms before this change, and is now around 60ms.

Moving content out of `__init__` is preferred in general because it
avoids conflicts with module names and unnecessary imports.
@chrahunt chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Sep 21, 2019
@chrahunt chrahunt marked this pull request as ready for review September 21, 2019 21:00
@cjerdonek
Copy link
Member

Is there any reason why __init__.py can't be empty?

@pradyunsg
Copy link
Member

pradyunsg commented Sep 24, 2019

Is there any reason why init.py can't be empty?

There's warning silencing (for warnings from _vendor) we're doing in there that would need to happen before we load any vendored dependencies. I think __init__ is the best spot for doing that.


I wonder if there's merit in doing the following:

  • pip._internal.cli.main_parser -> pip._internal.cli.main (rename)
  • pip._internal.__init__:main -> pip._internal.cli.main:main (move, like this PR)

tests/conftest.py Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Member

cjerdonek commented Sep 24, 2019

There's warning silencing (for warnings from _vendor) we're doing in there that would need to happen before we load any vendored dependencies.

If __init__.py is empty though, we won't have loaded any vendored dependencies. My point is just that @chrahunt says that it's better if __init__.py imports more quickly. So why not make it import as quickly as possible by making it empty. How many ms would that be? 😛

@chrahunt
Copy link
Member Author

chrahunt commented Sep 24, 2019

I think it can be empty, but expect that moving the warning silencing will require a little more thought or discussion. I figured it was better to submit the obvious change alone.

It is definitely worth doing though, you can see these are not cheap imports.

@pradyunsg
Copy link
Member

If __init__.py is empty though, we won't have loaded any vendored dependencies.

Right, but we'd want to silence them before loading them. As @chrahunt pointed out, it's best for doing in a follow up.

None the less, I'm not 100% sure if that's the best place to investigate but, hey, I don't mean to block anyone from making improvements. :)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@chrahunt chrahunt merged commit 3b7d2ee into pypa:master Sep 26, 2019
@chrahunt chrahunt deleted the refactor/move-code-out-of-init branch September 26, 2019 17:36
atugushev added a commit to atugushev/pip-tools that referenced this pull request Sep 26, 2019
It should be imported directly from pip._internal.commands.
See PR pypa/pip#7061 which breaks pip-tools.
@pelson
Copy link
Contributor

pelson commented Oct 15, 2019

Just a heads up: This change will mean an update to ensurepip when they upgrade to pip>=19.3.* as ensurepip uses internal pip interfaces: https://github.com/python/cpython/blob/3.8/Lib/ensurepip/__init__.py#L28

@xavfernandez
Copy link
Member

Thanks :), python/cpython#16782 has been created to tackle this.

@chadwhitacre
Copy link

This also broke (some invocations of?) virtualenv: pypa/virtualenv#1436.

Maybe Pip should promote main to an external API, since other projects seem to depend on it?

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Nov 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants