Use data-dist-info-metadata (PEP 658) to decouple resolution from downloading

[cosmicexplorer]

Problem

#10748 implements several things at once, which makes it large and difficult to review. After a recent discussion on that PR (#10748 (comment)), I realized that support for PEP 658 is totally orthogonal to generating a resolution report.

Solution

• Implement PEP 658 support on top of the existing --use-feature=fast-deps work from @McSinyx.
  • This will look for and fetch *.metadata files if provided by the index instead of using LazyZipOverHTTP. If --use-feature=fast-deps is enabled, we then fall back to LazyZipOverHTTP if this is unavailable (which is the case for PyPI currently, and will remain the case for any --find-links repos).
• Add thorough testing for success and failure modes of PEP 658 (this wasn't done in metadata-only resolve with pip download --dry-run --report! #10748 already).

Result

When PyPI implements PEP 658, we will immediately be able to use it to speed up resolves!

cc @uranusjr

[pradyunsg]

I'm here to say that it's almost 01:11:11 where I am, and I really like this PR's number. :)

[cosmicexplorer]

@sbidoul: I was able to disentangle ArchiveInfo and LinkHash here and remove a lot of code in the process (it now avoids touching direct_url.py at all). I think it might make sense to have ArchiveInfo retain an Optional[LinkHash] instance instead of an Optional[str], especially since link.py no longer imports direct_url.py, so there is no import cycle. What do you think?

[pradyunsg]

Hmm... I don't think the PEP 658 code path needs to be behind the fast-deps flag. It is standard-backed behaviour, which should be implemented in a way that makes things work.

[pradyunsg]

Beyond that, I have to pack for a trip -- so I won't be able to look at this properly for ~a month (unless I end up looking at this at the airport or whatever). :)

I'm reallly excited that you've done this work though @cosmicexplorer! While I've been a bit busy getting the 21.1 release out, I'm looking forward to getting this PEP landed, hopefully in time for the next release cycle in a couple of months. ^.^

[uranusjr]

I don’t think this should be behind a flag either. Instead, it should be done in a way that works by default, and falls back gracefully if the server does not provide this information.

Thanks a ton for doing this though! This is something I’ve been thinking about but never had the time for.

[cosmicexplorer]

It is standard-backed behaviour, which should be implemented in a way that makes things work.

Instead, it should be done in a way that works by default, and falls back gracefully if the server does not provide this information.

This totally makes sense! Luckily the implementation from @McSinyx was so nice that it was very easy to incorporate this metadata-only resolution without needing to activate --use-feature=fast-deps -- take a look at b36ae5a!

[cosmicexplorer]

@pradyunsg:

While I've been a bit busy getting the 21.1 release out, I'm looking forward to getting this PEP landed, hopefully in time for the next release cycle in a couple of months. ^.^

Thanks so much for the timeline!!! I really appreciate these updates. There's no rush on my end, so please just come back to this as you have time. For my part I'll work to make this mergeable so you can hopefully breeze through it.

[pradyunsg]

The implementation and tests LGTM. One minor comment. :)

Also, could you rebase this? :)

[pfmoore]

Also, could you rebase this?

You should be aware that we have an implementation of PEP 691 now. It's possible that the merge conflicts might not indicate that you might need some parallel logic for the case of a JSON format index.

Also, while it will be great to have this in pip, PyPI still doesn't actually serve any projects with data-dist-info-metadata set. It looks like that's blocked on pypi/warehouse#8254.

[pradyunsg]

Yea, I'd prefer to take the same approach as PEP 591 -- we implemented the data-yanked handling in pip quite a while before PyPI implemented it on the index server side. :)

[pradyunsg]

FWIW, I'm also likely going to touch this code as a part of #7049 (which in turn, blocks #6607). I do think it makes sense to defer making my changes until we can land this tho. :)

[pfmoore]

Please let me know if that looks like a nice way to integrate the PEP 691 work.

I've only had time to skim the code, but this looks like a very reasonable refactoring, yes.

[cosmicexplorer]

@pfmoore: ah, I just reviewed PEP 691 and saw that it also incorporates the dist-info-metadata attribute. In 8da1bdc I ensure that Link.from_json() uses the same code path for its dist-info-metadata attribute that we do for data-dist-info-metadata in Link.from_element(). I'll add a test for that presently.

[cosmicexplorer]

Hm, I'm seeing that there aren't already test cases for PEP 691 unless I'm misreading 6f167b5, so I'm going to do that now but put it in a separate PR to avoid inflating this one further.

[cosmicexplorer]

Oh nvm, I see it as test_parse_links_json()! Will stop pinging this thread and use that.

[cosmicexplorer]

Ok, this should be ready to review again! cc @uranusjr

[uranusjr]

One comment on the (internal) interface. Logic looks fine to me.