From 79a91ab542f914686670fb11e1c0cb2dd6f533c8 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 13:25:51 -0400 Subject: [PATCH 1/9] Guard requests imports in exceptions.py under TYPE_CHECKING Running `pip show packaging` now results in 420 entries in sys.modules instead of 607. --- src/pip/_internal/exceptions.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/pip/_internal/exceptions.py b/src/pip/_internal/exceptions.py index 29acd9babc6..0609e450683 100644 --- a/src/pip/_internal/exceptions.py +++ b/src/pip/_internal/exceptions.py @@ -15,7 +15,6 @@ from itertools import chain, groupby, repeat from typing import TYPE_CHECKING, Dict, Iterator, List, Literal, Optional, Union -from pip._vendor.requests.models import Request, Response from pip._vendor.rich.console import Console, ConsoleOptions, RenderResult from pip._vendor.rich.markup import escape from pip._vendor.rich.text import Text @@ -23,6 +22,8 @@ if TYPE_CHECKING: from hashlib import _Hash + from pip._vendor.requests.models import Request, Response + from pip._internal.metadata import BaseDistribution from pip._internal.req.req_install import InstallRequirement @@ -293,8 +294,8 @@ class NetworkConnectionError(PipError): def __init__( self, error_msg: str, - response: Optional[Response] = None, - request: Optional[Request] = None, + response: Optional["Response"] = None, + request: Optional["Request"] = None, ) -> None: """ Initialize NetworkConnectionError with `request` and `response` From a7b6ba694caf10d32e1e2bb3fdbb40f841893e4a Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 14:53:51 -0400 Subject: [PATCH 2/9] Lazy import pip.network in req_file.py This notably helps pip freeze which doesn't need the network unless a URL is given as a requirement file. --- src/pip/_internal/req/req_file.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/pip/_internal/req/req_file.py b/src/pip/_internal/req/req_file.py index 58cf94cd25f..6f474bce49a 100644 --- a/src/pip/_internal/req/req_file.py +++ b/src/pip/_internal/req/req_file.py @@ -25,13 +25,12 @@ from pip._internal.cli import cmdoptions from pip._internal.exceptions import InstallationError, RequirementsFileParseError from pip._internal.models.search_scope import SearchScope -from pip._internal.network.session import PipSession -from pip._internal.network.utils import raise_for_status from pip._internal.utils.encoding import auto_decode from pip._internal.utils.urls import get_url_scheme if TYPE_CHECKING: from pip._internal.index.package_finder import PackageFinder + from pip._internal.network.session import PipSession __all__ = ["parse_requirements"] @@ -133,7 +132,7 @@ def __init__( def parse_requirements( filename: str, - session: PipSession, + session: "PipSession", finder: Optional["PackageFinder"] = None, options: Optional[optparse.Values] = None, constraint: bool = False, @@ -210,7 +209,7 @@ def handle_option_line( lineno: int, finder: Optional["PackageFinder"] = None, options: Optional[optparse.Values] = None, - session: Optional[PipSession] = None, + session: Optional["PipSession"] = None, ) -> None: if opts.hashes: logger.warning( @@ -278,7 +277,7 @@ def handle_line( line: ParsedLine, options: Optional[optparse.Values] = None, finder: Optional["PackageFinder"] = None, - session: Optional[PipSession] = None, + session: Optional["PipSession"] = None, ) -> Optional[ParsedRequirement]: """Handle a single parsed requirements line; This can result in creating/yielding requirements, or updating the finder. @@ -321,7 +320,7 @@ def handle_line( class RequirementsFileParser: def __init__( self, - session: PipSession, + session: "PipSession", line_parser: LineParser, ) -> None: self._session = session @@ -526,7 +525,7 @@ def expand_env_variables(lines_enum: ReqFileLines) -> ReqFileLines: yield line_number, line -def get_file_content(url: str, session: PipSession) -> Tuple[str, str]: +def get_file_content(url: str, session: "PipSession") -> Tuple[str, str]: """Gets the content of a file; it may be a filename, file: URL, or http: URL. Returns (location, content). Content is unicode. Respects # -*- coding: declarations on the retrieved files. @@ -538,6 +537,9 @@ def get_file_content(url: str, session: PipSession) -> Tuple[str, str]: # Pip has special support for file:// URLs (LocalFSAdapter). if scheme in ["http", "https", "file"]: + # Delay importing heavy network modules until absolutely necessary. + from pip._internal.network.utils import raise_for_status + resp = session.get(url) raise_for_status(resp) return resp.url, resp.text From e7c66cdd1d0119097ff35a6984935b7b46472cec Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 15:02:12 -0400 Subject: [PATCH 3/9] Guard PackageFinder imports under TYPE_CHECKING in distributions So freeze doesn't need to import the index/network modules. --- src/pip/_internal/distributions/base.py | 8 +++++--- src/pip/_internal/distributions/sdist.py | 12 +++++++----- src/pip/_internal/distributions/wheel.py | 8 +++++--- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/pip/_internal/distributions/base.py b/src/pip/_internal/distributions/base.py index 6fb0d7b7772..6e4d0c91a90 100644 --- a/src/pip/_internal/distributions/base.py +++ b/src/pip/_internal/distributions/base.py @@ -1,10 +1,12 @@ import abc -from typing import Optional +from typing import TYPE_CHECKING, Optional -from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata.base import BaseDistribution from pip._internal.req import InstallRequirement +if TYPE_CHECKING: + from pip._internal.index.package_finder import PackageFinder + class AbstractDistribution(metaclass=abc.ABCMeta): """A base class for handling installable artifacts. @@ -44,7 +46,7 @@ def get_metadata_distribution(self) -> BaseDistribution: @abc.abstractmethod def prepare_distribution_metadata( self, - finder: PackageFinder, + finder: "PackageFinder", build_isolation: bool, check_build_deps: bool, ) -> None: diff --git a/src/pip/_internal/distributions/sdist.py b/src/pip/_internal/distributions/sdist.py index 4cb0e453969..28ea5cea16c 100644 --- a/src/pip/_internal/distributions/sdist.py +++ b/src/pip/_internal/distributions/sdist.py @@ -1,13 +1,15 @@ import logging -from typing import Iterable, Optional, Set, Tuple +from typing import TYPE_CHECKING, Iterable, Optional, Set, Tuple from pip._internal.build_env import BuildEnvironment from pip._internal.distributions.base import AbstractDistribution from pip._internal.exceptions import InstallationError -from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata import BaseDistribution from pip._internal.utils.subprocess import runner_with_spinner_message +if TYPE_CHECKING: + from pip._internal.index.package_finder import PackageFinder + logger = logging.getLogger(__name__) @@ -29,7 +31,7 @@ def get_metadata_distribution(self) -> BaseDistribution: def prepare_distribution_metadata( self, - finder: PackageFinder, + finder: "PackageFinder", build_isolation: bool, check_build_deps: bool, ) -> None: @@ -66,7 +68,7 @@ def prepare_distribution_metadata( self._raise_missing_reqs(missing) self.req.prepare_metadata() - def _prepare_build_backend(self, finder: PackageFinder) -> None: + def _prepare_build_backend(self, finder: "PackageFinder") -> None: # Isolate in a BuildEnvironment and install the build-time # requirements. pyproject_requires = self.req.pyproject_requires @@ -110,7 +112,7 @@ def _get_build_requires_editable(self) -> Iterable[str]: with backend.subprocess_runner(runner): return backend.get_requires_for_build_editable() - def _install_build_reqs(self, finder: PackageFinder) -> None: + def _install_build_reqs(self, finder: "PackageFinder") -> None: # Install any extra build dependencies that the backend requests. # This must be done in a second pass, as the pyproject.toml # dependencies must be installed before we can call the backend. diff --git a/src/pip/_internal/distributions/wheel.py b/src/pip/_internal/distributions/wheel.py index eb16e25cbcc..bfadd39dcb7 100644 --- a/src/pip/_internal/distributions/wheel.py +++ b/src/pip/_internal/distributions/wheel.py @@ -1,15 +1,17 @@ -from typing import Optional +from typing import TYPE_CHECKING, Optional from pip._vendor.packaging.utils import canonicalize_name from pip._internal.distributions.base import AbstractDistribution -from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata import ( BaseDistribution, FilesystemWheel, get_wheel_distribution, ) +if TYPE_CHECKING: + from pip._internal.index.package_finder import PackageFinder + class WheelDistribution(AbstractDistribution): """Represents a wheel distribution. @@ -33,7 +35,7 @@ def get_metadata_distribution(self) -> BaseDistribution: def prepare_distribution_metadata( self, - finder: PackageFinder, + finder: "PackageFinder", build_isolation: bool, check_build_deps: bool, ) -> None: From 17b6a6419681867b605e2b3227c00c7820287310 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 15:12:43 -0400 Subject: [PATCH 4/9] Import Command directly from base_command.py for pip inspect ... to avoid importing all of the network/index related modules. --- src/pip/_internal/commands/inspect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/commands/inspect.py b/src/pip/_internal/commands/inspect.py index 27c8fa3d5b6..e810c13166b 100644 --- a/src/pip/_internal/commands/inspect.py +++ b/src/pip/_internal/commands/inspect.py @@ -7,7 +7,7 @@ from pip import __version__ from pip._internal.cli import cmdoptions -from pip._internal.cli.req_command import Command +from pip._internal.cli.base_command import Command from pip._internal.cli.status_codes import SUCCESS from pip._internal.metadata import BaseDistribution, get_environment from pip._internal.utils.compat import stdlib_pkgs From 428c86b3ac4b6007138f481cfe91aa4934bfeb7f Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 15:50:32 -0400 Subject: [PATCH 5/9] Add test to ensure we don't regress --- tests/unit/test_commands.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/unit/test_commands.py b/tests/unit/test_commands.py index 7a5c4e8319d..3fa08586067 100644 --- a/tests/unit/test_commands.py +++ b/tests/unit/test_commands.py @@ -1,3 +1,5 @@ +import subprocess +import sys from typing import Callable, List from unittest import mock @@ -120,3 +122,35 @@ def is_requirement_command(command: Command) -> bool: return isinstance(command, RequirementCommand) check_commands(is_requirement_command, ["download", "install", "wheel"]) + + +@pytest.mark.parametrize( + "command", ["cache", "check", "config", "freeze", "hash", "help", "inspect", "show"] +) +def test_no_network_imports(command: str) -> None: + """ + Verify that commands that don't access the network do NOT import network code. + + This helps to reduce the startup time of these commands. + + Note: This won't catch lazy network imports, but it'll catch top-level + network imports which were accidently added (which is the most likely way + to regress anyway). + """ + code = f""" +import sys +from pip._internal.commands import create_command + +command = create_command("{command}") +names = sorted(mod.__name__ for mod in sys.modules.values()) +for mod in names: + print(mod) + """ + proc = subprocess.run( + [sys.executable], encoding="utf-8", input=code, capture_output=True, check=True + ) + imported = proc.stdout.splitlines() + assert not any("pip._internal.index" in mod for mod in imported) + assert not any("pip._internal.network" in mod for mod in imported) + assert not any("requests" in mod for mod in imported) + assert not any("urllib3" in mod for mod in imported) From 72e2366f32a37fad91683777e7723784120db8dd Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 11 Mar 2024 16:32:09 -0400 Subject: [PATCH 6/9] =?UTF-8?q?=F0=9F=93=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- news/4768.feature.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/4768.feature.rst diff --git a/news/4768.feature.rst b/news/4768.feature.rst new file mode 100644 index 00000000000..df69a245a65 --- /dev/null +++ b/news/4768.feature.rst @@ -0,0 +1 @@ +Reduce startup time of commands (e.g. show, freeze) that do not access the network by 15-30%. From 35ccbd1da5e3f294726389df497fd466cb3ecccb Mon Sep 17 00:00:00 2001 From: Richard Si Date: Sun, 7 Apr 2024 20:26:13 -0400 Subject: [PATCH 7/9] Rewrite regression test --- tests/functional/test_cli.py | 48 ++++++++++++++++++++++++++++++++++++ tests/unit/test_commands.py | 34 ------------------------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index 3c3f45d51d1..27cc26e3585 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -1,9 +1,13 @@ """Basic CLI functionality checks. """ +import subprocess +import sys +from pathlib import Path from textwrap import dedent import pytest +from pip._internal.commands import commands_dict from tests.lib import PipTestEnvironment @@ -45,3 +49,47 @@ def test_entrypoints_work(entrypoint: str, script: PipTestEnvironment) -> None: result2 = script.run("fake_pip", "-V", allow_stderr_warning=True) assert result.stdout == result2.stdout assert "old script wrapper" in result2.stderr + + +@pytest.mark.parametrize( + "command", + set(commands_dict).symmetric_difference( + # Exclude commands that are expected to use the network. + # TODO: uninstall and list should only import network modules as needed + {"install", "uninstall", "download", "search", "index", "wheel", "list"} + ), +) +def test_no_network_imports(command: str, tmp_path: Path) -> None: + """ + Verify that commands that don't access the network do NOT import network code. + + This helps to reduce the startup time of these commands. + + Note: This won't catch lazy network imports, but it'll catch top-level + network imports which were accidently added (which is the most likely way + to regress anyway). + """ + file = Path(tmp_path, f"imported_modules_for_{command}") + code = f""" +import runpy +import sys + +sys.argv[1:] = [{command!r}, "--help"] + +try: + runpy.run_module("pip", alter_sys=True, run_name="__main__") +finally: + with open({str(file)!r}, "w") as f: + print(*sys.modules.keys(), sep="\\n", file=f) + """ + subprocess.run( + [sys.executable], + input=code, + encoding="utf-8", + check=True, + ) + imported = file.read_text().splitlines() + assert not any("pip._internal.index" in mod for mod in imported) + assert not any("pip._internal.network" in mod for mod in imported) + assert not any("requests" in mod for mod in imported) + assert not any("urllib3" in mod for mod in imported) diff --git a/tests/unit/test_commands.py b/tests/unit/test_commands.py index 3fa08586067..7a5c4e8319d 100644 --- a/tests/unit/test_commands.py +++ b/tests/unit/test_commands.py @@ -1,5 +1,3 @@ -import subprocess -import sys from typing import Callable, List from unittest import mock @@ -122,35 +120,3 @@ def is_requirement_command(command: Command) -> bool: return isinstance(command, RequirementCommand) check_commands(is_requirement_command, ["download", "install", "wheel"]) - - -@pytest.mark.parametrize( - "command", ["cache", "check", "config", "freeze", "hash", "help", "inspect", "show"] -) -def test_no_network_imports(command: str) -> None: - """ - Verify that commands that don't access the network do NOT import network code. - - This helps to reduce the startup time of these commands. - - Note: This won't catch lazy network imports, but it'll catch top-level - network imports which were accidently added (which is the most likely way - to regress anyway). - """ - code = f""" -import sys -from pip._internal.commands import create_command - -command = create_command("{command}") -names = sorted(mod.__name__ for mod in sys.modules.values()) -for mod in names: - print(mod) - """ - proc = subprocess.run( - [sys.executable], encoding="utf-8", input=code, capture_output=True, check=True - ) - imported = proc.stdout.splitlines() - assert not any("pip._internal.index" in mod for mod in imported) - assert not any("pip._internal.network" in mod for mod in imported) - assert not any("requests" in mod for mod in imported) - assert not any("urllib3" in mod for mod in imported) From d7b1af524c161c0ad5bdf4382f7ba6771e0b4cf9 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Sun, 7 Apr 2024 20:42:32 -0400 Subject: [PATCH 8/9] sigh, make the parametrization values deterministic --- tests/functional/test_cli.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index 27cc26e3585..64bb9c8c60c 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -53,10 +53,12 @@ def test_entrypoints_work(entrypoint: str, script: PipTestEnvironment) -> None: @pytest.mark.parametrize( "command", - set(commands_dict).symmetric_difference( - # Exclude commands that are expected to use the network. - # TODO: uninstall and list should only import network modules as needed - {"install", "uninstall", "download", "search", "index", "wheel", "list"} + sorted( + set(commands_dict).symmetric_difference( + # Exclude commands that are expected to use the network. + # TODO: uninstall and list should only import network modules as needed + {"install", "uninstall", "download", "search", "index", "wheel", "list"} + ) ), ) def test_no_network_imports(command: str, tmp_path: Path) -> None: From 23f4ad59d569f88894224d18c70dd50f1f5d4b0f Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 8 Apr 2024 10:21:19 -0400 Subject: [PATCH 9/9] Use more idiomatic path code in tests/functional/test_cli.py Co-authored-by: Pradyun Gedam --- tests/functional/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/test_cli.py b/tests/functional/test_cli.py index 64bb9c8c60c..482ba735f07 100644 --- a/tests/functional/test_cli.py +++ b/tests/functional/test_cli.py @@ -71,7 +71,7 @@ def test_no_network_imports(command: str, tmp_path: Path) -> None: network imports which were accidently added (which is the most likely way to regress anyway). """ - file = Path(tmp_path, f"imported_modules_for_{command}") + file = tmp_path / f"imported_modules_for_{command}.txt" code = f""" import runpy import sys