Improve handling of constraints on requirements with extras

[sanderr]

Description

These changes improve the handling of constraints on requirements with extras (#11924). It includes the following changes:

1. Constraints are now all taken into account for the base candidate. To this end hello[world]~=1.0 now produces the implicit hello~=1.0 requirement. This allows us to provide resolvelib with a more representative candidate list for the base. The requirements with an extra then serve only to require the extra for the selected package. In cases where multiple constraints occur for the same package but with different extras (see examples below) this greatly reduces the number of candidates that need to be visited, and therefore downloaded.
2. Minor bugfix in comes_from tracking for requirements with extras: pip._internal.resolution.resolvelib.factory keeps a cache for link candidates, which would result in the very first requirement for a project being considered the origin of any candidates for it, regardless of the extras on the candidate or requirement. This bug exists on the main branch but with the changes I made for 1 it triggered consistently so I had to fix it in order to keep install reports stable. See examples below.
3. Minor bugfix in install report generation where only the very first requirement for a project would be considered for requested_extras in the report. Similar to 2, this bug can be triggered on main but it became consistent with the changes I made for 1. See examples below.

closes #11924
closes #12210

Status

This pull request still requires some minor refinement and cleanup, more specifically I still need to add tests, run style checks, fix compatibility with older Python versions (nox -s test-3.11 currently succeeds on my system) and add a news fragment. Before I get to that I thought it would be useful to already open it up as a draft pull request to get an initial conceptual review. If my approach has some fundamental problems I think this is the best time to address it.

Ready for review

Examples

#11924 contains a few examples of the issue. Each of those has been greatly improved by these changes. For consistency I'll limit myself to more minimal examples here:

1. pip download Paste Paste[openid]==1.7.5.1 works as expected on the latest stable pip but pip download Paste==1.7.5.1 Paste[openid] downloads 35 versions of Paste, no matter which order the two constraints appear in. With the changes in this pull request only the requested version is downloaded. Similar observations can be made for Paste[subprocess] ==1.7.5.1 Paste[openid] and 'Paste[subprocess]==1.7.5.1' 'Paste[openid]>=1.8' (which is trivially rejected with these changes).
2. With the latest stable pip download Paste Paste[openid] would log a line Collecting setuptools (from Paste). If you swap the order of the requirements it would get it right: Collecting setuptools (from Paste[openid]). With the changes in this pull request it will always get it right.
3. With the latest stable pip install --dry-run --report Paste==1.7.5.1 'Paste[openid]==1.7.5.1' would not include any requested_extras in the report. Swapping the requirements would report the requested extras correctly. Similarly for pip install --dry-run --report myreport.txt Paste[openid] Paste[subprocess] only openid would be in the report. With the changes in this pull request all requested extras will be present.

[uranusjr]

Instead of trying to modify a Requirement in-place (which IIRC is not documented to be an officially supported operation), it may be better to strip the extras in the requirement string instead. This should not be too dificult with some regex since both the project and extra names are quite predictable.

(I’m not committed to this though and wouldn’t object if packaging maintainers are OK with this cc @pradyunsg)

[sanderr]

I made the change though I don't like the fact that it feels a bit more brittle. On the other hand I do agree with your sentiment that I shouldn't something that isn't part of packaging.Requirement's contract. @pradyunsg do you have an opinion on this (faa3289)?

[pfmoore]

I'd be happy if packaging made it a supported operation, but the existing docs state that the Requirement class is for parsing the string form of a requirement, which to me doesn't imply the result is mutable. You could read it as saying that it parses the string into a structured form, which you are allowed to them mutate if you want, but I don't think that automatically follows.

So as things stand, I agree with @uranusjr.

As @pradyunsg is a packaging maintainer, if he says it's OK, then I'm fine with that (although I'd ask that the docs be changed to explicitly state what's supported).

[pradyunsg]

Yea, don't modify this in place. It doesn't break things as implemented right now but making this a frozen dataclass is something I want to do in the future.

[sanderr]

Thanks for your input. A frozen dataclass would be nice, that would then allow for dataclasses.replace, which I'd always prefer over in-place modifications.

[uranusjr]

Do we need the regex to be this complicated? I thought the extra is pretty easy to detect, isn’t all needed is to scan for \w and split the first non-match?

[sanderr]

You make a good point, if I interpret PEP508 correctly extras (if present) will always come right after the name. I don't think \w suffices because dots and dashes are allowed as well, but even with that modification to your proposal it should be simpler than what I used here.

[uranusjr]

Yes, the extras come immediately after, so we can scan for the name, skip any spaces if needed, remove existing extras if needed, and add the extras right in.

[sanderr]

Applied suggestion in 314d7c1

[sanderr]

@uranusjr @pradyunsg is there anything still blocking this from being merged? I rerequested review from @uranusjr, but I'm honestly not sure what your typical procedure for reviews is, I couldn't find anything about that in the contributing guidelines. If it's just that you've been too busy to give this your attention I don't mind being patient, I just want to make sure it's still on the radar and not blocked on something from my side.

[pradyunsg]

Thanks for checking! It's mostly availability, if I understand correctly.

I've added this to the milestone for the upcoming release. If @uranusjr isn't able to take a look at this by then, due to lack of availability, let's land this (assuming the RM doesn't have concerns around this change).

