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

Requirements with (potentially conflicting) options #7846

Open
pfmoore opened this issue Mar 11, 2020 · 8 comments
Open

Requirements with (potentially conflicting) options #7846

pfmoore opened this issue Mar 11, 2020 · 8 comments
Labels
state: needs discussion This needs some more discussion type: enhancement Improvements to functionality type: feature request Request for a new feature

Comments

@pfmoore
Copy link
Member

pfmoore commented Mar 11, 2020

This is a scenario which may never happen in practice, but for which we need to decide what a reasonable behaviour would look like. The current resolver probably does "something", but likely by accident of implementation. For the new resolver, though, this question may directly impact the implementation strategy.

Consider if the user has two requirements file something like this:

r1.txt:

foo --install-options option1

r2.txt

foo --install-options option2

and then suppose the user does an install as follows:

$ pip install -r r1.txt -r r2.txt

Basically, we now have two InstallRequirement objects, both for foo, but with different install options. Which one gets installed? How do we choose?

The current resolver appears (from my experiments) to pick the first one (option1). But the new resolver would end up creating a set of Candidate objects for foo - and which set of options should it choose to apply when preparing/building those candidates?

The problem here is very much that the new resolver will do find_matches against both requirements, and get 2 lists of Candidates. We need to merge those lists, as they will basically contain the same candidates. But if we merge, we have to decide which options to keep and which to discard.

I'm inclined to do as the current resolver does and just take the first set of options that the code encounters, but say that this is explicitly "not supported" in the sense that we don't guarantee any particular behaviour. I doubt it will cause anyone any issues in practice.

Ideally we could detect and warn/error on conflicts, but I don't know if that's practical (I chose install options as an easy to describe case, but really any configuration held in an InstallRequirement would have the same issue (markers, the isolated flag, hash options, the wheel cache...).

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Mar 11, 2020
@pfmoore
Copy link
Member Author

pfmoore commented Mar 11, 2020

Ping @pradyunsg @uranusjr for opinions.

For now, I'll implement "first one wins", so we're not blocked by this (obscure) problem.

@uranusjr
Copy link
Member

I’m inclined to say “error out” but agree anything would work for now. This is probably an edge case that no-one would ever encounter.

@pfmoore
Copy link
Member Author

pfmoore commented Mar 11, 2020

Error (or warn) is better, I suspect, but even checking all the data might be hard. But checking what I can and reporting that, but not worrying about trying to be perfect sounds reasonable.

As I say, I don't want to block on over-thinking this :-)

@pradyunsg
Copy link
Member

Yea, I'm very much in favor of erroring out in cases where we're not sure of a "consistent" approach to things. If users hit these issues in practice, that can help inform us about the escape hatch they'd need.

Given that we're going to do a hopefully-decent beta run, I suggest we try to be as strict as possible in our initial implementation and relax things based on user feedback.

@xavfernandez
Copy link
Member

I'm also in favor of erroring out in this case (or at least warn loudly).

@McSinyx
Copy link
Contributor

McSinyx commented Mar 24, 2020

How about giving users options to choose from, like aptitude (at the end of the manual)? We'd probably need an option to opt-out of this of automation (e.g. CIs) though,

@uranusjr
Copy link
Member

The prompt would be nicer to have, but IMO it is better to error out than prompt the user if the conflicting options come from a requirements file (the user should fix it), so that would be my choice to implement first.

@pradyunsg pradyunsg added state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality labels May 25, 2020
@triage-new-issues triage-new-issues bot removed S: needs triage Issues/PRs that need to be triaged labels May 25, 2020
@pradyunsg pradyunsg added state: needs discussion This needs some more discussion type: feature request Request for a new feature and removed state: awaiting PR Feature discussed, PR is needed labels May 28, 2020
@brainwane
Copy link
Contributor

Legacy pip has inconsistent behavior and the new resolver will act differently; we're moving this to fix post-release, and as we get more user feedback we'll know more about what to provide errors/notifications about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: needs discussion This needs some more discussion type: enhancement Improvements to functionality type: feature request Request for a new feature
Projects
None yet
Development

No branches or pull requests

6 participants