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

Allow ignoring sub-dependencies #12790

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

Conversation

leorochael
Copy link

@leorochael leorochael commented Jun 24, 2024

Implement --no-deps-for=pkg to allow ignoring sub-dependencies of specific packages, as opposed to the global --no-deps flag.

The flag is accepted on the command line and in requirements files.

Implemented in both the new and the legacy resolvers.

Also, implemented acceptance of the global --no-deps on the requirements file.

Fixes: #9948

@pfmoore
Copy link
Member

pfmoore commented Jun 24, 2024

-1 on implementing this for the leagcy resolver. We should not be making any changes to that code - it's destined for removal.

Apart from that comment, I've not looked at this PR yet so I won't say anything more right now.

@leorochael
Copy link
Author

@pfmoore, I only implemented it on the legacy resolver because it was a one-liner (other than formatting).

It took a lot longer for me to add tests to the legacy resolver to check those changes than the change itself.

Adding a check on the legacy resolver to fail to work with the new option (as has been done to other changes) would have taken the same complexity.

But I'll gladly remove the change to the legacy resolver if that is the consensus. I'll await further comments on this PR before doing that.

@leorochael leorochael force-pushed the leorochael-no-deps-for branch 3 times, most recently from 712ed55 to 30aef34 Compare June 25, 2024 10:39
@leorochael leorochael marked this pull request as draft June 25, 2024 12:43
Implement `--no-deps-for=pkg` to allow ignoring sub-dependencies of
specific packages, as opposed to the global `--no-deps` flag.

The flag is accepted on the command line and in requirements files.

Implemented in both the new and the legacy resolvers.
@leorochael leorochael marked this pull request as ready for review June 25, 2024 14:25
@leorochael
Copy link
Author

I've fixed the remaining CI errors.

I'd appreciate a review and indication of next steps.

@pfmoore
Copy link
Member

pfmoore commented Jun 25, 2024

Some general comments, intended as a review but they seem to fit better here than in the review screen.

  • I still want the changes to the legacy resolver (and the associated tests) removed. You want to wait for comments from other maintainers, which is fine.
  • There are a number of edge cases that need to be defined, documented and tested. For example, if A and B both depend on C, what should pip install --no-deps-for=A A B do? Should it install C (respecting the dependencies of B) or not (respecting --no-deps-for but breaking B)?
  • Another example - if A depends on C 1.0, and B depends on C 2.0, what should pip install --no-deps-for=A A B do? Install C 2.0 (respecting B's dependencies but breaking A) or not (sort of respecting --no-deps-for, but not really because A has no dependency on C 2.0)?
  • A third example - A 1.0 does not depend on C, A 2.0 depends on C 1.0. B depends on C 2.0. What should pip install --no-deps-for=A A B do? Install A 1.0, B and C 2.0 (a perfectly safe resolve regardless of --no-deps-for)? Install A 2.0, B and C 2.0 (respects --no-deps-for but breaks A)?

To be honest, I think this PR might be premature, as the behaviour isn't fully defined yet. I'd be very uncomfortable if we simply said that the answers to the above questions was "whatever this PR does, or whatever is simplest to add to this PR".

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

Successfully merging this pull request may close these issues.

--no-deps flag inside requirements.txt
2 participants