[uranusjr]

I really don’t like how Black formats some of the code but that’s not your fault and there’s not much we can do 😮‍💨

[uranusjr]

I’ll just merge since reading the above discussion I seem to be the only blocker anyway.

[AndydeCleyre]

Hello! This introduced a change in pip-tools behavior (jazzband/pip-tools#2003), as a bunch of non-guaranteed pip internals are still used for now.

Before addressing it in pip-tools, I wanted to check that the change here was intentional. It seems from point 1 of the description that it may indeed be.

---

As of 5f8f40e, we're getting for example coverage listed as a dependency of coverage[toml] in some cases, demonstrated with the first example here, where it's coming in as a dependency of pytest-cov:

$ <<<'pytest-cov' pip-compile --annotation-style line - -o - 2>/dev/null | grep coverage
coverage[toml]==7.3.2     # via coverage, pytest-cov

$ <<<'coverage[toml]' pip-compile --annotation-style line - -o - 2>/dev/null | grep coverage
coverage[toml]==7.3.2     # via -r -

$ <<<'pytest-cov'$'\n''coverage[toml]' pip-compile --annotation-style line - -o - 2>/dev/null | grep coverage
coverage[toml]==7.3.2     # via -r -, pytest-cov

---

I'm guessing that this is just a pip-tools problem where we need to account for the change, but mention it here in case it indicates a surprise effect.

[sanderr]

@AndydeCleyre I just had a look at this and I am inclined to agree that this is an intentional internal change. As far as I can tell the unexpected "via coverage" comes from the InstallRequirement "coverage[toml]==7.3.2 from https://<...>.whl (from coverage[toml]>=5.2.1->pytest-cov->-r -)". Before this pull request this would be just "... (from pytest-cov->-r -)".

As you already inferred, the change in behavior is due to point 1 in the description: coverage[toml]>=5.2.1 now results in two distinct requirements: coverage>=5.2.1 and coverage[toml]. Semantically, I feel that it is appropriate that these requirements point to coverage[toml]>=5.2.1 as their origin (comes_from). It is only because pip-compile trims some of the constraint metadata (I don't mean to say that it shouldn't) that it comes across as strange.

With the following rough patch I get the old behavior back. I'm not sure if the name check is a good/stable metric, but it may be of use in finding a proper solution.

diff --git a/piptools/writer.py b/piptools/writer.py
index 1ae341d..92a2a33 100644
--- a/piptools/writer.py
+++ b/piptools/writer.py
@@ -295,7 +295,7 @@ class OutputWriter:
                 if src_ireq.comes_from
             }

-        if ireq.comes_from:
+        if ireq.comes_from and (isinstance(ireq.comes_from, str) or ireq.comes_from.name != ireq.name):
             required_by.add(_comes_from_as_string(ireq.comes_from))

         required_by |= set(getattr(ireq, "_required_by", set()))

To be clear: I'm not a pip maintainer, so this comment is not authoritative and represents nothing more than my view on the situation. Let me know if you have further questions or if you disagree with any of the above.

PS: sorry about the commit history you had to wade through. I'm used to pull request commits being squashed before merging so I don't always pay too much attention to make clear commits.