Pass Install Extras to Markers #9553

reesehyde · 2024-07-15T17:24:43Z

Pull Request Check List

Resolves:

This PR exposes the extras supplied at install to the 'extra' marker for dependencies, taking up @radoering's offer to create a PR implementing this functionality to allow switching torch installation versions based on a cuda extra.

It is a duplicate of the feature in python-poetry/poetry-core#613 but it appears that PR is no longer active, with the last updates in August 2023 and an unanswered question about timeline from March 2024. This PR takes a different approach: I noticed that python-poetry/poetry-core#636 added special handling for the extras marker value but that marker isn't populated often, so I opted to populate the value when extras are specified in the installer.

See issues and unit tests for more details but the idea is to, among other things, enable exclusive extras like so:

[tool.poetry]
package-mode = false

[tool.poetry.dependencies]
python = "^3.10"
click = [
    { markers = "extra == 'one' and extra != 'two' and extra != 'three'", version = "8.1.1", optional = true},
    { markers = "extra != 'one' and extra == 'two' and extra != 'three'", version = "8.1.2", optional = true},
    { markers = "extra != 'one' and extra != 'two' and extra == 'three'", version = "8.1.3", optional = true}
 ]

[tool.poetry.extras]
one = ["click"]
two = ["click"]
three = ["click"]

@radoering it looks like you're very familiar with this issue, would you be the right person to review? 🙏
I'm sure I'm missing something here but look forward to iterating!

Edit by @radoering: fix links to poetry-core PRs

Added tests for changed code.
Updated documentation for changed code.

radoering · 2024-07-16T15:45:58Z

I will try to take a closer look at the end of the week.

radoering

This looks really good. Nice work. 👍

It looks like some of the changes are not tested yet or may be redundant. See separate comments for details.

src/poetry/puzzle/provider.py

radoering · 2024-07-20T12:57:17Z

src/poetry/puzzle/provider.py

@@ -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


Seems like that is not tested yet. (Tests succeed without this change.) You probably have to add a test similar to test_solver_resolves_duplicate_dependency_in_extra, which tests this for non root extras.

Ah yep, added test_solver_resolves_duplicate_dependency_in_root_extra(). But this results in both versions of A being selected, see the note about the bug here

Oops, I simply forgot to populate active_extras when constructing the Solver 🤦

I've fixed and validated that this adds test coverage for this change! However I see your comment about test_solver_resolves_duplicate_dependency_in_root_extra() relying on root extra markers being an unrealistic situation. I'd lean towards leaving this as is but happy to replace that test and remove this change if they're unnecessary.

I added another comment concerning the test. I think we can keep the test with slight modifications.

radoering · 2024-07-20T13:05:07Z

src/poetry/puzzle/provider.py

@@ -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


Seems like the non root part is not tested yet. (Tests succeed when replacing dependency.extras with None.) Maybe, this path can be triggered by taking one of your tests and introducing an intermediate package?

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.

You can ignore the comment above, that breaking test relied on conflicting versions of the same package between extras which I don't think is a realistic situation that needs to be covered.

Instead, what I've discovered is that this functionality is already covered: markers are used to populate the Dependency.in_extras field which is then used to skip unneeded packages just below for non-root packages. This is described as the more typical behavior for non-root extras in this comment since root extras aren't passed as marker values into dependencies.

So I've updated this accordingly, to only use self._active_root_extras to construct marker values in the root case while leaving the non-root case as simply self._env.marker_env.

that breaking test relied on conflicting versions of the same package between extras which I don't think is a realistic situation that needs to be covered.

Not sure what I was thinking here, that's absolutely an important case in transitive dependencies as well as root (e.g. what if your dependency has a --cuda flag to switch Pytorch versions, and you want your own --cuda flag to select between the conflicting cpu and cuda versions in the transitive Pytorch dependency).

This was in fact the bug identified earlier. I added a better test based on the earlier one to illustrate it, which covers conflicting dependencies in root and non-root cases. It would pass in the root case (due to duplicate resolution) but fail in the non-root case.

I then pushed a fix in the latest commit, all tests pass but could use validation that it's not unwise: I updated the duplicate resolution logic to raise OverrideNeededErrors when duplicates conflict even with different extras, but not perform overlapping marker resolution in those "same package with different extras" cases.

I found that in that case overlapping marker resolution results in incomplete lockfiles which don't contain all possible package versions, while the override exceptions are needed to prevent solver failure.

I found that in that case overlapping marker resolution results in incomplete lockfiles which don't contain all possible package versions, while the override exceptions are needed to prevent solver failure.

Yes, overlapping marker resolution is not prepared for duplicates with different extras.

I updated the duplicate resolution logic to raise OverrideNeededErrors when duplicates conflict even with different extras, but not perform overlapping marker resolution in those "same package with different extras" cases.

Unfortunately, it is not that simple. The updated logic will only produce correct results if markers are already mutually exclusive. We have to make sure that overrides are not triggered if markers do overlap. I just added a commit with a refined logic. Please take a look.

radoering · 2024-07-20T13:10:10Z

src/poetry/puzzle/provider.py

+            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
+                    )
+                )
+            )


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...

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.

Confirmed that this change is redundant: either this change or the one on lines 558 are sufficient. Happy to leave this in or to remove it since we haven't found a case depending on this case alone.

In the one or two cases I looked at the other change alone took the same number of steps as both changes together, while this change alone took more steps. But I'm not sure if there are some cases I didn't find on a glance where having both changes may reduce the number of steps required.

I took a closer look so that I am quite sure now that we can remove the complete filtering statement (_dependencies = [...]) because incompatibilities_for() is only called after complete_package(), which does the same filtering, so that _dependencies and dependencies will always be equal. I even think we should remove it considering how long it took us to check that it is redundant. Otherwise, it will remain a source of confusion.

reesehyde · 2024-07-25T02:35:27Z

Thank you so much for the great feedback here! All the comments make sense to me, I'm working through it and will submit updates and responses later this week

GeeCastro · 2024-08-22T17:41:32Z

How would this differentiate between the extras for the project itself and the extras for a dependency package?

For the project the scenario would be "I want to install my project with cuda124 extra" which I guess you could interface with the -E/--extra option

poetry install -E cuda124

and define the in the package

[tool.poetry.extras]
cuda124 = ["torch"]

Here your suggestion would add

[tool.poetry.dependencies]
python = "^3.10"
torch = [
    { markers = "extra == 'cuda124'", version = "2.4.0+cu124", optional = true},
                ^^^^^^

And a package-specific extra for example

poetry add "pandas[aws]"

which would show in the extras field as follows

[tool.poetry.dependencies]
python = "^3.11"
pandas = {extras = ["aws"], version = "^2.2.2"}
          ^^^^^

Would the differentiation be made on the following?

project extras in the marker string
package's extras in the extras field

If so, I must have misunderstood the docs because I thought these 2 syntaces express the same requirements/constraints

reesehyde · 2024-08-29T18:55:05Z

I apologize for the long delay here, I had to pause this before I managed to finish all the updates. But it hasn't left my mind, and I'll be able to resume the updates in a couple weeks and will push them up and respond to everything in mid-September! Thanks for the patience

mg515 · 2024-09-25T10:31:42Z

@reesehyde Any update on this? Would love to have this feature available. ^^

reesehyde

I apologize for the long delay here, I've been working on the suggested updates but uncovered an issue in the process. I've added a couple breaking tests which will hopefully help illuminate it. But I'm a little stuck on the best course of action, so would love a push in the right direction if you're able @radoering.

It seems that part of the reason the initial tests passed is because they used conflicting versions of torch, which works only because they then go through the process of merging duplicates since the package name is the same in all cases.

However, dependencies with their own differing extras (e.g. torch[cpu] and torch[cuda]) are not considered duplicates, so they result in a SolverProblemError.
Simply considering them duplicates creates its own issues (e.g. see the test_solver_resolves_duplicate_dependency_in_extra test). So we need some way to do overlapping marker resolution in these cases without breaking other solving cases.

Another possibility would be scoping this PR down to not support conflicting dependency extras, but I'm not certain how that would look.

reesehyde · 2024-10-01T20:26:38Z

src/poetry/puzzle/provider.py

@@ -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


Ah yep, added test_solver_resolves_duplicate_dependency_in_root_extra(). But this results in both versions of A being selected, see the note about the bug here

src/poetry/puzzle/provider.py

reesehyde · 2024-10-11T20:30:25Z

src/poetry/puzzle/provider.py

+            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
+                    )
+                )
+            )


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.

reesehyde · 2024-10-11T20:33:32Z

