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.

Sign up for GitHub

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

Jump to bottom

Handle restricted dependencies as implicit multiple-constraints dependencies #6969

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+101 −17
Draft

Handle restricted dependencies as implicit multiple-constraints dependencies #6969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 52 additions & 16 deletions src/poetry/puzzle/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,9 @@ def complete_package(
continue
self.search_for_direct_origin_dependency(dep)

active_extras = None if package.is_root() else dependency.extras
_dependencies = self._add_implicit_dependencies(_dependencies, active_extras)
Copy link

Choose a reason for hiding this comment

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

Since _dependencies is only used once, it's probably better to skip the variable assignment, by inlining in into the _add_implicit_dependencies call


dependencies = self._get_dependencies_with_overrides(
_dependencies, dependency_package
)
Expand Down Expand Up @@ -606,7 +609,6 @@ def complete_package(

# For dependency resolution, markers of duplicate dependencies must be
# mutually exclusive.
active_extras = None if package.is_root() else dependency.extras
deps = self._resolve_overlapping_markers(package, deps, active_extras)

if len(deps) == 1:
Expand Down Expand Up @@ -856,6 +858,42 @@ def _is_relevant_marker(
and (not self._env or marker.validate(self._env.marker_env))
)

def _add_implicit_dependencies(
self,
dependencies: Iterable[Dependency],
active_extras: Collection[NormalizedName] | None,
) -> list[Dependency]:
Copy link

Choose a reason for hiding this comment

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

The return value here is used only for _get_dependencies_with_overrides, which (unlike the annotations suggest), should accept any Iterable[Dependency].
So it doesn't need to return a list; any iterable will do:

Suggested change
) -> list[Dependency]:
) -> Iterable[Dependency]:

With this, you can avoid creating the entire dependency list, e.g. using itertools, or by turning this method into a generator .

"""
This handles an edge case where the dependency is not required for a subset
of possible environments. We have to create such an implicit "not required"
dependency in order to not miss other dependencies later.
For instance:
• foo (1.0) ; python == 3.7
• foo (2.0) ; python == 3.8
• bar (2.0) ; python == 3.8
• bar (3.0) ; python == 3.9
the last dependency would be missed without this,
because the intersection with both foo dependencies is empty.

A special case of this edge case is a restricted dependency with a single
constraint, see https://github.com/python-poetry/poetry/issues/5506
for details.
"""
by_name: dict[str, list[Dependency]] = defaultdict(list)
for dep in dependencies:
by_name[dep.name].append(dep)
for _name, deps in by_name.items():
marker = marker_union(*[d.marker for d in deps])
if marker.is_any():
continue
Comment on lines +886 to +888
Copy link

Choose a reason for hiding this comment

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

Is marker_union also needed when e.g. len(deps) == 1?
Because, at a glance, marker_union looks like a rather expensive function call.

inverted_marker = marker.invert()
if self._is_relevant_marker(inverted_marker, active_extras):
# Set constraint to empty to mark dependency as "not required".
inverted_marker_dep = deps[0].with_constraint(EmptyConstraint())
inverted_marker_dep.marker = inverted_marker
deps.append(inverted_marker_dep)
Comment on lines +882 to +894
Copy link

Choose a reason for hiding this comment

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

These loops could be merged if 1) you use itertools.groupby, with e.g. operators.attrgetter('name') as key function, and 2) turn this method into a generator (e.g. with a yield from in the first if statement, and yield in the second).
This way you can avoid creating temporary lists altogether, for a significant speedup.

return [dep for deps in by_name.values() for dep in deps]
Copy link

Choose a reason for hiding this comment

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

Suggested change
return [dep for deps in by_name.values() for dep in deps]
return itertools.chain.from_iterable(by_name.values())


def _resolve_overlapping_markers(
self,
package: Package,
Expand All @@ -878,7 +916,20 @@ def _resolve_overlapping_markers(
dependencies = self._merge_dependencies_by_constraint(dependencies)

new_dependencies = []

# We have can sort out the implicit "not required" dependency determined
# in _add_implicit_dependencies, because it does not overlap with
# any other dependency for sure.
for i, dep in enumerate(dependencies):
if dep.constraint.is_empty():
new_dependencies.append(dependencies.pop(i))
Copy link

Choose a reason for hiding this comment

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

The list.pop method can be very slow operation, and I think that it can be avoided here, by using a "blacklist" approach, e.g.

blacklist = set()
for dep in dependencies:
    if dep.constraint.is_empty():
        blacklist.add(dep)
        break

Then later on in itertools.product use
repeat=len(dependencies) - len(blacklist).
And when looping over dep in dependencies again, simply skip it if dep in blacklist.

This avoids the list.pop operation, which has a time-complexity O(n), by relying on set.__contains__, which is only O(1).

break

for uses in itertools.product([True, False], repeat=len(dependencies)):
if not any(uses):
# handled by the implicit "not required" dependency
continue

# intersection of markers
# For performance optimization, we don't just intersect all markers at once,
# but intersect them one after the other to get empty markers early.
Expand Down Expand Up @@ -917,21 +968,6 @@ def _resolve_overlapping_markers(
# conflict in overlapping area
raise IncompatibleConstraintsError(package, *used_dependencies)

if not any(uses):
# This is an edge case where the dependency is not required
# for the resulting marker. However, we have to consider it anyway
# in order to not miss other dependencies later, for instance:
# • foo (1.0) ; python == 3.7
# • foo (2.0) ; python == 3.8
# • bar (2.0) ; python == 3.8
# • bar (3.0) ; python == 3.9
# the last dependency would be missed without this,
# because the intersection with both foo dependencies is empty.

# Set constraint to empty to mark dependency as "not required".
constraint = EmptyConstraint()
used_dependencies = dependencies

# build new dependency with intersected constraint and marker
# (and correct source)
new_dep = (
Expand Down
2 changes: 1 addition & 1 deletion tests/installation/fixtures/update-with-locked-extras.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ files = []

[package.dependencies]
"B" = {version = "^1.0", optional = true}
"C" = {version = "^1.0", markers = "python_version >= \"2.7\" and python_version < \"2.8\""}
"C" = {version = ">=1.0,<2.0", markers = "python_version >= \"2.7\" and python_version < \"2.8\""}

[package.extras]
foo = ["B"]
Expand Down
48 changes: 48 additions & 0 deletions tests/puzzle/test_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2101,6 +2101,54 @@ def test_duplicate_path_dependencies_same_path(
check_solver_result(transaction, [{"job": "install", "package": demo1}])


@pytest.mark.parametrize(
"marker1, marker2",
[
("python_version < '3.7'", "python_version >= '3.7'"),
("sys_platform == 'linux'", "sys_platform != 'linux'"),
(
"python_version < '3.7' and sys_platform == 'linux'",
"python_version >= '3.7' and sys_platform == 'linux'",
Comment on lines +2107 to +2111
Copy link

Choose a reason for hiding this comment

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

I don't think python<3.7 is relevant anymore

),
],
)
def test_solver_restricted_dependencies_with_empty_marker_intersection(
marker1: str, marker2: str, solver: Solver, repo: Repository, package: Package
) -> None:
package.add_dependency(
Factory.create_dependency("A1", {"version": "1.0", "markers": marker1})
)
package.add_dependency(
Factory.create_dependency("A2", {"version": "1.0", "markers": marker2})
)

package_a1 = get_package("A1", "1.0")
package_a1.add_dependency(Factory.create_dependency("B", {"version": "<2.0"}))

package_a2 = get_package("A2", "1.0")
package_a2.add_dependency(Factory.create_dependency("B", {"version": ">=2.0"}))

package_b10 = get_package("B", "1.0")
package_b20 = get_package("B", "2.0")

repo.add_package(package_a1)
repo.add_package(package_a2)
repo.add_package(package_b10)
repo.add_package(package_b20)

transaction = solver.solve()

check_solver_result(
transaction,
[
{"job": "install", "package": package_b10},
{"job": "install", "package": package_b20},
{"job": "install", "package": package_a1},
{"job": "install", "package": package_a2},
],
)


def test_solver_fails_if_dependency_name_does_not_match_package(
solver: Solver, repo: Repository, package: ProjectPackage
) -> None:
Expand Down