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

cache "concrete" dists by Distribution instead of InstallRequirement #12863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Jul 20, 2024

Split out of #12186.

Problem

The RequirementPreparer currently has a lot of mutable state it updates at various points of the resolve/prepare process. This difficulty has been exacerbated as we have added metadata-only resolve functionality via #12208 and #11111. We would like to solve #12603, but we don't have a uniform internal API for checking whether a distribution:

  1. is metadata-only and must be downloaded (via PEP 658 or --use-feature=fast-deps),
  2. must be built into a wheel from an sdist,
  3. is a wheel on the local filesystem.

We currently only have a single boolean flag InstallRequirement#needs_more_preparation to determine whether we need to do any more work to get to phase (3).

Solution

  • Add a .is_concrete property to our Distribution wrappers to describe whether the dist needs to be built into a wheel (or is already a built wheel).
  • Remove InstallRequirement#needs_more_preparation, and introduce the two methods .cache_concrete_dist() and .cache_virtual_metadata_only_dist() to manage a Distribution object attached to the InstallRequirement object itself (this is the iteration of .dist_from_metadata from use .metadata distribution info when possible #11512).
  • Execute .cache_concrete_dist() from within the AbstractDistribution#get_metadata_distribution() implementations to centralize the single place where we instantiate a dist.

Result

There should be no changes to pip's external behavior, and a lot more documentation has been added to the methods which traverse the initialization phases of a Distribution.

In particular, we now have the following distinction:

  • AbstractDistribution subclasses (still) always refer to installable distributions. This means they are not metadata-only.
  • BaseDistribution subclasses may be metadata-only. If so, they return False from .is_concrete.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jul 22, 2024

cc @edmorley @pfmoore @sbidoul @pradyunsg: this should now be ready for review, as a pure refactoring change.

@cosmicexplorer
Copy link
Contributor Author

Actually, sorry for the noise: I've convinced myself to split this out into two separate PRs. Will ping again with a smaller one to review first.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jul 22, 2024

Ok, this PR is ready for review, and #12871 has been created on top of it (in draft form). Please review this PR first, as it introduces the most important distinction of "concrete" vs metadata-only Distribution instances.

@notatallshaw
Copy link
Member

Doc failures related to #12886 ?

@cosmicexplorer
Copy link
Contributor Author

Doc failures related to #12886 ?

Thanks so much for the ping! I just rebased but I'm not sure if that will fix it.

@cosmicexplorer
Copy link
Contributor Author

docs tests passed!

@cosmicexplorer
Copy link
Contributor Author

Maybe cc @edmorley @pfmoore in case they haven't seen that this one is ready for review. I think it was a really good idea to split out this change (thanks Paul!) and I think this one is much easier to review and provides a sound basis for the subsequent ones.

@pfmoore
Copy link
Member

pfmoore commented Aug 13, 2024

Thanks for the ping, @cosmicexplorer - I get a lot of notifications for your work, mostly commits that I'd rather ignore (because I can't keep up with them!) so a specific request is appreciated.

I'll try to take a look at this over the next few days, but can I make a couple of requests?

  1. Most importantly, please don't push any further changes to this PR, if it genuinely is ready for review. If you absolutely must push a change, please don't force-push - there's nothing more frustrating for me than being part way through reviewing a PR only to find a force-push means what I'd already looked at has potentially changed. This applies to any of your changes, by the way - once they are ready for review, stop force-pushing and keep changes to a minimum, to give us a chance to review.
  2. In addition, a "master" issue that links to the various PRs you have open would be very useful. Having a summary all of the PRs, what their status is, and any interdependencies, would be extremely useful for keeping track of things. If you link to it at the top of each PR, that would keep things neat and easily referenceable.

@notatallshaw
Copy link
Member

notatallshaw commented Aug 13, 2024

2. Having a summary all of the PRs, what their status is, and any interdependencies, would be extremely useful for keeping track of things.

In particular I would be appreciative of an order in which to read the PRs. If I get a chance to help review I am currently wasn't sure in which order to approach them.

@cosmicexplorer
Copy link
Contributor Author

Thanks so much for the patient feedback, I had been thinking it would make sense to make a checklist of open work and not sure why I didn't do that earlier. It's also very easy to wait a little longer to push when changes are ready as well as avoid force pushes entirely.

@cosmicexplorer
Copy link
Contributor Author

@pfmoore @notatallshaw: created #12921 just now to track this work. You'll see that this PR is first of 3 under "To avoid downloading dists for metadata-only commands".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants