From 0103fb6ae9962ce2bcb626130d4b0a2c0cf6fa35 Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Sat, 28 Sep 2024 17:37:09 -0400 Subject: [PATCH 1/7] Simplify, fix, and add unit tests for `PipProvider.get_preference` --- .../html/topics/more-dependency-resolution.md | 3 +- .../resolution/resolvelib/provider.py | 46 +++--- .../resolution_resolvelib/test_provider.py | 154 ++++++++++++++++-- 3 files changed, 165 insertions(+), 38 deletions(-) diff --git a/docs/html/topics/more-dependency-resolution.md b/docs/html/topics/more-dependency-resolution.md index b955e2ec114..6b24d889368 100644 --- a/docs/html/topics/more-dependency-resolution.md +++ b/docs/html/topics/more-dependency-resolution.md @@ -156,10 +156,9 @@ grey parts of the graph). Pip's current implementation of the provider implements `get_preference` as follows: -* Prefer if any of the known requirements is "direct", e.g. points to an - explicit URL. * If equal, prefer if any requirement is "pinned", i.e. contains operator ``===`` or ``==``. +* If a requirement is part of the current cause for backtracking. * If equal, calculate an approximate "depth" and resolve requirements closer to the user-specified requirements first. * Order user-specified requirements by the order they are specified. diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index fb0dd85f112..83b518a8aeb 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -118,18 +118,17 @@ def get_preference( The lower the return value is, the more preferred this group of arguments is. - Currently pip considers the following in order: + Currently, pip considers the following in order: - * Prefer if any of the known requirements is "direct", e.g. points to an - explicit URL. - * If equal, prefer if any requirement is "pinned", i.e. contains + * If equal, prefer if any requirement is "pinned", i.e., contains operator ``===`` or ``==``. + * If the a member of backtrack causes. * If equal, calculate an approximate "depth" and resolve requirements closer to the user-specified requirements first. If the depth cannot - by determined (eg: due to no matching parents), it is considered + be determined (e.g., due to no matching parents), it is considered infinite. * Order user-specified requirements by the order they are specified. - * If equal, prefers "non-free" requirements, i.e. contains at least one + * If equal, prefer "non-free" requirements, i.e., contains at least one operator, such as ``>=`` or ``<``. * If equal, order alphabetically for consistency (helps debuggability). """ @@ -144,9 +143,9 @@ def get_preference( if has_information: lookups = (r.get_candidate_lookup() for r, _ in information[identifier]) - candidate, ireqs = zip(*lookups) + _icandidates, ireqs = zip(*lookups) else: - candidate, ireqs = None, () + _icandidates, ireqs = (), () operators = [ specifier.operator @@ -154,27 +153,25 @@ def get_preference( for specifier in specifier_set ] - direct = candidate is not None - pinned = any(op[:2] == "==" for op in operators) + pinned = any(op in ("==", "===") for op in operators) unfree = bool(operators) - try: - requested_order: Union[int, float] = self._user_requested[identifier] - except KeyError: + if identifier in self._user_requested: + requested_order: float = self._user_requested[identifier] + inferred_depth = 1.0 + elif not has_information: requested_order = math.inf - if has_information: - parent_depths = ( - self._known_depths[parent.name] if parent is not None else 0.0 - for _, parent in information[identifier] - ) - inferred_depth = min(d for d in parent_depths) + 1.0 - else: - inferred_depth = math.inf + inferred_depth = math.inf else: - inferred_depth = 1.0 - self._known_depths[identifier] = inferred_depth + requested_order = math.inf + parent_depths = ( + 0.0 if parent is None else self._known_depths[parent.name] + for _, parent in information[identifier] + ) + inferred_depth = min(parent_depths) + 1.0 - requested_order = self._user_requested.get(identifier, math.inf) + + self._known_depths[identifier] = inferred_depth # Requires-Python has only one candidate and the check is basically # free, so we always do it first to avoid needless work if it fails. @@ -187,7 +184,6 @@ def get_preference( return ( not requires_python, - not direct, not pinned, not backtrack_cause, inferred_depth, diff --git a/tests/unit/resolution_resolvelib/test_provider.py b/tests/unit/resolution_resolvelib/test_provider.py index 5f30e2bc1dd..f5fac9c93c4 100644 --- a/tests/unit/resolution_resolvelib/test_provider.py +++ b/tests/unit/resolution_resolvelib/test_provider.py @@ -1,21 +1,30 @@ -from typing import TYPE_CHECKING, List, Optional +import math +from typing import TYPE_CHECKING, Dict, Iterable, Optional, Sequence +from unittest.mock import Mock +import pytest from pip._vendor.resolvelib.resolvers import RequirementInformation from pip._internal.models.candidate import InstallationCandidate from pip._internal.models.link import Link from pip._internal.req.constructors import install_req_from_req_string +from pip._internal.resolution.resolvelib.candidates import REQUIRES_PYTHON_IDENTIFIER from pip._internal.resolution.resolvelib.factory import Factory from pip._internal.resolution.resolvelib.provider import PipProvider from pip._internal.resolution.resolvelib.requirements import SpecifierRequirement if TYPE_CHECKING: - from pip._internal.resolution.resolvelib.provider import PreferenceInformation + from pip._vendor.resolvelib.providers import Preference + from pip._internal.resolution.resolvelib.base import Candidate, Requirement -def build_requirement_information( - name: str, parent: Optional[InstallationCandidate] -) -> List["PreferenceInformation"]: + PreferenceInformation = RequirementInformation[Requirement, Candidate] + + +# Utility to build requirement information +def build_req_info( + name: str, parent: Optional[InstallationCandidate] = None +) -> "PreferenceInformation": install_requirement = install_req_from_req_string(name) # RequirementInformation is typed as a tuple, but it is a namedtupled. # https://github.com/sarugaku/resolvelib/blob/7bc025aa2a4e979597c438ad7b17d2e8a08a364e/src/resolvelib/resolvers.pyi#L20-L22 @@ -23,7 +32,37 @@ def build_requirement_information( requirement=SpecifierRequirement(install_requirement), # type: ignore[call-arg] parent=parent, ) - return [requirement_information] + return requirement_information + + +# Utility to construct a mock factory for PipProvider +def create_mock_factory() -> Factory: + return Factory( + finder=Mock(), + preparer=Mock(), + make_install_req=Mock(), + wheel_cache=Mock(), + use_user_site=False, + force_reinstall=False, + ignore_installed=False, + ignore_requires_python=False, + ) + + +# A helper for constructing the test setup in a single call +def setup_provider( + user_requested: Dict[str, int], initial_depths: Optional[Dict[str, float]] = None +) -> PipProvider: + provider = PipProvider( + factory=create_mock_factory(), + constraints={}, + ignore_dependencies=False, + upgrade_strategy="to-satisfy-only", + user_requested=user_requested, + ) + if initial_depths: + provider._known_depths.update(initial_depths) + return provider def test_provider_known_depths(factory: Factory) -> None: @@ -38,14 +77,14 @@ def test_provider_known_depths(factory: Factory) -> None: user_requested={root_requirement_name: 0}, ) - root_requirement_information = build_requirement_information( + root_requirement_information = build_req_info( name=root_requirement_name, parent=None ) provider.get_preference( identifier=root_requirement_name, resolutions={}, candidates={}, - information={root_requirement_name: root_requirement_information}, + information={root_requirement_name: [root_requirement_information]}, backtrack_causes=[], ) assert provider._known_depths == {root_requirement_name: 1.0} @@ -59,7 +98,7 @@ def test_provider_known_depths(factory: Factory) -> None: ) transitive_requirement_name = "my-transitive-package" - transitive_package_information = build_requirement_information( + transitive_package_information = build_req_info( name=transitive_requirement_name, parent=root_package_candidate ) provider.get_preference( @@ -67,8 +106,8 @@ def test_provider_known_depths(factory: Factory) -> None: resolutions={}, candidates={}, information={ - root_requirement_name: root_requirement_information, - transitive_requirement_name: transitive_package_information, + root_requirement_name: [root_requirement_information], + transitive_requirement_name: [transitive_package_information], }, backtrack_causes=[], ) @@ -76,3 +115,96 @@ def test_provider_known_depths(factory: Factory) -> None: transitive_requirement_name: 2.0, root_requirement_name: 1.0, } + + +@pytest.mark.parametrize( + "identifier, information, backtrack_causes, user_requested, known_depths, expected", + [ + # Test case for REQUIRES_PYTHON_IDENTIFIER + ( + REQUIRES_PYTHON_IDENTIFIER, + {REQUIRES_PYTHON_IDENTIFIER: [build_req_info("python")]}, + [], + {REQUIRES_PYTHON_IDENTIFIER: 1}, + {}, + (False, True, True, 1.0, 1, True, REQUIRES_PYTHON_IDENTIFIER), + ), + # Pinned package with "==" + ( + "pinned-package", + {"pinned-package": [build_req_info("pinned-package==1.0")]}, + [], + {"pinned-package": 1}, + {}, + (True, False, True, 1.0, 1, False, "pinned-package"), + ), + # Package that caused backtracking + ( + "backtrack-package", + {"backtrack-package": [build_req_info("backtrack-package")]}, + [build_req_info("backtrack-package")], + {"backtrack-package": 1}, + {}, + (True, True, False, 1.0, 1, True, "backtrack-package"), + ), + # Depth inference for child package + ( + "child-package", + { + "child-package": [ + build_req_info( + "child-package", + InstallationCandidate( + "parent-package", "1.0", Link("https://parent-package.com") + ), + ) + ], + "parent-package": [build_req_info("parent-package")], + }, + [], + {"parent-package": 1}, + {"parent-package": 1.0}, + (True, True, True, 2.0, math.inf, True, "child-package"), + ), + # Root package requested by user + ( + "root-package", + {"root-package": [build_req_info("root-package")]}, + [], + {"root-package": 1}, + {}, + (True, True, True, 1.0, 1, True, "root-package"), + ), + # Unfree package (with specifier operator) + ( + "unfree-package", + {"unfree-package": [build_req_info("unfree-package<1")]}, + [], + {"unfree-package": 1}, + {}, + (True, True, True, 1.0, 1, False, "unfree-package"), + ), + # Free package (no operator) + ( + "free-package", + {"free-package": [build_req_info("free-package")]}, + [], + {"free-package": 1}, + {}, + (True, True, True, 1.0, 1, True, "free-package"), + ), + ], +) +def test_get_preference( + identifier: str, + information: Dict[str, Iterable["PreferenceInformation"]], + backtrack_causes: Sequence["PreferenceInformation"], + user_requested: Dict[str, int], + known_depths: Dict[str, float], + expected: "Preference", +) -> None: + provider = setup_provider(user_requested, known_depths) + preference = provider.get_preference( + identifier, {}, {}, information, backtrack_causes + ) + assert preference == expected, f"Expected {expected}, got {preference}" From 40f3644c3fba1ae8c676daac9eb3741b713763cb Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Sat, 28 Sep 2024 17:54:41 -0400 Subject: [PATCH 2/7] NEWS ENTRY --- news/12982.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/12982.bugfix.rst diff --git a/news/12982.bugfix.rst b/news/12982.bugfix.rst new file mode 100644 index 00000000000..6d06851ae56 --- /dev/null +++ b/news/12982.bugfix.rst @@ -0,0 +1 @@ +Improve consistency in package selection during dependency resolution. From 904ced57e369245e9bfedc24915e4d96bb320e39 Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Sat, 28 Sep 2024 17:57:31 -0400 Subject: [PATCH 3/7] Linting fix --- src/pip/_internal/resolution/resolvelib/provider.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 83b518a8aeb..26a51f9b79c 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -170,7 +170,6 @@ def get_preference( ) inferred_depth = min(parent_depths) + 1.0 - self._known_depths[identifier] = inferred_depth # Requires-Python has only one candidate and the check is basically From d5ca340460ff4bddd65eba7a92137eeee4303468 Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Sun, 29 Sep 2024 11:25:10 -0400 Subject: [PATCH 4/7] Simplify test setup --- .../resolution_resolvelib/test_provider.py | 48 ++++++------------- 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/tests/unit/resolution_resolvelib/test_provider.py b/tests/unit/resolution_resolvelib/test_provider.py index f5fac9c93c4..c24701912b4 100644 --- a/tests/unit/resolution_resolvelib/test_provider.py +++ b/tests/unit/resolution_resolvelib/test_provider.py @@ -1,6 +1,5 @@ import math from typing import TYPE_CHECKING, Dict, Iterable, Optional, Sequence -from unittest.mock import Mock import pytest from pip._vendor.resolvelib.resolvers import RequirementInformation @@ -21,7 +20,6 @@ PreferenceInformation = RequirementInformation[Requirement, Candidate] -# Utility to build requirement information def build_req_info( name: str, parent: Optional[InstallationCandidate] = None ) -> "PreferenceInformation": @@ -35,36 +33,6 @@ def build_req_info( return requirement_information -# Utility to construct a mock factory for PipProvider -def create_mock_factory() -> Factory: - return Factory( - finder=Mock(), - preparer=Mock(), - make_install_req=Mock(), - wheel_cache=Mock(), - use_user_site=False, - force_reinstall=False, - ignore_installed=False, - ignore_requires_python=False, - ) - - -# A helper for constructing the test setup in a single call -def setup_provider( - user_requested: Dict[str, int], initial_depths: Optional[Dict[str, float]] = None -) -> PipProvider: - provider = PipProvider( - factory=create_mock_factory(), - constraints={}, - ignore_dependencies=False, - upgrade_strategy="to-satisfy-only", - user_requested=user_requested, - ) - if initial_depths: - provider._known_depths.update(initial_depths) - return provider - - def test_provider_known_depths(factory: Factory) -> None: # Root requirement is specified by the user # therefore has an inferred depth of 1 @@ -154,7 +122,7 @@ def test_provider_known_depths(factory: Factory) -> None: "child-package": [ build_req_info( "child-package", - InstallationCandidate( + parent=InstallationCandidate( "parent-package", "1.0", Link("https://parent-package.com") ), ) @@ -202,9 +170,21 @@ def test_get_preference( user_requested: Dict[str, int], known_depths: Dict[str, float], expected: "Preference", + factory: Factory, ) -> None: - provider = setup_provider(user_requested, known_depths) + provider = PipProvider( + factory=factory, + constraints={}, + ignore_dependencies=False, + upgrade_strategy="to-satisfy-only", + user_requested=user_requested, + ) + + if known_depths: + provider._known_depths.update(known_depths) + preference = provider.get_preference( identifier, {}, {}, information, backtrack_causes ) + assert preference == expected, f"Expected {expected}, got {preference}" From 4316fb28fc6cbc67f7d4c4fde9ae1dad607a3dad Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Mon, 21 Oct 2024 20:15:47 -0400 Subject: [PATCH 5/7] Update backtracking sentance in docs --- docs/html/topics/more-dependency-resolution.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/html/topics/more-dependency-resolution.md b/docs/html/topics/more-dependency-resolution.md index 6b24d889368..c80631dc061 100644 --- a/docs/html/topics/more-dependency-resolution.md +++ b/docs/html/topics/more-dependency-resolution.md @@ -158,7 +158,8 @@ follows: * If equal, prefer if any requirement is "pinned", i.e. contains operator ``===`` or ``==``. -* If a requirement is part of the current cause for backtracking. +* If equal, prefer if any requirement is part of the current causes + for backtracking. * If equal, calculate an approximate "depth" and resolve requirements closer to the user-specified requirements first. * Order user-specified requirements by the order they are specified. From 8e75c8891ebfb65cd520530bdd9a2617c21b5649 Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Mon, 21 Oct 2024 20:16:09 -0400 Subject: [PATCH 6/7] Update docstring for get_preference --- src/pip/_internal/resolution/resolvelib/provider.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pip/_internal/resolution/resolvelib/provider.py b/src/pip/_internal/resolution/resolvelib/provider.py index 26a51f9b79c..400d2192a5d 100644 --- a/src/pip/_internal/resolution/resolvelib/provider.py +++ b/src/pip/_internal/resolution/resolvelib/provider.py @@ -122,7 +122,8 @@ def get_preference( * If equal, prefer if any requirement is "pinned", i.e., contains operator ``===`` or ``==``. - * If the a member of backtrack causes. + * If equal, prefer if any requirement is part of the current causes + for backtracking. * If equal, calculate an approximate "depth" and resolve requirements closer to the user-specified requirements first. If the depth cannot be determined (e.g., due to no matching parents), it is considered From b8d1f1129d27b122dff6d49a8de8cc740424a33f Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Mon, 21 Oct 2024 20:22:00 -0400 Subject: [PATCH 7/7] Fix linting --- tests/unit/resolution_resolvelib/test_provider.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/resolution_resolvelib/test_provider.py b/tests/unit/resolution_resolvelib/test_provider.py index c24701912b4..239c9c90cde 100644 --- a/tests/unit/resolution_resolvelib/test_provider.py +++ b/tests/unit/resolution_resolvelib/test_provider.py @@ -2,6 +2,7 @@ from typing import TYPE_CHECKING, Dict, Iterable, Optional, Sequence import pytest + from pip._vendor.resolvelib.resolvers import RequirementInformation from pip._internal.models.candidate import InstallationCandidate