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 overriding constraints with '-o' in constraints file. #11537

Closed
wants to merge 1 commit into from
Closed

Allow overriding constraints with '-o' in constraints file. #11537

wants to merge 1 commit into from

Conversation

mauritsvanrees
Copy link
Sponsor Contributor

@mauritsvanrees mauritsvanrees commented Oct 20, 2022

This is one approach of solving issue #8076, you know, the one with over one hundred comments. I had basically the same idea as @PiotrDabkowski expressed in one of the most recent comments: extend the syntax of what you can say in a constraints file.

Let's show what it can do with a simple package without any dependencies.

$ cat constraints.txt
plone.intelligenttext == 3.1.0
-o plone.intelligenttext == 3.0.0
$ bin/pip install plone.intelligenttext -c constraints.txt 
...
Successfully installed plone.intelligenttext-3.0.0

So I requested version 3.1.0, then overrode this with version 3.0.0, and the override was installed.

The package is used in Plone, or at least included in the Plone constraints file. Installing an editable version of a package that is pinned in a constraints file, is not possible, see issue #8307. But with this PR we can override it without a version pin:

$ cat constraints.txt
# The next file contains plone.intelligenttext==3.1.0
-c https://dist.plone.org/release/6.0.0b3/constraints.txt
-o plone.intelligenttext
$ bin/pip install -e git+https://github.com/plone/plone.intelligenttext.git#egg=plone.intelligenttext -c constraints.txt 
...
Successfully installed plone.intelligenttext-3.1.1.dev0

The implementation is rather easy: a few lines of (perhaps too manual) parsing of a line starting with -o or --override, adding is_overrideto theInstallRequirementclass and several functions, and then checkingis_override` when getting the constraints of the root requirements.

Obviously this needs tests and documentation, which I won't (or shouldn't) have time for soon. But what do you think: would this solve most problems, without giving people a too powerful gun to shoot in their own foot?

@pradyunsg
Copy link
Member

This is one approach of solving issue #8067, you know, the one with over one hundred comments.

Did you get the issue number wrong?

@pfmoore
Copy link
Member

pfmoore commented Oct 20, 2022

It should have been #8076.

I'm baffled by the explanation here, though, @mauritsvanrees. Your example constraints file:

plone.intelligenttext == 3.1.0
-o plone.intelligenttext == 3.0.0

makes no sense to me. Why wouldn't you just replace it with one that says

plone.intelligenttext == 3.0.0

? Maybe I need to go back and read the extensive debate on the original thread, but I frankly don't have the time or inclination to do so right now. And in any case, this PR will need to include documentation, and that documentation will need to explain what the feature is for, what issues it will address, and how it will work, without expecting the reader to know all of the history on that issue. So why not give a standalone explanation here, so that people can evaluate the PR on its own merits?

@pfmoore
Copy link
Member

pfmoore commented Oct 20, 2022

Having now re-read the thread, I think I prefer @notatallshaw's proposal of a completely separate "overrides" file rather than trying to include this in the existing constraints file. I appreciate that a proposal with a PR has a significant advantage over a proposal that no-one has bothered to implement, regardless of any other factors, but I understood the design of the override file proposal pretty much straight away, whereas I find the "override constraint" approach a lot more confusing.

I guess you could argue that the two are equivalent, as you can take all the -o REQ lines out of a constraints file and put them in an overrides file omitting the -o and vice versa. But the overrides file approach suggests that you'd have either overrides or constraints, whereas the -o proposal feels like it's natural to have both. And I think the interaction between constraints and overrides would be confusing at best. So I prefer to keep the ideas separate.

I'm also not convinced that the implementation here would work. At a minimum, please add a test that demonstrates that overriding works as required.

@notatallshaw
Copy link
Member

I'm a bit confused by reading the code of this implementation, does it actually override transitive dependency requirements?

@mauritsvanrees
Copy link
Sponsor Contributor Author

@pfmoore Sorry, you are right, this PR assumes too much knowledge on the original issue.

About my strange example:

plone.intelligenttext == 3.1.0
-o plone.intelligenttext == 3.0.0

This was meant as an easy test to show that the PR works: you have a constraint and you can override it. In practice it would be used to override an upstream constraints file:

# The next file contains plone.intelligenttext==3.1.0
-c https://dist.plone.org/release/6.0.0b3/constraints.txt
-o plone.intelligenttext == 3.0.0

Let me show some real world examples of what I want to enable.

Example: Plone 5.2 with two newer packages

Plone 5.2 works on both Python 2.7 and Python 3.8. Here we use 3.8.
We focus on two packages, for which the constraints file has this:

plone.namedfile==5.6.0
plone.scale==3.1.2

Both packages have beta versions that only work on Python 3. I want to use them. So I create this constraints file:

-c https://dist.plone.org/release/5.2.9/constraints.txt
plone.namedfile == 6.0.0b5
plone.scale == 4.0.0b5

Create a virtualenv using a pip clone, on main for now, and try to use the above constraints file to install the smallest of the two packages:

$ python3.8 -mvenv .
$ bin/pip install -e <path to existing pip clone>
$ bin/pip install plone.scale -c constraints.txt
ERROR: Cannot install plone.scale because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested plone.scale
    The user requested (constraint) plone-scale==3.1.2,==4.0.0b5

Now try the same with my pip branch and using the new overrides spelling:

$ cat constraints.txt
-c https://dist.plone.org/release/5.2.9/constraints.txt
-o plone.namedfile == 6.0.0b5
-o plone.scale == 4.0.0b5
$ bin/pip install plone.scale -c constraints.txt
...
Successfully installed ... plone.scale-4.0.0b5 ...
$ bin/pip check
No broken requirements found.

Let's try the second package for good measure, which pulls in lots more packages:

$ bin/pip install plone.namedfile -c constraints.txt
...
Successfully installed ... plone.namedfile-6.0.0b5 ...
$ bin/pip check
No broken requirements found.

Example: editable package from constraints

Let's stick with the same packages. Now I want to use a checkout of plone.scale. Try with current pip:

$ cat constraints.txt
-c https://dist.plone.org/release/5.2.9/constraints.txt
$ bin/pip install -e git+https://github.com/plone/plone.scale.git#egg=plone.scale -c constraints.txt
...
ERROR: Cannot install plone-scale 4.0.0b6.dev0 (from git+https://github.com/plone/plone.scale.git#egg=plone.scale) because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested plone-scale 4.0.0b6.dev0 (from git+https://github.com/plone/plone.scale.git#egg=plone.scale)
    The user requested (constraint) plone-scale==3.1.2

Now with my branch:

$ cat constraints.txt
-c https://dist.plone.org/release/5.2.9/constraints.txt
-o plone.scale
$ bin/pip install -e git+https://github.com/plone/plone.scale.git#egg=plone.scale -c constraints.txt
...
Successfully installed plone.scale-4.0.0b6.dev0
$ bin/pip check
No broken requirements found.

I actually need this to work when running tests for these two packages on gh-actions or locally. Currently I am using an extra tool that was created this year by someone in the Plone community to solve this use case: mxdev. I won't go into details, as that is out of scope for this PR, but just the general idea (which maybe has too much overlap with parts of pip-tools):

  • tox calls these commands.
  • mxdev is called there as pre-processor to create constraints and requirement files
  • mxdev uses this ini file as input. There we specify that we want to ignore two package pins, and use a checkout.
  • The resulting constraints and requirements files are then called with a standard pip install.

It would be nice if we don't need to rely on such a tool created in one community, as it makes it harder for new developers to start working on a package.

@notatallshaw writes:

I'm a bit confused by reading the code of this implementation, does it actually override transitive dependency requirements?

That is not my intention, but I think it currently does, so I may need to change this. My intention is to honour any minimum or maximum version requirements in any setup.py/cfg file. So if I install plone.intelligenttext plus package B that requires plone.intelligenttext >= 3.0.5, then I am happy if the above override fails. Currently it won't, so I will want to do something about that first.

Some people in the original issue do want to be able to override a version even if pip check would fail. That could be an extra command line option like --allow-overrides-to-lead-to-a-setup-with-incompatible-dependencies, or some nicer name. That could be a different PR on top of this one if someone wants to implement it.

@pfmoore
Copy link
Member

pfmoore commented Oct 21, 2022

Some people in the original issue do want to be able to override a version even if pip check would fail. That could be an extra command line option like --allow-overrides-to-lead-to-a-setup-with-incompatible-dependencies, or some nicer name. That could be a different PR on top of this one if someone wants to implement it.

Ah, that's what I assumed you were trying to do, as that's basically what #8076 is about.

For your use case, you should just edit the existing constraints file. I don't think we need a new option just for that, sorry.

@mauritsvanrees
Copy link
Sponsor Contributor Author

@pfmoore wrote:

Having now re-read the thread, I think I prefer @notatallshaw's proposal of a completely separate "overrides" file rather than trying to include this in the existing constraints file.

There is a warning by @uranusjr that I would summarise as: the parsing of requirements and constraints files is already difficult; if we would add a third type of file the difficulty would even increase.

But then, me adding -o as option in a constraints file also makes it more difficult...

I could have a go at an overrides file. One of my examples above would then become this:

$ cat constraints.txt
-c https://dist.plone.org/release/5.2.9/constraints.txt
-o overrides.txt
$ cat overrides.txt
plone.namedfile == 6.0.0b5
plone.scale == 4.0.0b5

The allowed format within overrides.txt would be exactly the same as in constraints.txt.

Probably -o should then also be allowed in requirements.txt or on the command line.

@mauritsvanrees
Copy link
Sponsor Contributor Author

For your use case, you should just edit the existing constraints file. I don't think we need a new option just for that, sorry.

The existing constraints file is on an upstream server. So I would need to download it, manually edit it, and store it in git. And do this in every project where I need something similar.

Then upstream releases a new version of the constraints file. I need to download it, compare it with the previous original, manually edit it, and store it in git again.

Or have a script that downloads it, and use for example sed to replace some contents, hopefully in a way that does not break every few months.

And then another developer looks at the project where I did this, and instead of a clear constraints url with two updated pins, she sees a local constraints file of 500 lines (yes, Plone is big), which is probably Plone 5.2.9, but zero, one, or more pins may have been updated, and it is not immediately clear which ones they are.

That is not really an option for me.

@mauritsvanrees
Copy link
Sponsor Contributor Author

Let me put this differently. Regardless of the implementation, I see two options:

  1. Let an override really override everything, including explicit version bounds in setup.py.
  2. Let an override purely override constraints from a constraints file, and let pip still fail when this conflicts with a setup.py.

I was aiming for number 2, but currently have number 1, which is easier.

@pfmoore You prefer number 1 here? I could work with that.

@pfmoore
Copy link
Member

pfmoore commented Oct 21, 2022

I personally have no use for any of this, but my understanding of #8076 is that it is solely about telling the resolver to use a specific version of a package, ignoring all other dependency specifications and accepting that the resulting environment will probably fail pip check.

Being able to override values from a constraints file is, as far as I can tell, an almost entirely accidental side-effect of that requirement.

For this PR to be acceptable, I'd want some sort of consensus from the participants in that other issue that it addresses their needs.

@notatallshaw
Copy link
Member

notatallshaw commented Oct 21, 2022

Yes, I agree with @pfmoore here, my understanding of (and definitely my proposal in) #8076 is to give some level of control to the user to outright replace transitive dependency requirements with something other than how they are specified by the downstream projects.

My proposal is that just as the primary purpose of a constraints file is to constraint transitive dependencies that an overrides file's primary purpose would be to override transitive dependencies.

In my view this gives the user a lot of control that is not easy to replicate with simple scripts.

It seems to me that purpose of this PR is that an upstream project has a constraints file but you would like to modify it or merge it with your own constraints. While this seems totally valid to me it also seems not too difficult to implement outside Pip. If I had that problem I would be tempted to make a package that could download multiple constraints files and merge them based on the order which I specified them. But maybe there's something I'm missing here.

@pradyunsg
Copy link
Member

pradyunsg commented Oct 21, 2022

FWIW, if we do go down the route of a overrides file, IMO it shouldn't be a part of the options that requirements or constraints file can take within them, but a separate file entirely.

It's reasonable to limit it to only allow package==version, syntax (I don't mind the format not accommodating for comments even, in the first cut).

@PiotrDabkowski
Copy link

I am against creating another file. Constraint is a constraint - it must be satisfied no matter what, as the name suggests. Having 2 terms, and 2 files: override and constraint is confusing.

The important thing is what pip should do if the constraint is impossible to satisfy - currently it returns an error. I think there should be also an option to enforce the specified constraints, ignoring everything else.

@notatallshaw
Copy link
Member

notatallshaw commented Oct 21, 2022

I think someone should make a new issue for what exactly is being requested here and what problems it solves so the discussion can be focused on if this is a good proposal or not.

The merits of the problem that this PR is solving and what #8076 is asking for are very different. E.g.

I am against creating another file. Constraint is a constraint - it must be satisfied no matter what, as the name suggests. Having 2 terms, and 2 files: override and constraint is confusing.

But in #8076 overrides are not constraints, they are a way to override transitive dependency requirements, not constrain them, they therefore have very different behavior in practice. And I agree with @pradyunsg should be implemented in a very limited fashion and only expanded if legitimate requirements come up for it.

This PR though is intended to just deal with constraints, hence the confusion.

@pfmoore
Copy link
Member

pfmoore commented Oct 21, 2022

This PR though is intended to just deal with constraints, hence the confusion.

IMO, if this PR isn't intended to address #8076 then I'm -1 on it.

@mauritsvanrees
Copy link
Sponsor Contributor Author

Quoting @pfmoore who quotes @notatallshaw:

This PR though is intended to just deal with constraints, hence the confusion.

IMO, if this PR isn't intended to address #8076 then I'm -1 on it.

Even though it was not my intention, this PR does currently address it. So I think it could still be considered. But these new possibilities make this a tool that is a bit too sharp for my taste, so I will stop my efforts here. If someone else wants to pick this up, go ahead. I'll close this PR but keep the fork and branch for now. I think the basis works, but the next items are needed to take this to completion:

  • Confirmation from others in the issue that this indeed fixes their problem. I will add a comment there pointing to this PR.
  • A decision on the format: extend the constraints syntax with -o like in this PR, or use a separate overrides file with strictly limited formatting. Looks like the discussion is leaning towards the last.
  • tests
  • documentation

For my own use case, I was hoping to be able to only override constraints, and not override requirements from setup.py/cfg as well. This PR would still be a good base for that, with a bit more effort. But given some remarks above, it seems unlikely that this would be accepted. Quoting a comment by @pfmoore on the issue:

I'd love to see an ecosystem of tools like this that work with pip, removing the endless pressure to cram everything into pip itself.

I was trying to avoid needing a separate tool for this. But that goes against your comment, which I can understand. So I will use mxdev then, as I explained in a comment above.

@stonebig
Copy link
Contributor

stonebig commented Nov 1, 2022

mxdev looks nice, but if it' doesn't enter pip, there is no sustained future on the idea....
maybe an alternative way would be package providers to be able to specify "advised" constraints instead of "mandatory" constraints ?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants