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

Address #6692: Only import a Command class when needed #6694

Merged
merged 2 commits into from
Jul 28, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Jul 10, 2019

This implements the (first) suggestion listed in issue #6692, which is not to import all command classes when starting up and only to import the one that's actually needed.

On my system, a bare pip invocation sped up by about 24% with this change.

Also, when inserting the following lines to list all imported pip modules at this point in time:

names = sorted(
    mod.__name__ for mod in sys.modules.values()
        if mod.__name__.startswith('pip._internal.')
)
print(len(names))
for name in names:
    print(name)

after these lines in pip/_internal/__init__.py:

try:
cmd_name, cmd_args = parse_command(args)
except PipError as exc:
sys.stderr.write("ERROR: %s" % exc)
sys.stderr.write(os.linesep)
sys.exit(1)

the following lines are printed:

26
pip._internal.cli
pip._internal.cli.autocompletion
pip._internal.cli.cmdoptions
pip._internal.cli.main_parser
pip._internal.cli.parser
pip._internal.cli.status_codes
pip._internal.commands
pip._internal.configuration
pip._internal.exceptions
pip._internal.locations
pip._internal.models
pip._internal.models.format_control
pip._internal.models.index
pip._internal.models.search_scope
pip._internal.models.target_python
pip._internal.pep425tags
pip._internal.utils
pip._internal.utils.appdirs
pip._internal.utils.compat
pip._internal.utils.deprecation
pip._internal.utils.glibc
pip._internal.utils.hashes
pip._internal.utils.logging
pip._internal.utils.misc
pip._internal.utils.typing
pip._internal.utils.ui

Without the PR, 80 modules were printed.

@cjerdonek cjerdonek added type: enhancement Improvements to functionality type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Jul 10, 2019
src/pip/_internal/commands/__init__.py Outdated Show resolved Hide resolved
src/pip/_internal/commands/__init__.py Outdated Show resolved Hide resolved
@xavfernandez
Copy link
Member

I like it.

Without the PR, 80 modules were printed.

It would also be interesting to check the pip._internal.vendor imports number :)

@cjerdonek
Copy link
Member Author

Thanks!

It would also be interesting to check the pip._internal.vendor imports number :)

Yeah, agreed, I can check.

(Note that there is still more work to be done on this front after this PR because e.g. base_command.py imports PackageFinder even though PackageFinder isn't needed for all commands. Those imports happen when the command object is instantiated, though before this PR they were happening before.)

@cjerdonek
Copy link
Member Author

Here it is (versus 234 before):

55
pip._vendor.appdirs
pip._vendor.colorama
pip._vendor.colorama.ansi
pip._vendor.colorama.ansitowin32
pip._vendor.colorama.initialise
pip._vendor.colorama.win32
pip._vendor.colorama.winterm
pip._vendor.packaging
pip._vendor.packaging.__about__
pip._vendor.packaging._compat
pip._vendor.packaging._structures
pip._vendor.packaging.markers
pip._vendor.packaging.requirements
pip._vendor.packaging.specifiers
pip._vendor.packaging.utils
pip._vendor.packaging.version
pip._vendor.pkg_resources
pip._vendor.pkg_resources.py31compat
pip._vendor.progress
pip._vendor.progress.bar
pip._vendor.progress.spinner
pip._vendor.pyparsing
pip._vendor.retrying
pip._vendor.six
pip._vendor.six.moves
pip._vendor.six.moves.urllib
pip._vendor.six.moves.urllib_parse
pip._vendor.urllib3
pip._vendor.urllib3._collections
pip._vendor.urllib3.connection
pip._vendor.urllib3.connectionpool
pip._vendor.urllib3.contrib
pip._vendor.urllib3.contrib._appengine_environ
pip._vendor.urllib3.exceptions
pip._vendor.urllib3.fields
pip._vendor.urllib3.filepost
pip._vendor.urllib3.packages
pip._vendor.urllib3.packages.six
pip._vendor.urllib3.packages.six.moves
pip._vendor.urllib3.packages.six.moves.urllib
pip._vendor.urllib3.packages.six.moves.urllib_parse
pip._vendor.urllib3.packages.ssl_match_hostname
pip._vendor.urllib3.poolmanager
pip._vendor.urllib3.request
pip._vendor.urllib3.response
pip._vendor.urllib3.util
pip._vendor.urllib3.util.connection
pip._vendor.urllib3.util.queue
pip._vendor.urllib3.util.request
pip._vendor.urllib3.util.response
pip._vendor.urllib3.util.retry
pip._vendor.urllib3.util.ssl_
pip._vendor.urllib3.util.timeout
pip._vendor.urllib3.util.url
pip._vendor.urllib3.util.wait

src/pip/_internal/commands/__init__.py Outdated Show resolved Hide resolved

commands_dict = {
'check': (
'pip._internal.commands.check', 'CheckCommand',
Copy link
Member

Choose a reason for hiding this comment

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

We can probably drop the "pip._internal.commands." prefix and attach it at the point where it's used. It's not a significant compute cost and helps reduce duplication (and chances of typos).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually how I had it originally until I encountered that test case you also discovered further on. :)

Copy link
Member

Choose a reason for hiding this comment

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

One possible downside of not fully-qualifying the name is that automatic refactoring tools will not pick it up, but a quick test with PyCharm (CE 2019.1.3) indicates that even with the string as-is it does not recognize this string when I refactor the module check to check2.

'A helper command used for command completion.',
),
'config': (
'pip._internal.commands.configuration', 'ConfigurationCommand',
Copy link
Member

Choose a reason for hiding this comment

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

It might even make sense to, in a follow up, rename the this module to "config" and then simply reuse the key as the module name, since that's the case for everything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thought!

@@ -26,8 +24,10 @@ class AddFakeCommandMixin(object):

def setup(self):
self.environ_before = os.environ.copy()
commands_dict[FakeCommand.name] = FakeCommand
commands_dict['fake'] = (
'tests.lib.options_helpers', 'FakeCommand', 'fake summary',
Copy link
Member

Choose a reason for hiding this comment

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

Ah! This would be complicated by my suggestion for simplification and DRYing of "pip._internal.commands.". Let's look into that in a follow up then. :)

@cjerdonek cjerdonek force-pushed the remove-command-imports branch 3 times, most recently from ae8f31b to 81a9fae Compare July 11, 2019 02:34
@cjerdonek
Copy link
Member Author

Thanks, @xavfernandez and @pradyunsg. I've updated the PR. (There is also a new unit/test_commands.py test module that I forgot to add in the previous version that you reviewed.)

@pradyunsg
Copy link
Member

@cjerdonek do you mind waiting for 19.2 before merging this?

I'd prefer to keep that release smaller and while I don't expect this to cause any issues, I'd prefer to defer this for a later release. Does that sound fine to you?

@cjerdonek
Copy link
Member Author

I guess that's okay since it doesn't really impact behavior, though it will hold back work on refactoring that depends on this.

@cjerdonek
Copy link
Member Author

By the way, does anyone know why there is a lone test failure in the Travis CI run (Python 3.5) that's caused by an unrelated change? The failure seems to persist even when re-running the tests. I'm attaching a screenshot with some debug info from the error. The test that fails is test_deprecated_message_contains_information().

error

@pradyunsg
Copy link
Member

does anyone know why there is a lone test failure in the Travis CI run (Python 3.5) that's caused by an unrelated change?

Network-related warnings are raised and that test isn't robust to them.

@cjerdonek
Copy link
Member Author

But why does it not happen for other PR's but for this one it seems to be happening reproducibly?

@pradyunsg
Copy link
Member

Hmm... It seems to have passed here now.

@pradyunsg
Copy link
Member

If we see it again, let's file a dedicated issue for it and fix it for good.

This resulted in an approximate 24% speed-up of a vanilla `pip`
invocation on one system (0.477 secs before, 0.363 secs after).
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 27, 2019
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jul 27, 2019
@cjerdonek cjerdonek merged commit 2cd26a2 into pypa:master Jul 28, 2019
@cjerdonek cjerdonek deleted the remove-command-imports branch July 28, 2019 02:52
atugushev added a commit to atugushev/pip-tools that referenced this pull request Aug 5, 2019
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Aug 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 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: enhancement Improvements to functionality type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants