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.

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

Add support for extras in markers #613

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BergLucas
Copy link

@BergLucas BergLucas commented Jul 8, 2023

Resolves: python-poetry#834

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

Description

Hello dear maintainers, it's my first contribution to Poetry so I hope I have done the things right.

As you may have seen in the issue mentioned above, there's a feature that's been requested but hasn't yet been implemented. Since I also need this feature, I decided to try to implement it myself and contribute to the project.

In the issue, it is requested that Poetry implements a feature that allows differing sets of dependency extras in the tool.poetry.extras section. I've chosen to make a slightly different implementation that the ones that were proposed because they had some problems in my opinion but this implementation also fix their problems.

In the pyproject.toml, it would look like that :

[tool.poetry.dependencies]
dependency = { version = "...", optional = true }

[tool.poetry.extras]
feature-a = ["dependency[feature-a]"]
feature-b = ["dependency[feature-b]"]

The extras that are located in the tool.poetry.extras section are additional extras that are added to the ones in tool.poetry.dependencies section. So, if there were an extra named feature-c in dependency, installing my_project[feature-a] would install dependency[feature-a,feature-c].

Why this implementation ?

In the 2 implementations proposed in the issue, I thought that there was some problems.

In the one with aliases, it would required many changes to make it work and the dependencies would have been less clear since we would not be able to read the true package names directly.

In the one with the superset that can be filtered, I thought that it would be a good idea at first but on second thought, it was not a really good idea since it would create backward-incompatible changes. Indeed, if we have a pyproject.toml like the one below, how should it be interpreted ?

[tool.poetry.dependencies]
dependency = { version = "...", optional = true, extras = ["feature-a", "feature-b"] }

[tool.poetry.extras]
no-feature = ["dependency"]

In the current implementation, it would download both features but if we implemented the new feature like a superset that can be filtered, it would have been more logical that it download neither of the features.

That's the reason why I implemented the feature like additional extras that can be added in the tool.poetry.extras section. It is backward-compatible because the feature only applies when we add square brackets at the end of an extra and since it was not possible before, it does not break anything.

In addition, it involves only very minor modifications because we just need to add additional dependencies when we found an extra in the tool.poetry.extras section.

Finally, it also solves another problem that someone mentioned in the issue. Indeed, with this implementation, it is possible to have optional extras by writing something like this :

[tool.poetry.dependencies]
dependency = { version = "..." }

[tool.poetry.extras]
feature = ["dependency[feature]"]

With this configuration, it would install dependency[feature] only when installing my_project[feature] and it would install dependency when installing my_project.

If you are wondering why I choose to add additional extras and not to override extras, it's only because it could create inconsistencies. For example, with the following configuration, installing my_project[feature-c] would not install dependency[feature-a, feature-b] and since it is in the required section, it is not normal :

[tool.poetry.dependencies]
dependency = { version = "...", extras=["feature-a", "feature-b"] }

[tool.poetry.extras]
feature-c = ["dependency[feature-c]"]

Conclusion

Thanks for reading this far and sorry for writing so much. I really wanted to expose my ideas and it took more space than expected.

Please let me know if there are any changes that could be made, and thank you for creating this fantastic software.

@sonarcloud
Copy link

sonarcloud bot commented Jul 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Thank you for the comprehensive issue description. Although I'm not completely sure about the design, you have highlighted why other approaches don't work so it might be the best solution for now. (Sooner or later we'll support PEP 621 anyway, which IMO will make it easier to specify extras.)

You've already considered a lot of things I might have forgotten, but there are still a few issues.

For example poetry check is still an issue because of these lines. When running poetry check on your example pyproject.toml, you will get

Error: Cannot find dependency "transformers[torch,torch-vision]" for extra "torch" in main dependencies.

and so on. (See the inline comments for further issues.)

src/poetry/core/factory.py Outdated Show resolved Hide resolved
src/poetry/core/factory.py Outdated Show resolved Hide resolved
tests/test_factory.py Outdated Show resolved Hide resolved
@dimbleby
Copy link
Contributor

I'm not sure that new syntax is needed, I'd half expect something like this to work today:

foo = [
  { version = "^1.13.1" },
  { version = "^1.13.1", extras = ["their-extra", "their-other-extra"], markers = "extra == 'our-extra'" },
]

presumably it doesn't, but perhaps it wouldn't be so hard to make that work.

@radoering
Copy link
Member

@BergLucas Can you take a look whether dimbleby's proposal is feasible or if there are any drawbacks? (This seems to be more in line with the current design.)

Further, python-poetry/poetry#6409 (comment) looks quite similar to dimbleby's proposal and your use case and it claims that it's working but only from Poetry 1.6.

@BergLucas
Copy link
Author

@radoering It seems that dimbleby's proposal works but only if an empty list is specified for the foo extra.

[tool.poetry.extras]
foo = []

For example, if I use the following pyproject.toml with pip wheel:

[tool.poetry]
name = "extra-package"
version = "1.2.3"
description = "Some description."
authors = ["Your Name <you@example.com>"]
license = "MIT"

[tool.poetry.dependencies]
python = "^3.10"
transformers = [
    { version = "^4.30.2" },
    { version = "^4.30.2", optional = true, extras = ["tf"], markers = "extra == 'tf'"}
]

[tool.poetry.extras]
tf = []

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"

It produces the following METADATA file:

Metadata-Version: 2.1
Name: extra-package
Version: 1.2.3
Summary: Some description.
License: MIT
Author: Your Name
Author-email: you@example.com
Requires-Python: >=3.10,<4.0
Classifier: License :: OSI Approved :: MIT License
Classifier: Programming Language :: Python :: 3
Classifier: Programming Language :: Python :: 3.10
Classifier: Programming Language :: Python :: 3.11
Provides-Extra: tf
Requires-Dist: transformers (>=4.30.2,<5.0.0)
Requires-Dist: transformers[tf] (>=4.30.2,<5.0.0) ; extra == "tf"

It seems that the METADATA file is correct.

I just think it's a bit confusing to specify an empty list in the extras section and then add the extras at the markers in the dependencies.

@radoering
Copy link
Member

I just think it's a bit confusing to specify an empty list in the extras section and then add the extras at the markers in the dependencies.

We could document that it's necessary or change it so that it's not necessary. No strong feelings so far. Either way, we should probably extend the docs mentioning this alternative (more precise) method to define extras.

@dimbleby
Copy link
Contributor

do poetry install --extras tf and poetry install both give the expected results in this example? I guessed that something would go wrong with this, good news if not...

@BergLucas
Copy link
Author

You were right @dimbleby, it doesn't install the extras when I run poetry install --extras ... even if it does create the right METADATA file using pip wheel.

I tested with Poetry 1.6.1.

@dimbleby
Copy link
Contributor

imo debugging and fixing that would be preferable to introducing a new syntax

@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@BergLucas BergLucas changed the title Add additional extras support in the "tool.poetry.extras" section Add support for extra in markers Aug 23, 2023
@BergLucas BergLucas changed the title Add support for extra in markers Add support for extras in markers Aug 23, 2023
@BergLucas
Copy link
Author

BergLucas commented Aug 23, 2023

I've tried to make the changes that you two suggested and it works for poetry-core but it does not when using poetry. I don't really know why but it is weird that Poetry does not use the data from the factory correctly when installing the dependencies. Currently, it tries to install every extra instead of some of them.

For example, with the following pyproject.toml:

[tool.poetry]
name = "extra-package"
version = "1.2.3"
description = "Some description."
authors = ["Your Name <you@example.com>"]
license = "MIT"

[tool.poetry.dependencies]
python = "^3.10"
psycopg = [
    { version = "^3.1.9", optional = true , extras = ["c"], markers = "extra in 'extra-c, extra-pool'"},
    { version = "^3.1.9", optional = true , extras = ["binary"], markers = "extra == 'extra-binary'"},
    { version = "^3.1.9" },
]

[build-system]
requires = ["poetry-core@file://C:/Users/lucas/Documents/GitHub/poetry-core"]
build-backend = "poetry.core.masonry.api"

The factory produces the following data:

requires = [
    <Dependency psycopg[c] (>=3.1.9,<4.0.0)>,
    <Dependency psycopg[binary] (>=3.1.9,<4.0.0)>,
    <Dependency psycopg (>=3.1.9,<4.0.0)>
]
extras = {
    'extra-pool': [<Dependency psycopg[c] (>=3.1.9,<4.0.0)>],
    'extra-c': [<Dependency psycopg[c] (>=3.1.9,<4.0.0)>],
    'extra-binary': [<Dependency psycopg[binary] (>=3.1.9,<4.0.0)>]
}

But when I run poetry install -E extra-binary, it tries to install everything:

$ poetry install -E extra-binary

Package operations: 2 installs, 0 updates, 0 removals

  • Installing psycopg-binary (3.1.10)
  • Installing psycopg-c (3.1.10)

As for my changes, it does not work for extra != '...' at the moment and I'm not sure if I handle the MultiMarker and MultiConstraint classes correctly since it could potentially install dependencies only if multiples extras are enabled (extra == 'a' and extra == 'b').

@@ -188,6 +229,18 @@ def configure_package(
dep.in_extras.append(extra_name)
package.extras[extra_name].append(dep)

for dep in package.requires:
extras = cls._get_extras_from_marker(dep.marker)
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that basically what dep.in_extras should be?

def marker(self, marker: str | BaseMarker) -> None:
from poetry.core.constraints.version import parse_constraint
from poetry.core.packages.utils.utils import convert_markers
from poetry.core.version.markers import BaseMarker
from poetry.core.version.markers import parse_marker
if not isinstance(marker, BaseMarker):
marker = parse_marker(marker)
self._marker = marker
markers = convert_markers(marker)
if "extra" in markers:
# If we have extras, the dependency is optional
self.deactivate()
for or_ in markers["extra"]:
for op, extra in or_:
if op == "==":
self.in_extras.append(canonicalize_name(extra))
elif op == "" and "||" in extra:
for _extra in extra.split(" || "):
self.in_extras.append(canonicalize_name(_extra))

for dep in package.requires:
extras = cls._get_extras_from_marker(dep.marker)
for extra in extras:
if extra not in dep.in_extras:
Copy link
Member

Choose a reason for hiding this comment

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

How does this happen? Shouldn't extra be always be in dep.in_extras?

@radoering
Copy link
Member

I've tried to make the changes that you two suggested and it works for poetry-core but it does not when using poetry. I don't really know why but it is weird that Poetry does not use the data from the factory correctly when installing the dependencies.

Probably, one of these methods is not sufficient for this use case.

@Bibo-Joshi
Copy link

Hi. Just wanted to add two more observations:

  1. pip seems to be happy enough with the produced metadata. pip install . and pip install .[extra] work as expected. only tested with simple extra = '…' markers and tbh I don't quite get yet if the "extra != , and, or, in" cases are poetry specific or not :D.
  2. With poetry 1.7.1, I observe the contrary of

But when I run poetry install -E extra-binary, it tries to install everything:

i.e. poetry install none of the extra requirements

@GOGKI
Copy link

GOGKI commented Mar 1, 2024

Hi, will this PR be merged and closed? Is there an ETA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants