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

feat: Implement PEP735 support #2448

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

olivier-lacroix
Copy link
Contributor

@olivier-lacroix olivier-lacroix commented Nov 10, 2024

Fixes #2256 as well as #2149 (maybe we want to create a separate issue to interpret / use markers).

This PR adds handling of [dependency-groups] in pyproject.toml:

  • any group in this table is interpreted as a feature by pixi (similar to current behaviour for optional-dependencies)
  • if a dependency group and a group of optional dependencies have the same name, all dependencies will be added to a single common feature

This PR also changes the behaviour of pixi add --pypi in case of a pyproject.toml manifest:

  • if a feature is specified, the dependency is now added to the [dependency-groups] table instead of the [optional-dependencies] table currently
  • if --platform or --editable is specified, the pypi dependency will be added to the tool.pixi.pypi-dependencies table instead, since native arrays have no support for platform-specific or editable dependencies.
  • it adds a --location argument:
    • Specifiying --location to pixi will force adding the pypi dependency to the tool.pixi.pypi-dependencies table.
    • Specifying --location to optional-dependencies will force adding the pypi dependency to the project.optional-dependencies table (this will only have an effect if a non-default feature is also specified).

This PR also changes the behaviour of pixi init when strating from an existing pyproject.toml manifest:

  • one environment per dependency-group will be added to the environments table, similar to what is being done currently with groups of optional dependencies.

As a drive-by improvement, when removing a pypi dependency from a pyproject.toml manifest, any empty array will be deleted.

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

First off, thank you 💯, amazing contribution! Well documented, Kudos!

I have a few small requests to finish it off.

Also could it print in what location it was added in the pixi add output. e.g.:

❯ pixi add --pypi --location optional-dependencies boltons
✔ Added boltons >=24.1.0, <25
Added these to `optional-dependencies`.

❯ pixi add --pypi numpy
✔ Added numpy >=2.1.3, <3
Added these as `project.dependencies`.

❯ pixi add --pypi --feature test pytest
✔ Added numpy >=2.1.3, <3
Added these as `dependency-groups`.

}

#[derive(Parser, Debug, Clone, PartialEq, ValueEnum)]
pub enum PypiDependencyLocation {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding the dependencies-groups here as well so you are allowed to be explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@@ -73,6 +83,29 @@ pub struct Args {
/// Whether the pypi requirement should be editable
#[arg(long, requires = "pypi")]
pub editable: bool,

/// Where to add the pypi requirement to the manifest
#[arg(long, requires = "pypi")]
Copy link
Contributor

@ruben-arts ruben-arts Nov 12, 2024

Choose a reason for hiding this comment

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

This location thing works but confused me a little as it is also required in combination with --feature
What if we instead extend the CLI with --optional-dependencies [OPTIONAL_GROUP] --dependency-group [DEPENDENCY_GROUP] and only force it into tool.pixi if it has a --editable or --platform

Then it would result in this:

> pixi add --pypi numpy
> pixi add --pypi --optional-dependencies cuda torch
> pixi add --pypi --dependency-group test pytest
> pixi add --pypi "package @ PATH" --editable
> pixi add --pypi --platform linux-64 cmake
> pixi add --pypi --feature sql postgresql-api
> pixi add --pypi --feature default click
[project]
dependencies = ["numpy>=2.1.3,<3"]

[project.optional-dependencies]
cuda = ["torch>=a.b.c, <b"]

[dependency-groups]
test = ["pytest>=8.3.3,<9"]

[tool.pixi.pypi-dependencies]
package = { path = "PATH", editable = true }

[tool.pixi.target.linux-64.pypi-dependencies]
cmake =  ">=a.b.c, <b"

[tool.pixi.feature.sql.pypi-dependencies]
postgresql-api =  ">=a.b.c, <b"

[tool.pixi.pypi-dependencies]
click = ">=a.b.c, <b"

EDIT: added the --feature definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that may be a better option. Two questions @ruben-arts

  • This would mean one could not force the dependency to be added to the tool.pixi.pypi-dependencies table; Is that OK? Note that with this PR, it is possible, for both default or non-default features
  • What would happen if one specifies a —feature ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Features are pixi only so lets make that the tool.pixi.feature.name.pypi-dependencies.
I don't have an opinionated answer for the first question but we could just allow --feature default to force it into tool.pixi.pypi-dependencies.
I added it to this initial post, let me know what you think.

src/cli/upgrade.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Python dependency groups
2 participants