-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Implement a --group
option for installing from [dependency-groups]
found in pyproject.toml
files
#13065
base: main
Are you sure you want to change the base?
Conversation
Exciting to see this up!
To clarify for those that have not read the discussion, I feel like we did not reach consensus that automatically discovering a I'll disclaim that I am one of the people opposed, but so was @pfmoore — I feel like this merits further discussion.
As a minor note, we have not added dependency group support to the |
I agree with that assessment. I'm proposing it as an initial option, but am 100% ready to adjust course if there's an alternative with pip maintainer support.
Maybe I'm inferring too much, but does this suggest that there's an alternative UX you would find less surprising? I've thought up alternatives, but none of them seem clearly better to me. And 👍 to the note about keeping |
I'm OK with It did occur to me when this PR was submitted that we hadn't reached agreement on auto-discovering I don't know the right answer here. It feels to me like the "traditional" role of pip, as a standalone installer, is being eroded by the ecosystem drift towards the "everything is a project" model. I don't like that, but maybe at some point I have to accept the inevitable and stop blocking things based on an outdated view of what pip is for. I would definitely like to know what the other @pypa/pip-committers think about this, though. There's also a wider discussion about how pip fits into the modern packaging ecosystem that I think the maintainers need to have, but that hasn't happened yet either. I don't want to block things on the basis of some grand philosophical debate, so if someone says "let's just do it for now and worry about the bigger picture later", I'm OK with that. |
What about adding a FWIW, I also like Original post:I think users would like for this to work without having to add some new pip install -e. --group test However, then I think most people would expect this to work: pip install . --group test which is a lot more dubious; another clearer example would be pip install ./some/thing ./other/thing --group test Which isn't clear to me at all which of the two directories would be used - or both. Or neither and just the current directory. If there was a pip install -e. --project-dir . --group test But that's fully explicit. I think either defaulting the project-dir to FWIW, I'm pretty happy if this gets in in any state, and I like shorter CLIs if possible. :) |
This was one option I considered, as well as, similar,
I read some of your comments there as weakly supporting the idea of
I do not like the idea that Is this perhaps an argument in favor of accepting the pyproject file (or dir?) as an option? Any user who asked "why doesn't I'm much more comfortable with the idea of adding an option for the A file option keeps A dir option adds the notion that there is a "project directory", and I'm much less clear on what that means. It would only drive |
I made two concrete recommendations in the issue (both of which are similar to @henryiii's suggestions here):
For discussion purposes, let's list a couple more proposals:
I don't want to just repeat the discussion from there — I think there's a fair bit of context on the upsides and downsides to each of those in the thread already. Unfortunately, I don't think there's an obviously superior option here. I'll try to summarize some thoughts briefly: (1) is very explicit which is good for teaching but can be repetitive and verbose Since @sirosen replied while I was authoring this..
The |
Fascinating discussion. I am also ok for accepting any option, but I think the best way will be to use Just to make a discussion more concrete - (and provide context) we are eyeing into refactoring airflow monorepo for Airflow 3 (it's partially done already but we are also waiting for this one to land), and we will have 90+ sub-projects eventually in Airflow monorepo (yep, I know it's crazy). We are now recommending So - for multiple reasons we make sure in our docs and workflows that both In our case we currently have (this wil change and improve but here it is):
eventually it will be:
What I really would like with groups is:
Generally i'd be for not having In
|
Weakly in the sense of "yeah, I guess we might have to do that". It has problems, as you mention, though, so I'm definitely not enthusiastic about it 🙁
Yes. And stronger, it's an argument for requiring the
+1. Your arguments make sense to me. (But I'm not convinced about defaulting the
There are two aspects I don't like:
|
I'm a strong -1 on this. There are so many edge cases that don't have intuitive (or in some cases even plausible) interpretations that I don't even think this is the "user friendly" option. It's deceptively straightforward in simple cases, but will end up causing nasty bugs as soon as people do something unusual. I suggest that we simply drop this as a possibility, as it feels like people are only thinking about the "obvious" cases, and I have no appetite for coming up with multiple problematic cases just so that people can suggest workaround after workaround. (For example, if a requirements file includes |
In general, I am only interested in implementing behaviors which are easy to reason about and don't contain avoidable ambiguities. Even within that confined space, we are debating how to create a UX in which most naive users won't be easily confused.
I think this is a good point of concern. A user could do something like...
and expect... ? I'd like to have a solution in which the above usage, or its analogue, emits a warning or error, instructing the user that they're misapplying the options.
I find this convincing in principle, but I worry that it makes for a very verbose CLI experience for interactive usage.
If we go down this path, I have a question: And an initial expectation about requirements and behavior
The check for
This is still on my mind. I agree that it seems redundant, but it also is the most in-keeping with Does it make a difference that the proposed behavior would allow someone to pass something other than
Maybe that's a point against this, but I want to note it and understand if it elicits a strong positive or negative response from anyone. The behavior as implemented today doesn't have to worry about the filename. I was thinking Footnotes
|
I think this rules out (2) — I'm happy not to discuss that option further as suggested by @pfmoore but it's worth reiterating (as you have) that users will expect this and there should be warnings that guide them to the correct behavior.
The idea that this would be allowed is what is most concerning to me about including the filename, especially since it's such a clearly defined standard (a file named As a minor note regarding As I was looking at the CLI to write the above, I realized we recently added |
I know. That's the biggest drawback here. But I'd rather that we're verbose rather than confusing...
I'm against that - while this is important functionality in the wider ecosystem, it's not well aligned to pip's core feature set, and I don't think it deserves one of the limited single-character option names.
No, pip has a general feature that all command line options can be specified in a config file, or via an environment variable. So simply by having the command line option we automatically get the ability to set a default in those ways.
I'm not comfortable about it. But to be fair, the implementation currently allows an invalid |
There's a lot of design discussion going on in this PR that to me seems to boil down to:
If the answer to 1 for this PR is "no", I'd like to point out that it doesn't stop pip adding a concept of a project in the future. If And if the answer to 1 for this PR is "yes", then I'd like people to consider that the answer for 2 could affect a lot more than just this feature, for example if at some point pip reads it's own configuration out of pyproject.toml (i.e. #13003). So I would caution to consider this design quite carefully, looking at what other tools have done here and what challenges they've faced. I would be in favor of something as minimal and unopinionated about user workflows as possible, but I don't have a strong sense for what that is. |
I'm inclined to answer (1) as "no", at least within the scope of this PR. If a narrow change is not possible, I'd rather step back and make a more complete proposal which starts from the notion that
Ah, that explains why I didn't see the kind of env var logic I expected! I noticed the config loading Upcoming updateFor now, I plan to update the PR to implement I'll also validate that the filename provided is Idea:
|
That works for me. The |
I like it too. It also works nice as a building block of our |
I still think pip should accept any filename for the reasons I outlined in #12963 (comment). However given the path is explicit there is the obvious workaround of creating a directory and sticking a custom pyproject.toml there. And nothing stops removing this validation in the future if opinion on this changes. |
Side comment @notatallshaw - this problem in "messy monorepo" could be solved by converting the "custom named" toml files into putting pyproject.toml file for each team in a separate sub-folder of that directory where you currently have custom named files (directories named same way as currrent files). I think that is way better approach - especially that IDEs and such already have "if pyproject.toml" use the right schema etc. |
I personally don't like the "any filename" support, as the PEP specifically is for
I don't mind this, though. While it does allow someone to put a specific file in, it is optimized for the "correct" case following the PEPs. Also, I second that subdirectories with pyproject.toml's would be better, IDEs and validators would like that at least. |
Then we would have a clean monorepo 😛. Sure, that's the goal, but technical nuances and resource capacity have so far got in the way. For fresh projects we do this and can use opinionated tools like poetry, but for the older projects we stick to tools that don't force specific workflows or project structure, and pip has always been excellent in this regard. Anyway, I think I made my point in the linked post, and as long as there is an explicit path there is a workaround. I didn't mean to belabor it here. |
This, to me, is an important reason to include the requirements that the filename is If So the better short-term choice, from a compatibility perspective, is to validate the name. |
Why include the |
It can be |
Personally I think |
As I understand the building consensus, they have to be read from a e.g. Footnotes
|
I don't quite see the difference. To me (and from uv's perspective), a Python project is simply a directory with a I see the allure of requiring the path to the Regardless, I feel like pip needs to introduce the concept of project awareness to implement this feature. |
In my example I think the distinction is that
Then this conversation is regressing rather than moving forward, I posed that question directly in #13065 (comment) and it was answered "no" by the PR author in #13065 (comment) and other comments that followed seem to agree with this sentiment. I also agree with the PR author that if the answer is "yes" then the design should be addressed in a seperate issue, and not discussed in the review of this PR |
As @notatallshaw pointed out, this would be a step backward. I've said all this before, but I think that introducing a concept of project awareness to pip is a big change, not just to the implementation details but to the underlying design. And at this point, I'm not even sure it's the right thing to do - we have loads of project-based tools, what distinguishes pip is precisely the fact that it doesn't require you to follow that model. However, I think that being able to install dependency groups is a useful feature for pip, and it's something I absolutely expect users to request. So that leads me to the view that we need to find a way to express that without needing a project-based model. And saying "install this dependency group from this I'm more than comfortable with a minor wart in how we name the option (including |
I am as well. I haven't had the bandwidth to review the implementation though! |
Just FYI - we'd love to get this in for Aiflow. We are heavily modernising our packagins setup in preparation for Airlfow 3 (and we have a very complex setup with eventually ~ 100 projects in a single monorepo that we will manage) - and we have now setup wher we use Since PEP for that one is approved - we would love to entirely move to have - for example - our dev dependencies to be kept as dependency groups (rather than as we have it as dynamic editable extras), And lack of support for it in |
It's not a huge blocker, of course, more "nice to have" - but if there is anything we can do help with it, let us know please. |
@sirosen are you going to add that documentation discussed above? Assuming you do, I'm inclined to merge this - it's been around for long enough that if anyone had any major issues, they would have raised them. @sbidoul do you have any objections to this being in 25.0 if @sirosen gets the docs added? We can wait till 25.1 if the short notice is a concern. |
- Add a ``--group`` option which allows installation from PEP 735 Dependency | ||
Groups. ``--group`` accepts arguments of the form ``group`` or | ||
``path:group``, where the default path is ``pyproject.toml``, and installs | ||
the named Dependency Group from the provided ``pyproject.toml`` file. |
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.
+1
I'll release 25.0 this weekend. I'm fine getting this in, but perhaps should we then label it as experimental for one cycle, so we have some latitude to postpone possible bug fixes (if any) to the next release? |
Ah, that's a bit sooner than I realised. I suggest we defer until 25.1 in that case, both to avoid a last minute merge and to take any pressure off @sirosen. |
Steps taken: - add `dependency-groups==1.3.0` to vendor.txt - add dependency-groups to vendor __init__.py - run vendoring sync - examine results to confirm apparent correctness (rewritten tomli imports)
`--group` is supported on `download` and `install` commands. The option is parsed into the more verbose and explicit `dependency_groups` name on the parsed args. Both of these commands invoke the same processor for resolving dependency groups, which loads `pyproject.toml` and resolves the list of provided groups against the `[dependency-groups]` table. A small alteration is made to `pip wheel` to initialize `dependency_groups = []`, as this allows for some lower-level consistency in the handling of the commands.
A new unit test module is added for parsing dependency groups and used to verify all of the pip-defined behaviors for handling dependency-groups. In one path, the underlying exception message from `dependency-groups` is exposed to users, where it should offer some explanation of why parsing failed, and this is therefore tested. Some related changes are applied to the dependency groups usage sites in the src tree. The signature of the dependency group requirement parse function is simplified, and its usage is therefore updated. A bugfix is applied to add a missing `f` on an intended f-string.
This initial suite of tests is modeled fairly closely on existing tests for requirements files. Tests cover the following cases: - installing an empty dependency group (and nothing else) - installing from a simple / straightforward group - installing from multiple groups in a single command - normalizing names from the CLI and pyproject.toml to match - applying a constraints file to a dependency-group install
Per review, support on `pip wheel` is desirable. This is net-net simpler, since we don't need any trickery to "dodge" the fact that it is a `RequirementCommand` but wasn't supporting `--group`. The desire to *not* support `--group` here was based on a mistaken idea about what `pip wheel` does.
In discussions about the correct interface for `pip` to use [dependency-groups], no strong consensus arose. However, the option with the most support appears to be to make it possible to pass a file path plus a group name. This change converts the `--group` option to take colon-separated path:groupname pairs, with the path part optional. The CLI parsing code is responsible for handling the syntax and for filling in a default path of `"pyproject.toml"`. If a path is provided, it must have a basename of `pyproject.toml`. Failing to meet this constraint is an error at arg parsing time. The `dependency_groups` usage is updated to create a DependencyGroupResolver per `pyproject.toml` file provided. This ensures that we only parse each file once, and we keep the results of previous resolutions when resolving multiple dependency groups from the same file. (Technically, the implementation is a resolver per path, which is subtly different from per-file, in that it doesn't account for symlinks, hardlinks, etc.)
Co-authored-by: Paul Moore <[email protected]>
The new section comes after `requirements.txt` and `constraints.txt` documentation but before documentation about wheels. The doc attempts to be beginner-friendly and balance - clarity about the path behavior - explanation of `[dependency-groups]` itself - justification of the path-oriented design -- in the form of an example of installing from two different subprojects simultaneously There is therefore ample material not covered in the new section -- e.g., there is no mention of `include-group`, which is explained at length in the specification doc.
a7dcec9
to
e602bfc
Compare
I've rebased and added an initial pass at a doc (this commit for the diff). |
LGTM! |
- use `test-groups` instead of `test-requires` for cibuildwheel - temporary workaround to install dev dependency-group using uvx until pip adds support - can later be removed and instead adding `--group dev` or similar to the pip install command (pypa/pip#13065) - use build.verbose instead of deprecated cmake.verbose - remove no longer used requirements-dev.txt
Update: This PR has undergone a major revision. See this comment for the revised proposal.
The initial comment here is preserved, as it contains useful context and is necessary to understand the following discussion.
This changeset implements
--group
as an option for loading dependency groups, leveraging thedependency-groups
library1.resolves #12963
Example usage
pyproject.toml loading and working dir
Per the discussion in #12963 , this is implemented with support only for loading data out of the
pyproject.toml
file found in the working directory.Although in theory there might be a desire to have any of the following interfaces in the future...
...there is not a clear "winner" amongst these and they are only usable with
--group
.Until or unless there is a more strongly demonstrated need to specify the path in some way, no attempt is made here to support it.2
--group
as the Option Name--group
is known to match the option provided byuv
for installation from groups.There are some lines of argument in favor (e.g., the initial post which mentions this), and some arguments against (e.g., my note that identical option names could incorrectly imply identical behavior).
Ultimately, I chose to propose
--group
because of a UV-independent argument that it is short and declarative.I am open to arguments from
pip
maintainers in favor of other names -- but I think--group
is a good name and that "potential confusion withuv
" is a bad reason not to choose it if it's the best name for the feature.Any alternative name should have some reason that it would also be good for users. e.g.,
--dependency-group GROUP1
could work because it's very explicit.--group
as a repeated option vs comma-delimited--groups
I originally proposed an interface along the lines of
--groups foo,bar,baz
in #12963.I'll just make a quick note that as soon as I started implementing this, I could not see any particularly good reason to add specialized parsing.
Option parsing can collect these for us trivially, and it removes the potential for ambiguity in cases like
--groups foo,bar --groups foo,baz
.Wrapped
dependency-groups
libUse of
dependency-groups
passes through a wrapper module which conforms the errors from that library toInstallationError
.Details of the implementation do (intentionally) leak out, most notably the error strings.
This can be revised if there's a desire for
pip
to control the error messages more tightly.Tests
It was not clear to me exactly how much effort I should invest in unit and functional tests.
I therefore generally tried to follow the guide of "test the subset of features which map to existing tests of
requirements.txt
files".New unit tests and functional tests explore some particular cases, but they are intentionally a little bit limited.
If desirable, we can retest most of the behaviors provided by
dependency_groups.resolve
under the unit tests.Commits & Review
Here's a summary of the (at time of writing) commit series:
--group
--group
supportNotably, the first commit contains the whole vendoring step with no usage. So it should be possible to diff the HEAD of the PR against that commit to see the "real work" here.
Footnotes
As a potentially important aside, I intend to open a thread to move
dependency-groups
intogithub.com/pypa/
for better continuity of ownership and maintenance. ↩Some users will likely be disappointed with this decision. But the nice thing about not deciding anything today is that it can easily be added if a strong argument is made. "No is temporary; yes is forever." ↩