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.

… with active root extras

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.

radoering · 2024-07-20T12:16:06Z

src/poetry/puzzle/provider.py

+        """
+        Marker values, per:
+        1. marker_env of `self._env`
+        2. 'extras' will be added to the 'extra' marker if not already present


cannot happen, can it?

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

src/poetry/puzzle/provider.py

+            if "extra" not in result:
+                result["extra"] = set(extras)
+            else:
+                result["extra"] = set(result["extra"]).union(extras)


I do not think that it is possible that extra is already present. This might even be an error. Thus, maybe we just assert "extra" not in result.

radoering · 2024-07-20T12:21:39Z

src/poetry/puzzle/provider.py

@@ -860,7 +881,7 @@ def _is_relevant_marker(
                get_python_constraint_from_marker(marker)
            )
            and (active_extras is None or marker.validate({"extra": active_extras}))
-            and (not self._env or marker.validate(self._env.marker_env))
+            and (not self._env or marker.validate(self._marker_values(active_extras)))


This change should not be necessary because extras are already handled one line above (no matter if self._env is set or not).

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.

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?

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

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

reesehyde added 13 commits July 13, 2024 20:19

add breaking unit test for conflicting pytorch cpu/cuda extras

c35676f

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

664daa8

… with active root extras

add locking test

900913c

fix default when active root extras not passed

1ee3a90

add unit tests for python-poetry#834

6b55283

consolidate tests

57ed6f7

add documentation

dc86259

fix types

afdab51

fix formatting

6b3f24e

simplify exclusive extras example

de6e032

run ruff formatter

ec5592f

fix key not in dict format

4de59bd

fix pre-commit and full run

203c037

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

radoering requested changes Jul 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Install Extras to Markers #9553

Pass Install Extras to Markers #9553

reesehyde commented Jul 15, 2024 •

edited by radoering

Loading

radoering commented Jul 16, 2024 •

edited

Loading

radoering left a comment

radoering Jul 20, 2024

radoering Jul 20, 2024

radoering Jul 20, 2024

radoering Jul 20, 2024

radoering Jul 20, 2024

radoering Jul 20, 2024

reesehyde commented Jul 25, 2024

GeeCastro commented Aug 22, 2024 •

edited

Loading

reesehyde commented Aug 29, 2024

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

radoering Jul 20, 2024

Choose a reason for hiding this comment

radoering Jul 20, 2024

Choose a reason for hiding this comment

radoering Jul 20, 2024

Choose a reason for hiding this comment

radoering Jul 20, 2024

Choose a reason for hiding this comment

radoering Jul 20, 2024

Choose a reason for hiding this comment

radoering Jul 20, 2024

Choose a reason for hiding this comment

reesehyde commented Jul 25, 2024

GeeCastro commented Aug 22, 2024 • edited Loading

reesehyde commented Aug 29, 2024

reesehyde commented Jul 15, 2024 •

edited by radoering

Loading

radoering commented Jul 16, 2024 •

edited

Loading

GeeCastro commented Aug 22, 2024 •

edited

Loading