src/poetry/puzzle/provider.py

@@ -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


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.

reesehyde · 2024-10-11T20:56:13Z

How would this differentiate between the extras for the project itself and the extras for a dependency package?

@GeeCastro this is a great question, you're right that they are basically the same syntax! The problem comes up when you have conflicting extras, e.g. if the cuda extra requires torch == 1.0.0+cuda while the cpu extra requires torch == 1.0.0+cpu. Packages must be resolvable with every possible combination of extras, so if you tried to use both (--extra cpu --extra cuda) it would be impossible since 1.0.0+cuda conflicts with 1.0.0+cpu. Markers provide a way to say "cuda extra requires torch == 1.0.0+cuda, but only if the cpu extra isn't also present" so they don't conflict.

tests/puzzle/test_solver.py

… with active root extras

reesehyde

@radoering thank you again for all the help here! I've pushed updates, responded to all threads, and rebased on main.

I believe everything has test coverage now, there are just small open questions on:

Whether to leave redundant changes in both places or isolate to one place
- Either the L465 change or L558 change are sufficient to pass the new test_installer tests, but not sure if it's worth including both for "marker value correctness" or in cases where having both may shorten the solving steps required.
Whether to leave or remove the changes and test which rely on root extra dependencies having the extra marker value even though it's not generally populated in those cases
- I added test coverage for the L623 change but you mentioned that test scenario may be unrealistic

Edit: The bug referenced was still present, but I added an improved test for it and finally found a fix. Final open question is:

A review of that new logic

reesehyde · 2024-11-13T20:11:42Z

src/poetry/puzzle/provider.py

@@ -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


You can ignore the comment above, that breaking test relied on conflicting versions of the same package between extras which I don't think is a realistic situation that needs to be covered.

Instead, what I've discovered is that this functionality is already covered: markers are used to populate the Dependency.in_extras field which is then used to skip unneeded packages just below for non-root packages. This is described as the more typical behavior for non-root extras in this comment since root extras aren't passed as marker values into dependencies.

So I've updated this accordingly, to only use self._active_root_extras to construct marker values in the root case while leaving the non-root case as simply self._env.marker_env.

reesehyde · 2024-11-13T20:42:42Z

src/poetry/puzzle/provider.py

@@ -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


Oops, I simply forgot to populate active_extras when constructing the Solver 🤦

I've fixed and validated that this adds test coverage for this change! However I see your comment about test_solver_resolves_duplicate_dependency_in_root_extra() relying on root extra markers being an unrealistic situation. I'd lean towards leaving this as is but happy to replace that test and remove this change if they're unnecessary.

tests/puzzle/test_solver.py

reesehyde · 2024-11-13T21:23:00Z

src/poetry/puzzle/provider.py

+            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
+                    )
+                )
+            )


Confirmed that this change is redundant: either this change or the one on lines 558 are sufficient. Happy to leave this in or to remove it since we haven't found a case depending on this case alone.

In the one or two cases I looked at the other change alone took the same number of steps as both changes together, while this change alone took more steps. But I'm not sure if there are some cases I didn't find on a glance where having both changes may reduce the number of steps required.

…nt deps

…itive deps case

radoering · 2024-11-16T07:18:18Z

pre-commit.ci autofix

radoering

I think we are slowly approaching the finish line. 😄

I addressed all open questions in the relevant threads.

radoering · 2024-11-16T07:57:25Z

tests/puzzle/test_solver.py

+    solver = Solver(package, pool, [], [], io, active_root_extras=extra)
+    transaction = solver.solve()


Suggested change

solver = Solver(package, pool, [], [], io, active_root_extras=extra)

transaction = solver.solve()

solver = Solver(

package, pool, [], [package_a1, package_a2], io, active_root_extras=extra

)

with solver.use_environment(MockEnv()):

transaction = solver.solve()

More realistic: Solver will only be called with active_root_extras for installation (not for creating the lock file). In this case, locked will be populated. Further, we are solving for a specific environment.

If we did not solve for a specific environment, the expectation would have to be different.

radoering · 2024-11-16T08:16:27Z

tests/installation/fixtures/with-conflicting-dependencies-root-extras.test

@@ -0,0 +1,24 @@
+[[package]]


This file seems to be unused.

radoering · 2024-11-16T09:04:16Z

src/poetry/puzzle/provider.py

+            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
+                    )
+                )
+            )


I took a closer look so that I am quite sure now that we can remove the complete filtering statement (_dependencies = [...]) because incompatibilities_for() is only called after complete_package(), which does the same filtering, so that _dependencies and dependencies will always be equal. I even think we should remove it considering how long it took us to check that it is redundant. Otherwise, it will remain a source of confusion.

radoering · 2024-11-16T13:39:09Z

src/poetry/puzzle/provider.py

@@ -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


I found that in that case overlapping marker resolution results in incomplete lockfiles which don't contain all possible package versions, while the override exceptions are needed to prevent solver failure.

Yes, overlapping marker resolution is not prepared for duplicates with different extras.

I updated the duplicate resolution logic to raise OverrideNeededErrors when duplicates conflict even with different extras, but not perform overlapping marker resolution in those "same package with different extras" cases.

Unfortunately, it is not that simple. The updated logic will only produce correct results if markers are already mutually exclusive. We have to make sure that overrides are not triggered if markers do overlap. I just added a commit with a refined logic. Please take a look.

radoering · 2024-11-16T13:43:11Z

src/poetry/puzzle/provider.py

@@ -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


I added another comment concerning the test. I think we can keep the test with slight modifications.

Secrus requested a review from radoering July 16, 2024 09:22

radoering requested changes Jul 20, 2024

View reviewed changes

reesehyde commented Oct 11, 2024

View reviewed changes

radoering reviewed Oct 19, 2024

View reviewed changes

tests/puzzle/test_solver.py Show resolved Hide resolved

reesehyde added 17 commits November 13, 2024 16:45

add breaking unit test for conflicting pytorch cpu/cuda extras

301b288

support project extras in markers: populate 'extras' marker including…

b018d44

… with active root extras

add locking test

e1adc53

fix default when active root extras not passed

ce435c7

add unit tests for python-poetry#834

84999c1

consolidate tests

4ae7b5f

add documentation

bc1202c

fix types

fc05bf7

fix formatting

eeb990f

simplify exclusive extras example

a0fcab7

run ruff formatter

63b2798

fix key not in dict format

7a5a3c9

fix pre-commit and full run

c1fe267

add markers assert and remove unnecessary check

2659365

add test for root extras

36fd729

add broken tests for conflicting dependency extras

7646ed0

pr feedback: remove unneeded non-root case and fix tests

ad85d88

reesehyde force-pushed the extra-markers branch from 4734c0f to ad85d88 Compare November 13, 2024 21:46

reesehyde commented Nov 13, 2024

View reviewed changes

fix extra name type in unit test

3a23622

reesehyde added 6 commits November 14, 2024 16:05

better breaking test for conflicting dependencies in root and transie…

00649a0

…nt deps

add overrides for different-extra duplicates to fix conflicting trans…

1326833

…itive deps case

test cleanup: improve docstrings and make extra specs harder to solve

449a2f0

remove redundant test

84482a2

update issue links in unit tests

4e097a9

apply pre-commit hooks

73cf6cd

radoering added 2 commits November 16, 2024 08:20

pre-commit

3811ab5

fix logic for adding overrides for different-extra duplicates

b446675

radoering requested changes Nov 16, 2024

View reviewed changes

		solver = Solver(package, pool, [], [], io, active_root_extras=extra)
		transaction = solver.solve()

Pass Install Extras to Markers #9553

Are you sure you want to change the base?

Pass Install Extras to Markers #9553

Conversation

reesehyde commented Jul 15, 2024 • edited by radoering Loading

Pull Request Check List

radoering commented Jul 16, 2024 • edited Loading

radoering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reesehyde Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reesehyde commented Jul 25, 2024

GeeCastro commented Aug 22, 2024 • edited Loading

reesehyde commented Aug 29, 2024

mg515 commented Sep 25, 2024

reesehyde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reesehyde commented Oct 11, 2024

reesehyde left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reesehyde Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

radoering commented Nov 16, 2024

radoering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reesehyde commented Jul 15, 2024 •

edited by radoering

Loading

radoering commented Jul 16, 2024 •

edited

Loading

reesehyde Nov 13, 2024 •

edited

Loading

GeeCastro commented Aug 22, 2024 •

edited

Loading

reesehyde left a comment •

edited

Loading

reesehyde Nov 13, 2024 •

edited

Loading