Skip to content

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

Open
cosmicexplorer wants to merge 1 commit intopypa:mainfrom
cosmicexplorer:dist-caching-changes
Open

cache "concrete" dists by Distribution instead of InstallRequirement#12863
cosmicexplorer wants to merge 1 commit intopypa:mainfrom
cosmicexplorer:dist-caching-changes

Conversation

@cosmicexplorer
Copy link
Copy Markdown
Contributor

@cosmicexplorer cosmicexplorer commented Jul 20, 2024 •
edited
Loading

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 cosmicexplorer mentioned this pull request Jul 20, 2024
Draft
@ichard26 ichard26 added the type: refactor Refactoring code label Jul 20, 2024
@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Jul 22, 2024 •
edited
Loading

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

@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Jul 22, 2024

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 cosmicexplorer marked this pull request as draft July 22, 2024 22:56
@cosmicexplorer cosmicexplorer marked this pull request as ready for review July 22, 2024 23:01
@cosmicexplorer cosmicexplorer mentioned this pull request Jul 22, 2024
Draft
@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Jul 22, 2024 •
edited
Loading

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.

@cosmicexplorer cosmicexplorer force-pushed the dist-caching-changes branch 2 times, most recently from 5a329fe to 4e7d29f Compare July 27, 2024 11:52
@notatallshaw notatallshaw mentioned this pull request Aug 5, 2024
Closed
1 task
@ichard26 ichard26 closed this Aug 9, 2024
@ichard26 ichard26 reopened this Aug 9, 2024
@ichard26 ichard26 self-requested a review August 9, 2024 00:35
@notatallshaw
Copy link
Copy Markdown
Member

notatallshaw commented Aug 9, 2024

Doc failures related to #12886 ?

@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Aug 13, 2024

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
Copy Markdown
Contributor Author

cosmicexplorer commented Aug 13, 2024

docs tests passed!

@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Aug 13, 2024

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
Copy Markdown
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
Copy Markdown
Member

notatallshaw commented Aug 13, 2024 •
edited
Loading

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
Copy Markdown
Contributor Author

cosmicexplorer commented Aug 15, 2024

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 cosmicexplorer mentioned this pull request Aug 17, 2024
Open
10 tasks
@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

cosmicexplorer commented Aug 17, 2024

@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

Reviewers

@ichard26 ichard26 Awaiting requested review from ichard26

Assignees

No one assigned

Labels

bot:chronographer:provided type: refactor Refactoring code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cosmicexplorer @notatallshaw @pfmoore @ichard26