-
Notifications
You must be signed in to change notification settings - Fork 3k
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
New resolver: “Nameless” constraint #8210
Comments
One question - do we have any indication that anyone actually uses this functionality? It looks incredibly weird, and I don't see what the purpose of such a constraint is. (I can technically work it out, but why would anyone use it?) I'd be reasonably comfortable taking the approach of (at least initially) dropping support for constructs like this in the new resolver, and seeing if anyone complains. It would affect the rollout process and how we handle backward compatibility, though, because if someone does pop up saying they use this, we'd need a workaround for them while we work out how to add it. It also changes the project goal, as "all tests must pass" won't be quite right in that case. @pradyunsg @brainwane Any thoughts? Is there any value in asking the user survey population "do you use constraints and if so how?" Or is that too small a sample to be worth it? PS I'm not going to review the rest of the constraint-related issues until after the weekend, but I wanted to add my thoughts to this one to give people a chance to respond. |
I vaguely remember being taught about mentioned constraints as a workaround to pip’s inability to merge extras (the feature described in #8211), but I assume many such usages are not needed anymore since the new resolver does not have the problem. As for direct URL in constraints, no idea at all. This is definitely a good case to delibreately not include the feature because YAGNI and see what happens IMO. |
100% on board for not implementing this, flagging this as a "this no longer works with the new resolver" line item somewhere, and seeing if anyone comes complaining about this during the beta. :) |
Constraints were not a work-around for merging extras. They were added by Openstack because they want to make sure that the entirety of openstack is installable together, without having to either maintain 20 separate See #2731 for a tiny bit more information. We would definitely want to reach out to the Openstack community to see if they're still using it. |
Actually I think that the feature described in #8211 is wrong? At least the inteded purpose of constraint files were to not cause anything extra to be installed, but to only apply additional constraints to whatever would ultimately get installed. |
It is my understanding as well. Sorry if my writing is causing misunderstandings, please feel free to add to/modify the issue. |
Openstack are actively testing the constraints implementation in the new resolver (@dhellmann contacted us to help out) so based on your comment, I'd suggest that we wait for feedback from them on whether their testing showed up any issues, and review these issues in the light of that feedback). I wonder if these tests were mistakenly added from a "completeness" point of view ("because you can add any requirement in a constraints file, we need to decide what |
As far as I know, OpenStack always uses package names, one rule with == or === and a version per constraint entry, and optionally an expression indicating which version of python needs the constraint (I can never remember the name of that feature, sorry). This allows the test jobs to "pin" to the same versions of a package across tests that have different combinations of applications installed. The global constraints file for OpenStack is at https://opendev.org/openstack/requirements/src/branch/master/upper-constraints.txt if you want to see an example. Most projects also have a per-project constraints file using the lower bounds for their requirements, used for a separate unit test job and possibly an application-specific functional test job. We do not run the integration tests with the lower bounds constraints. For an example of one of these files, have a look at https://opendev.org/openstack/nova/src/branch/master/lower-constraints.txt There are going to be a lot of those, since every repo will have its own. I have asked specifically about nameless constraints on the mailing list http://lists.openstack.org/pipermail/openstack-discuss/2020-May/014790.html /cc @prometheanfire |
we currently do not allow nameless constraints or requirements. As I understand it, those are git/http based uris? we set |
@dhellmann @prometheanfire Could you aldo help verify whether there are currently projects depending on the extras behaviour, as described in #8211? |
I'm a bit unsure about the precise meaning of 'nameless' and if this is coupled to the discussion around extras. However I will outline the way openstack-ansible uses constraints files to install a mixture of packages and source code just to make sure that this is captured. Example is here http://paste.openstack.org/show/793386/ but short story is we use constraints like "git+https://opendev.org/openstack/glance@6e3ced8251cd6e273aa73f553a24fc475b219db5#egg=glance" and put URLs to remote constraints in constraints files all the time. |
@jrosser So I'm not at all clear what such a constraint (a URL) would mean in practice. Constraints as I understand them, constrain a project by requiring a certain version or equivalent - so would your quoted example be saying that glance must be installed from precisely that URL - and hence that |
The constraint as we use it says "install this package, but take the source code from the URL at the SHA or branch specified". Here is an example I just did which installs ansible from our fork which has a bugfix applied:-
So long as this all keeps working with the new resolver, it's all OK. |
Ah, that's not how constraints in the new resolver work right now (and it's not how I read the docs). Looks like there's some extra complexity here we need to look into :-( |
@jrosser I'm curious about why that is being done via the constraints list rather than a regular requirement specification. Are there cases where some of the requirements depend on each other? @pfmoore IIUC, the current implementation of constraints is either limited to exact version matches or the constraint entry overrides all other version rules. The effect is when we list foo==version in a constraint list, we get exactly version of foo, even if that version would have conflicted with the requirements of a package that depends on foo version. That's probably not ideal behavior, and maybe the resolver should consider all of the version specifiers and fail if there is a conflict. |
@dhellmann The new resolver certainly won't do that (by design) - the current implementation allows There's no provision in the current implementation for the new resolver for any form of constraint that isn't a spec (i.e., of the form above). Adding the "named URL" form that @jrosser described would be very different, as it's introducing a source for the package that otherwise would not have been available, and disabling all other sources. Even if we do implement this (it's not necessarily going to be straightforward to do so) we still wouldn't install it if the result would conflict with other requirements/dependencies. A key feature of the new resolver is that it will only ever result in a set of candidates to be installed that satisfy the supplied requirements (and their dependencies)1. 1 There's still a possible situation where we could get an inconsistency with other packages - see #7744 for more detail - but that's not relevant here. |
@dhellmann looks like the same installation of ansible can be achieved through a requirements spec. This isn't a good enough example to prove that we can do a mixture of downloaded packages, specific things from source and have openstack upper-contraints apply across the whole lot.
I'd have to defer to Jesse P who developed a lot of this for openstack-ansible to know if there was a reason the source was specified in constraints rather than requirements. |
Yea, I agree that name @ URL is not a constraint, but a requirement. If the given URL does not work, well, the resolution should fail. That's exactly what making it a requirement would do. Passing this as a constraint isn't correct and "worked" because of a fun bunch of technical debt in pip quirks (overtly optimistic resolver, bad abstractions etc). Part of the problem here is with how constraints got implemented. They're represented with the same object within pip as requirements, and have no distinction other than a single boolean. That was fine then because requirements could only be name-op-version or name. However, this meant that we accidentally started allowing URLs in constraints when we added URLs to requirements! That's a bug we didn't know about until now! Now, that stuff still worked because the older dependency resolver "upgraded" constraints into a thing to install (by flipping the boolean and setting another attribute), which is fine in the optimistic world view of that resolver (which also includes I won't ever see dependency conflicts, so, it wasn't exactly sound). The new resolver doesn't work like that (as @pfmoore noted above) and that sort of behaviour is really tricky to reproduce in a model where "limits on what can be installed" and "thing to install" are (correctly) represented by different objects. I'm strongly opposed to bringing this bug over to the new resolver, since the usecase is already covered by regular requirements already as far as I can tell from the descriptions above. |
I think I agree with not carrying the URL handling behavior over. I definitely agree that constraints should not be installed unless they are listed as a requirement. |
Before going all the way, I would like to understand if there is some other way to express that if any requirement introduces a need for a package, it should be installed from source instead of from a dist. For example, if I am installing two applications that depend on a library and I want to install the library from source as part of a CI job, how do I do that? |
--no-binary common-library |
I understand this thread has drifted from nameless constraints to considering the removal of constraints as URL. If this is the case, please read on, otherwise apologies for the noise. I think constraints are important to have (compared to requirements) when you consider top level requirements and The typical use case for URL constraints is to install a patched version of a dependency, while not making the dependency a top level requirement. Imagine you have a project with such a from setuptools import setup
setup(
name="test",
install_requires=["packaging"],
) And this
[I'm using a tag here as an example, a real life use case would be a PR or a bug fix branch. If you The same can be achieved with Similarly if you remove IMO this use case for constraints is important and should be preserved. It will become even more important when we implement a better uninstall that considers user requested top level requirements. |
I have enough of a problem with the idea of Implementing URLs as constraints would be tricky - it really doesn't fit well with how the new resolver handles constraints. I don't think it would be impossible, but I'd like to see some solid examples where people are using the existing functionality, and can't achieve the same result any other way, before going down that route. I'm basically in agreement with @pradyunsg here, The documentation says "Constraints files are requirements files that only control which version of a requirement is installed" - that implies to me that the fact that URLs work is an accident of the implementation. That documentation was written in the same commit (bb0b429, addressing #2731) that constraints were implemented, so I think it's acceptable to take it as definitive. It's also worth noting that Having done that digging, I'm now certain that the intended use for constraints was only ever to supply version restrictions, and that's what we should limit the new implementation to. |
The URL use of constraints feels more like a feature request for some kind of resolver override. Like a "I don't care what the resolver says, I want this version" override feature, whereas constraints are more like "please feed this additional information into the resolver". |
A direct URL is a way to specify a version, even if it requires generating metadata before being usable in the resolver. I don't know why nor how that feature came into existence but it is coherent and useful. Even if you ignore REQUESTED, I for one use that regularly in applications: Constraints are basic primitives for dependency management and there is no reason they should not support direct URLs. A direct URL is just another way than |
This specifying constraints with URL feels a bit like how people use
as a succinct (enough?) way to replace |
The thing is constraint files don’t replace what the resolver thinks should be installed. They just further constrain what is acceptable. If that URL isn’t already in the set of acceptable versions for a project then it should result in an unresolvable dependency graph.
…Sent from my iPhone
On May 13, 2020, at 6:32 PM, Tzu-ping Chung ***@***.***> wrote:
This specifying constraints with URL feels a bit like how people use dependency_links, but is actually in a way we want it to be (i.e. specified at the install site), without needing to add awkward requirements, which dependency_links advocates have been complained about. I can see we suggest something like
pip install my-package -c my-package-constraints.txt
as a succinct (enough?) way to replace dependency_links.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
And I feel that would be what people expect, even those who want to use |
So... folks arguing that constraints should allow URLs, I'm curious: constraints.txt: What should be the result of this? From what I can tell, the options are:
|
Yes, that, without doubt for me. Some more cases to illustrate:
With |
OK, but the fundamental difference between the I'm happy to accept that there might be a need for a feature that says "override the finder to only return the following candidate(s) for this project" - but I don't think it should be part of the constraint mechanism, regardless of what historical accidents of implementation exist in the current constraint code. However, I think there's a bigger question here, which is more of a project management one. Is the current funded work on the resolver expected to deliver a result that matches bug-for-bug the current implementation, or do we have the scope to fix issues with the current behaviour? I don't know the answer to that, but I'll raise it at today's resolver project meeting. Basically, I'm certain that I don't want this feature in constraints long-term, although I'd be happy if someone wanted to add an equivalent feature that was scoped as "a way to override the finder". But if the new resolver is expected, as initially delivered, to match the existing constraint behaviour, we may need to add a temporary hack to the code to support this behaviour in the interim (I would, however, argue for adding a deprecation warning, to start the process of moving to a cleaner solution). /cc @brainwane for background before the meeting, if you get a chance :-) |
Can you elaborate that part? I don't understand. I would think the only difference between Budget and planning constraints are another thing of course, that I totally understand. Although I guess that should be put aside while discussing the mid-long term desired feature set. |
In particular, that invocation does not see https://github.com/OWNER/PKG/archive/master.zip. A constraint file (as described in the docs, and as implemented in the new resolver) removes items from the list of available candidates, to constrain the options available to the resolver when deciding what to install. The proposed behaviour for It's adding that extra candidate to the list that isn't a "constraint" as far as I can see, but rather an "extension" of the list.
Agreed. |
Thanks for the explanation, @pfmoore. I better understand the implementation complexity. |
How about as an alternative, rather than (mis-)using constraints files for this, we had a separate feature I'm sure this would immediately be subject to massive scope creep, as people decide they want to use it for all sorts of things where a custom index would be a better option, but maybe we could keep it under control 🙂 |
The part that I still don't get is why direct URLs would be considered a misuse of constraints. I mean when you view them as just another way to specify a version it all makes sense, no? So a name=>url map such as |
Sure. But if you see them as just a way to specify a version (by which I assume you mean version number), then there's no mechanism by which pip would know to consider that URL for installation, as it's not in the list of stuff the finder will locate. So a constraint of https://github.com/pypa/pip/archive/20.1.zip would be effectively identical to a constraint of It's important here not to get misled by the current implemenation, which treats constraints as (very broadly) just ordinary requirements with a "do not install" flag set. That's not conceptually how they work, though, and in a very practical sense, it's not possible for the new resolver to implement them that way (I know, I tried...)
Unnamed constraints are absolutely not allowed, IMO, as how would we know if they applied, if there's no name to match against what we're installing? And again, preparing a source dist just to say "so this is foo? Nope, we aren't interested in that" isn't reasonable. (It's less unreasonable in the current implementation, but again, don't get misled by that). |
Depends on your definition of "easy". :) Needing to "backfeed" potential sources for a name in the new resolver's (correct) model of separating requirement vs installable thing is... definitely not easy in my book.
+1 Part of the issue here is that "URLs in contraints" was not added intentionally, and has never undergone the "what can it be used for, how does that interact with other options we provide" etc discussions we have when adding functionality -- that's what's happening now. I don't think we should go unplanned functionality -> use cases, but rather use case -> functionality. That said, it's already an undocumented thing you can do with pip, and Hyrum's Law applies here -- we'd need to do a deprecation cycle if we remove it, but (IMO) that's a separate conversation of what it means and whether that is a good solution for the problem it's solving. |
I sort of like this. It meets the requirements I have, which could be described as "force installation from an unpublished location because you're running in a CI integration job that is trying to decide whether to publish the thing being tested". Regardless of whether that thing is eventually published to a custom index or PyPI, until the tests pass it isn't ready to be published at all. Now, the unpublished thing may not have a predictable version. It's not likely to be tagged, so it will have some version like 10.2.1-55 or something, and I'm not even sure it's possible to put that into a constraint file. It's certainly inconvenient to have to update the constraint to something like ">10.2.1" every time a release is tagged. If you have a few repos, it's not a big deal. When you have 10s of dozens, manual processes don't work. Selection doesn't have to be done via constraints, of course. If there's some other way to do it, that's fine. It could be the source list you describe, or it could even be a single local filesystem path so that if a package is found there then that version must be used. The CI job could pre-build wheels of everything it wants to install from source and drop them all into the same place. The key is that it's not just "a" source, we need some way to express that it is the only allowed source for a given package. |
If that suggestion seems like it would suit people's requirements, let's thrash out the details in a new feature request PR. I'll try to get some time to write up the proposal, but it will certainly be a few days at least, and may be longer (I have quite a lot on my plate at the moment) so ping me if I forget, or someone else can start things going if they want. |
@pfmoore I'm not convinced we really need to expose a new concept such as |
If everyone agrees on this point, I think we're good on this issue's goals. :) Let's close this issue and move the conversation about URL requirements/sources.txt to a separate new issue? |
I'm gonna close this issue now -- please feel free to write a comment here, if you'd like to dispute with the conclusion here (unnamed constraints are not allowed). |
Fine with me. I'm linking #8035 because I think it is at least tangentially relevant. |
Spawned from #8209.
According to
test_install_distribution_union_with_constraints
, this is a valid constraint:This is quite difficult to deal with. We can store the constraint information in a separate mapping (with the URL as key), but I’m not sure how it can be fetched. Here’s how we get the relevant constraint to use:
I guess we can do something like:
but we probably can agree this looks super wrong. Some additional abstraction is needed here.
The text was updated successfully, but these errors were encountered: