Support URL constraints in the new resolver

[mwchase]

This is an attempt to implement the behavior of URL constraints proposed in #8253 (comment). I got as far as "the tests look valid, and they pass". There are a number of choices I made where I feel like something else would be preferable, but I'm not sure what would be the best alternative.

[mwchase]

I tried to replicate those Windows failures, and I could not get sensible output out of tox. I feel like I need so much help with that that I don't even know what to ask for help with.

[uranusjr]

You can’t use < and > in script.pip() because those commands are redirections. Write those requirements into a temp file and do pip("install", "-r") instead.

[uranusjr]

I’ve only partially read the PR and need to think about the logic a bit more, but this looks very promising!

[uranusjr]

This is awesome! I have some thoughts on the implementation, but the overall logic looks good to me.

[uranusjr]

Oh I forgot to mention, maybe we can introduce a Links construct (similar to Hashes) to abstract away the FrozenSet[Link] logic. That can be done in a separate optimisation, but it’s fine if you want to do it here as well.

[mwchase]

I did my best to address the other concerns, but I haven't looked closely enough at the Hashes class to get a feel for writing something similar.

[uranusjr]

I think this is ready!

[sbidoul]

@mwchase thanks a lot for this work! I've not had time to review but I made a first test.

It works well but there is one issue I noticed: it does not generate direct_url.json.

Assuming constraints.txt like this:

git+https://github.com/pypa/pip-test-package@5547fa909e83df8bd743d3978d6667497983a4b7

pip install pip-test-package -c constraints.txt should generate a direct_url.json file in site-packages/pip_test_package-0.1.1.dist-info/, so pip freeze can then produce the constraint URL.

I've not had the chance to look in details yet. I guess at some point self.original_link must be set to be the constraint URL?.

[mwchase]

I haven't figured out how to fix that, but I did get a test written, so I pushed it just so this wouldn't look better than it is.

[uranusjr]

So direct_url.json is created by inspecting InstallRequirement.original_link. An InstallRequirement referencing a direct URL will have it populated, while one created from a version specifier will have it set to None. A “usual” direct URL candidate is always created from an ExplicitRequirement template, which is always backed by an InstallRequirement with original_link set.

But a direct URL candidate created from a constraint does not have an ExplicitRequirement to get the template from, so its backing InstallRequirement will have original_link set to None. I’m not sure how best to fix this either yet. The “easiest” fix would be to manually set candidate._ireq.original_link after its creation, but that’s pretty obvious a violation of abstraction.

We actually already have an InstallRequirement with original_link set. The resolver receives a list of InstallRequirement objects, and we currently threw those backing constraints away after Constraint objects are created. Maybe we can instead save those and use them as template here.

Yet another solution would be to create a “fake” InstallRequirement to use as a template instead. This isn’t actually too difficult, but may feel a bit wonky to do.

[mwchase]

I tried a few different ways. I didn't figure out the way to make "fake" InstallRequirements, but I did try just editing the new requirement, and using the constraint as a template.

Using the constraint broke a bunch of the hash tests I added, and somehow didn't get the new test to pass.
Reaching into the new requirement worked fine, but it does feel kind of off.

So, I don't think ignoring the actual requirements for template purposes will work. This sounds somewhat related to the note near the beginning of the factory's _iter_found_candidates method.

Would the right way to make a "fake" InstallRequirement be to call make_install_req_from_link or make_install_req_from_editable myself and modify the return value to be the new template, or is that the wrong part of the code to be looking at?

[uranusjr]

It may make more sense to use the lower level functions in pip._internal.req.constructors instead since we’re delibrately not trying to copy from a template ireq.

What are the test failures you got if you use the ireq from the constraint as template? I think this approach makes most sense in abstract (although may be tedious in practice since we need to keep an ireq reference all the way down the code path), and we may be able to fix those errors instead if possible.

[mwchase]

I believe it somehow didn't actually create the direct_url.json file, though I'd have to rerun to be sure. More definitely, it seems like it lost the hash constraint information that was associated with the requirement lines.

[uranusjr]

More definitely, it seems like it lost the hash constraint information that was associated with the requirement lines.

That’d make sense, since the hashes are collected from InstallRequirements. So if we’re using the constraint’s ireq, we’ll need to manually populate the hashes back.

[mwchase]

I'm currently trying out a few things at once. One is to write my own helper to generate a template for the method. This seems to work, but as I was thinking about it, I noticed a hole in my test coverage, and I'm currently working on patching that.

If writing my own function for "like this InstallRequirement, but with this link" is acceptable, I assume it should go in the constructors module in order to keep proper track of it?

As far as the hole in the coverage, it should be the case that if a package has an editable requirement and a constraint URL, it should get an editable install. I think that was working, but I don't think I had a test, so I added one.

[uranusjr]

If writing my own function for "like this InstallRequirement, but with this link" is acceptable, I assume it should go in the constructors module in order to keep proper track of it?

It’d also make sense to put it somewhere in the resolver submodule since it is only used there. But I don’t think it really matters that much, let’s just put it somewhere and discuss after things work.

As far as the hole in the coverage, it should be the case that if a package has an editable requirement and a constraint URL, it should get an editable install. I think that was working, but I don't think I had a test, so I added one.

IIRC having both a URL and an editable (with the same URL, of course) in the requirements would currently fail the resolver, because installing an editable and non-editable are not technically interchangeable currently. Like the link equivalency issue, we should check what the resolver’s current logic is for requirements, and try to do the same thing with constraints for now, and revisit the whole logic afterwards (and change it for both if we decide to change it).

[mwchase]

IIRC having both a URL and an editable (with the same URL, of course) in the requirements would currently fail the resolver, because installing an editable and non-editable are not technically interchangeable currently.

I was basing my reasoning on #8253 (comment):

"editable" is about how you install things, not about what you install. Constraints are entirely about what gets installed

Which I interpreted as "unlike requirements, constraints should have no bearing on whether the install is editable", which implies that constraints should accept either situation for requirements.

In any case, I'll get the code cleaned up and pushed in the next day or so.

[sbidoul]

Works perfectly in all my tests with pip-deepfreeze. I'm not familiar enough with the new resolver logic to do a full review, but from a functional standpoint 👍, thank you!

[uranusjr]

Giving the CI a kick

[sbidoul]

If there are no objections I'm going to merge so it goes in 21.1

[sbidoul]

One thing, though, @mwchase could you squash your commits ? The history here is not very relevant, and that would help in the unlikely case we'd need to revert after release for any reason.

[sbidoul]

Thank you @mwchase !

[apljungquist]

@mwchase thank you for implementing this.

Looking at your activity on issues like 8439 I get the impression that you are trying to enable a specific use case that you have. I suspect that I have a similar use case and I am curious to hear more about yours and how you have gone about to enabling it - I expect I could learn a thing or two.

To shift the burden of communication slightly to my side, I will describe my use case, how I have solved it and what problems I am still facing.

I maintain multiple, interdependent, private python packages in a single git repo. One reason for having multiple packages is that they deployed into different environments. Simplified the run time dependency graph looks something like below.

dependency-+------------+
           +-dependent0-|------------ # deployed to environment0
scipy-------------------+
                        +-dependent1- # deployed to environment1

For the above example I would have a requirements file like below. This does not work on recent releases of pip.

-e file://$TOP_LEVEL$/packages/dependency#egg=dependency
-e file://$TOP_LEVEL$/packages/dependent0#egg=dependent0
-e file://$TOP_LEVEL$/packages/dependent1#egg=dependent1
scipy

A git filter is used to replace $TOP_LEVEL$ with the absolute path of the top-level directory of the working tree.

The requirements file is compiled to a constraints file using pip-compile. Now any package can all be installed like pip install -e ./packages/package.

As for the problems that I am facing:

1. Thanks to you using URLs in the constraints file works 🥳 However, since they are not allowed to be editable dependencies will not be installed as editable making it hard to work on multiple packages at once. This can be worked around by installing the dependency after the dependent, but if I then install the other dependent then pip will again install the dependency as non-editable.
2. I cannot add hash checking to external packages because (a) either all packages must have hashes or none, and (b) there is no hash value that pip will accept when installing from a directory.
3. Constraints are not respected by pip at build time. Thank you for the workaround, I have yet to try it but it looks good.
4. pip-compile outputs extras but pip do not accept these in constraints files (though this can be worked around with a regex search-and-replace).
5. Propagating constraints through tox (though I am now thinking that your workaround could help with that too).

I apologize if this thread is the wrong forum but I could find no way to message you directly nor a better thread to ask in.

[mwchase]

Let's see. From what you've described, I think my use case is similar to yours, but simpler. (No editable installs, no expectation of running on another platform (yet).) So, right now the work and workarounds I have are sufficient for me.

Now, here's what I remember about the state of things; this could be off because I was mostly focused on stuff relevant to my use cases.

1. As far as editable installs, from discussion on the related issue, I know the desired behavior is that only requirements files can make a package editable. There's another issue for allowing constraints files to handle -e, but it would be handled by just ignoring it. So, I don't think that would help.
2. My initial reaction is to wonder whether splitting up the installation steps could work. Install external dependencies in one step with hash checking, and install local packages in editable mode, possibly using --no-deps. But that isn't one command.
3. I hope this generally works.
4. I believe extras are in a similar situation to editable (issue filed, planned behavior is to ignore; this shouldn't be as big of a problem if pip-compile is already resolving everything).

Anyway, thinking over all of that, I think that, unless I'm missing something, your best bet may be to write some kind of a wrapper around pip to handle generating the proper -e arguments. As it stands, I don't think there's a way to tell pip "If you install this package, make it an editable install". That'd need to be its own issue, with probably a lot of discussion.

Sorry for any inaccuracies, and that I couldn't be more help.

[apljungquist]

Just getting your opinion has been helpful. Thanks for taking time to read and reply.