From 6fe64c00e6a5442dae2a87958a6a5962f7d9215d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Randy=20D=C3=B6ring?= <30527984+radoering@users.noreply.github.com> Date: Sun, 30 Jul 2023 10:50:54 +0200 Subject: [PATCH] solver: tune heuristics for choosing the next dependency to resolve in a way that dependencies that are not required by another unsatisfied dependency are resolved first --- src/poetry/mixology/version_solver.py | 113 +++++++++++++----- .../version_solver/test_backtracking.py | 36 +++++- 2 files changed, 116 insertions(+), 33 deletions(-) diff --git a/src/poetry/mixology/version_solver.py b/src/poetry/mixology/version_solver.py index e3c122838d1..3872ca39a1b 100644 --- a/src/poetry/mixology/version_solver.py +++ b/src/poetry/mixology/version_solver.py @@ -22,6 +22,7 @@ if TYPE_CHECKING: + from poetry.core.constraints.version import VersionConstraint from poetry.core.packages.project_package import ProjectPackage from poetry.packages import DependencyPackage @@ -140,6 +141,13 @@ def __init__(self, root: ProjectPackage, provider: Provider) -> None: ] = collections.defaultdict(set) self._solution = PartialSolution() + # package name -> package.requires names for an arbitrary version of the package + # This is only used to determine which package to resolve next. + self._requiresHeuristics: dict[str, set[str]] = {} + self._get_package_for_dependency = functools.lru_cache(maxsize=None)( + self._get_package_for_dependency_uncached + ) + @property def solution(self) -> PartialSolution: return self._solution @@ -415,6 +423,30 @@ def _resolve_conflict(self, incompatibility: Incompatibility) -> Incompatibility raise SolveFailure(incompatibility) + def _get_package_for_dependency_uncached( + self, + dependency: Dependency, + # Constraint is unused but required to calculate the cache key, + # because two direct origin dependencies with the same origin are equal + # independent of the constraint. If neglected, we run into + # https://github.com/python-poetry/poetry/issues/7398 again. + constraint: VersionConstraint, + ) -> DependencyPackage | None: + locked = self._provider.get_locked(dependency) + if locked is None: + packages = self._dependency_cache.search_for( + dependency, self._solution.decision_level + ) + package = next(iter(packages), None) + + if package is None: + return None + else: + package = locked + + # complete package so that package.requires are set + return self._provider.complete_package(package) + def _choose_package_version(self) -> str | None: """ Tries to select a version of a required package. @@ -427,6 +459,27 @@ def _choose_package_version(self) -> str | None: if not unsatisfied: return None + transitive_names = set() + for dependency in unsatisfied: + if dependency.name not in self._requiresHeuristics: + package = self._get_package_for_dependency( + dependency, dependency.constraint + ) + if package is None: + # If there are no versions that satisfy the constraint, + # add an incompatibility that indicates that. + self._add_incompatibility( + Incompatibility([Term(dependency, True)], NoVersionsCause()) + ) + return dependency.complete_name + self._requiresHeuristics[dependency.name] = { + dep.name + for dep in package.package.requires + # dependencies with extra may depend on itself (without extra) + if dep.name != dependency.name + } + transitive_names.update(self._requiresHeuristics[dependency.name]) + class Preference: """ Preference is one of the criteria for choosing which dependency to solve @@ -440,17 +493,33 @@ class Preference: LOCKED = 3 DEFAULT = 4 - # Prefer packages with as few remaining versions as possible, - # so that if a conflict is necessary it's forced quickly. - # In order to provide results that are as deterministic as possible - # and consistent between `poetry lock` and `poetry update`, the return value - # of two different dependencies should not be equal if possible. - def _get_min(dependency: Dependency) -> tuple[bool, int, int]: + def _get_min(dependency: Dependency) -> tuple[bool, int, bool, int]: + """ + Heuristic for choosing which dependency to solve next: + 1. Prefer any marker dependencies. + 2. Prefer direct origin dependencies. + 3. Prefer packages that are not constrained + by other unsatisfied dependencies. + 3. Prefer packages with as few remaining versions as possible, + so that if a conflict is necessary it's forced quickly. + + In order to provide results that are as deterministic as possible + and consistent between `poetry lock` and `poetry update`, the return value + of two different dependencies should not be equal if possible. + + Returns a tuple of: + * True if dependency has a specific marker, False if any marker + * preference (see Preference class) + * True if dependency is required by other unsatisfied dependency, else False + * number of package versions that satisfy the dependency + """ + required_by_other = dependency.name in transitive_names + # Direct origin dependencies must be handled first: we don't want to resolve # a regular dependency for some package only to find later that we had a # direct-origin dependency. if dependency.is_direct_origin(): - return False, Preference.DIRECT_ORIGIN, 1 + return False, Preference.DIRECT_ORIGIN, required_by_other, 1 is_specific_marker = not dependency.marker.is_any() @@ -458,7 +527,7 @@ def _get_min(dependency: Dependency) -> tuple[bool, int, int]: if not use_latest: locked = self._provider.get_locked(dependency) if locked: - return is_specific_marker, Preference.LOCKED, 1 + return is_specific_marker, Preference.LOCKED, required_by_other, 1 num_packages = len( self._dependency_cache.search_for( @@ -472,33 +541,21 @@ def _get_min(dependency: Dependency) -> tuple[bool, int, int]: preference = Preference.USE_LATEST else: preference = Preference.DEFAULT - return is_specific_marker, preference, num_packages + return is_specific_marker, preference, required_by_other, num_packages if len(unsatisfied) == 1: dependency = unsatisfied[0] else: dependency = min(*unsatisfied, key=_get_min) - locked = self._provider.get_locked(dependency) - if locked is None: - packages = self._dependency_cache.search_for( - dependency, self._solution.decision_level + package = self._get_package_for_dependency(dependency, dependency.constraint) + if package is None: + # If there are no versions that satisfy the constraint, + # add an incompatibility that indicates that. + self._add_incompatibility( + Incompatibility([Term(dependency, True)], NoVersionsCause()) ) - package = next(iter(packages), None) - - if package is None: - # If there are no versions that satisfy the constraint, - # add an incompatibility that indicates that. - self._add_incompatibility( - Incompatibility([Term(dependency, True)], NoVersionsCause()) - ) - - complete_name = dependency.complete_name - return complete_name - else: - package = locked - - package = self._provider.complete_package(package) + return dependency.complete_name conflict = False for incompatibility in self._provider.incompatibilities_for(package): diff --git a/tests/mixology/version_solver/test_backtracking.py b/tests/mixology/version_solver/test_backtracking.py index 68e8f638746..217c7864d37 100644 --- a/tests/mixology/version_solver/test_backtracking.py +++ b/tests/mixology/version_solver/test_backtracking.py @@ -135,11 +135,13 @@ def test_backjump_to_nearer_unsatisfied_package( def test_traverse_into_package_with_fewer_versions_first( root: ProjectPackage, provider: Provider, repo: Repository ) -> None: - # Dependencies are ordered so that packages with fewer versions are tried - # first. Here, there are two valid solutions (either a or b must be - # downgraded once). The chosen one depends on which dep is traversed first. - # Since b has fewer versions, it will be traversed first, which means a will - # come later. Since later selections are revised first, a gets downgraded. + """ + Dependencies are ordered so that packages with fewer versions are tried + first. Here, there are two valid solutions (either a or b must be + downgraded once). The chosen one depends on which dep is traversed first. + Since b has fewer versions, it will be traversed first, which means a will + come later. Since later selections are revised first, a gets downgraded. + """ root.add_dependency(Factory.create_dependency("a", "*")) root.add_dependency(Factory.create_dependency("b", "*")) @@ -158,6 +160,30 @@ def test_traverse_into_package_with_fewer_versions_first( check_solver_result(root, provider, {"a": "4.0.0", "b": "4.0.0", "c": "2.0.0"}) +def test_traverse_into_package_not_required_by_other_package_first( + root: ProjectPackage, provider: Provider, repo: Repository +) -> None: + """ + Dependencies are ordered so that packages that are not required by other + packages are tried first. + Here, there are two valid solutions (either a or b must be downgraded). + The chosen one depends on which dep is traversed first. + Since a depends on b, a will be traversed first (in spite of the fact + that b has fewer versions), which means a will come later. + Since later selections are revised first, a gets downgraded. + """ + root.add_dependency(Factory.create_dependency("a", "*")) + root.add_dependency(Factory.create_dependency("b", "*")) + + add_to_repo(repo, "a", "1.0", deps={"b": "*"}) + add_to_repo(repo, "a", "2.0", deps={"b": "1.0"}) + add_to_repo(repo, "a", "3.0", deps={"b": "1.0"}) + add_to_repo(repo, "b", "1.0") + add_to_repo(repo, "b", "2.0") + + check_solver_result(root, provider, {"a": "3.0", "b": "1.0"}) + + def test_backjump_past_failed_package_on_disjoint_constraint( root: ProjectPackage, provider: Provider, repo: Repository ) -> None: