-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Pass Install Extras to Markers #9553
base: main
Are you sure you want to change the base?
Changes from all commits
c35676f
664daa8
900913c
1ee3a90
6b55283
57ed6f7
dc86259
afdab51
6b3f24e
de6e032
ec5592f
4de59bd
203c037
c95e3cf
8490954
fd1c1dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
from collections import defaultdict | ||
from contextlib import contextmanager | ||
from typing import TYPE_CHECKING | ||
from typing import Any | ||
from typing import ClassVar | ||
from typing import cast | ||
|
||
|
@@ -117,6 +118,7 @@ def __init__( | |
*, | ||
installed: list[Package] | None = None, | ||
locked: list[Package] | None = None, | ||
active_root_extras: Collection[NormalizedName] | None = None, | ||
) -> None: | ||
self._package = package | ||
self._pool = pool | ||
|
@@ -133,6 +135,9 @@ def __init__( | |
self._direct_origin_packages: dict[str, Package] = {} | ||
self._locked: dict[NormalizedName, list[DependencyPackage]] = defaultdict(list) | ||
self._use_latest: Collection[NormalizedName] = [] | ||
self._active_root_extras = ( | ||
frozenset(active_root_extras) if active_root_extras is not None else None | ||
) | ||
|
||
self._explicit_sources: dict[str, str] = {} | ||
for package in locked or []: | ||
|
@@ -457,7 +462,16 @@ def incompatibilities_for( | |
for dep in dependencies | ||
if dep.name not in self.UNSAFE_PACKAGES | ||
and self._python_constraint.allows_any(dep.python_constraint) | ||
and (not self._env or dep.marker.validate(self._env.marker_env)) | ||
and ( | ||
not self._env | ||
or dep.marker.validate( | ||
self._marker_values( | ||
self._active_root_extras | ||
if dependency_package.package.is_root() | ||
else dependency_package.dependency.extras | ||
) | ||
) | ||
) | ||
] | ||
dependencies = self._get_dependencies_with_overrides(_dependencies, package) | ||
|
||
|
@@ -541,7 +555,12 @@ def complete_package( | |
if dep.name in self.UNSAFE_PACKAGES: | ||
continue | ||
|
||
if self._env and not dep.marker.validate(self._env.marker_env): | ||
active_extras = ( | ||
self._active_root_extras if package.is_root() else dependency.extras | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the non root part is not tested yet. (Tests succeed when replacing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great catch! While trying to add a test for this I uncovered an issue referenced in a top-level comment. The breaking test I've added doesn't include the intermediate package for simplicity but I can add it as we get to the bottom of that. |
||
) | ||
if self._env and not dep.marker.validate( | ||
self._marker_values(active_extras) | ||
): | ||
continue | ||
|
||
if not package.is_root() and ( | ||
|
@@ -601,7 +620,9 @@ def complete_package( | |
|
||
# For dependency resolution, markers of duplicate dependencies must be | ||
# mutually exclusive. | ||
active_extras = None if package.is_root() else dependency.extras | ||
active_extras = ( | ||
self._active_root_extras if package.is_root() else dependency.extras | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like that is not tested yet. (Tests succeed without this change.) You probably have to add a test similar to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yep, added |
||
) | ||
deps = self._resolve_overlapping_markers(package, deps, active_extras) | ||
|
||
if len(deps) == 1: | ||
|
@@ -953,3 +974,17 @@ def _resolve_overlapping_markers( | |
# dependencies by constraint again. After overlapping markers were | ||
# resolved, there might be new dependencies with the same constraint. | ||
return self._merge_dependencies_by_constraint(new_dependencies) | ||
|
||
def _marker_values( | ||
self, extras: Collection[NormalizedName] | None = None | ||
) -> dict[str, Any]: | ||
""" | ||
Marker values, from `self._env` if present plus the supplied extras | ||
|
||
:param extras: the values to add to the 'extra' marker value | ||
""" | ||
result = self._env.marker_env.copy() if self._env is not None else {} | ||
if extras is not None: | ||
assert "extra" not in result, "'extra' marker key is already present in environment" | ||
result["extra"] = set(extras) | ||
return result |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
[[package]] | ||
name = "dependency" | ||
version = "1.1.0" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[[package]] | ||
name = "dependency" | ||
version = "1.2.0" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[extras] | ||
extra-one = ["dependency"] | ||
extra-two = ["dependency"] | ||
|
||
[metadata] | ||
python-versions = "*" | ||
lock-version = "2.0" | ||
content-hash = "123456789" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
[[package]] | ||
name = "demo" | ||
version = "1.0.0" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[package.extras] | ||
demo-extra-one = ["transitive-dep-one"] | ||
demo-extra-two = ["transitive-dep-two"] | ||
|
||
[package.dependencies] | ||
transitive-dep-one = {version = "1.1.0", optional = true} | ||
transitive-dep-two = {version = "1.2.0", optional = true} | ||
|
||
[[package]] | ||
name = "transitive-dep-one" | ||
version = "1.1.0" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[[package]] | ||
name = "transitive-dep-two" | ||
version = "1.2.0" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[extras] | ||
extra-one = ["demo", "demo"] | ||
extra-two = ["demo", "demo"] | ||
|
||
[metadata] | ||
python-versions = "*" | ||
lock-version = "2.0" | ||
content-hash = "123456789" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
[[package]] | ||
name = "torch" | ||
version = "1.11.0+cpu" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[package.source] | ||
reference = "pytorch-cpu" | ||
type = "legacy" | ||
url = "https://download.pytorch.org/whl/cpu" | ||
|
||
[[package]] | ||
name = "torch" | ||
version = "1.11.0+cuda" | ||
description = "" | ||
optional = true | ||
python-versions = "*" | ||
files = [] | ||
|
||
[package.source] | ||
reference = "pytorch-cuda" | ||
type = "legacy" | ||
url = "https://download.pytorch.org/whl/cuda" | ||
|
||
[extras] | ||
cpu = ["torch", "torch"] | ||
cuda = ["torch", "torch"] | ||
|
||
[metadata] | ||
python-versions = "*" | ||
lock-version = "2.0" | ||
content-hash = "123456789" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change is not required by any test. To be honest, I have currently no idea if this change is redundant or if a test is missing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I wasn't sure what the best policy was here: I just updated the places where I saw markers being evaluated to include extras. What I found is that of the three changes in
provider.py
, any one of them is sufficient to pass the first round of tests. So I think there is some redundancy, but my thought is to not eliminate anything yet due to the issue with uncovered around not resolving conflicts with their own different extras.