Download wheels in batch at the end of .prepare_linked_requirements_more()

[cosmicexplorer]

This PR corresponds to the second work section described in #7819 (comment) from the much-too-large #8448:

move out "download entire file" out of prepare (and hence, Resolver.resolve)

With some modifications to that prompt, it now consists of:

1. Create a method RequirementPreparer._complete_partial_requirements() to fully download and prepare lazily-downloaded wheels.
2. Call that new method at the bottom of .prepare_linked_requirements_more(), separating the batch download from the rest of that method.

[cosmicexplorer]

cc @pradyunsg @McSinyx wrote a tweet about much I loved your work on the lazy wheel fetching: https://twitter.com/hipsterelectron/status/1308306011409661954?s=20

[McSinyx]

I'm sorry to disappoint you, the lazy wheel isn't making download any faster at the moment. I suppose the other use-case (pip resolve) is possible with extra steps and this is what you're trying to do here.

Regarding the news file, since pip's internal API is internal, I think this should be flagged as trivial. Concerning the approach, I can recall discussions with @pradyunsg and @chrahunt resulting in keeping the late download in the resolver (at least until the legacy resolver is removed, to keep the consistency of output by the two resolvers), in particular, within RequirementPreparer (see GH-8685). IIRC @pradyunsg told me that it's preferable to pass a dry_run option to the resolver instead—the reason pip resolve has not been implemented is rather we don't know to be exact what format of the output would be most helpful.

Side comment: the use PartialRequirementDownloadCompleter is solely init then call it's only method, which I would love to see to be changed into a function if it turns out that my memory regarding the general approach said above isn't correct.

[pradyunsg]

Me likey!

That was the wrong shortcut, on the wrong window. :)

[pradyunsg]

Me likey the general clarity that factoring out the logic for the batch download logic brings.

As @McSinyx mentioned, until the older resolver dies, we'd want to keep the "download everything else" inside the resolver -- maintaining the abstraction of "everything is ready". While the "prepare more" logic is indeed a no-op with the old resolver, it'll be nice to keep it inside to be able to reason about these things more easily until that old one goes away.

Let's keep the download logic inside the resolver.resolve call, and add a flag to tell the resolver whether to fully-prepare partially prepared items. :)

[pradyunsg]

I made a mess of the review here -- but that's what I get for reviewing at 1am. :)

[cosmicexplorer]

I'm sorry to disappoint you, the lazy wheel isn't making download any faster at the moment.

You couldn't possibly disappoint me!!! Sounds like it's time for some profiling 🔍 . Sorry for disappearing off the face of the earth for so long! Your adaptation was much much cleaner than what I had and that is a much better place to start off than the other way around (at least, in my opinion, in this case).

I suppose the other use-case (pip resolve) is possible with extra steps and this is what you're trying to do here.

So my thought process was actually just that parallel downloading would be the key to actually improving the download performance!! And if not, I was thinking a further optimization would be to keep alive the connections used to download the metadata, in case requests wasn't doing that already. This is why I'm sorry I was so quiet for a few months 😅. This is definitely on me providing an unclear description of the goal and not handing off my work correctly!

Regarding the news file, since pip's internal API is internal, I think this should be flagged as trivial. Concerning the approach, I can recall discussions with @pradyunsg and @chrahunt resulting in keeping the late download in the resolver (at least until the legacy resolver is removed, to keep the consistency of output by the two resolvers), in particular, within RequirementPreparer (see GH-8685).

This is super super helpful especially with the link to the PR I missed!! Hardly gave @pradyunsg much else to review ^_^

IIRC @pradyunsg told me that it's preferable to pass a dry_run option to the resolver instead—the reason pip resolve has not been implemented is rather we don't know to be exact what format of the output would be most helpful.

Yes! dry_run seems much closer to the option I'd expect. Off the top of my head I was thinking something like a constraints.txt with pinned (==) requirements would be most interesting (I think whatever pip freeze produces would likely be super neat) -- but will follow up in #53!

Side comment: the use PartialRequirementDownloadCompleter is solely init then call it's only method, which I would love to see to be changed into a function if it turns out that my memory regarding the general approach said above isn't correct.

Will do!

Let's keep the download logic inside the resolver.resolve call, and add a flag to tell the resolver whether to fully-prepare partially prepared items. :)

This makes perfect sense! It's also less code ^_^

I made a mess of the review here -- but that's what I get for reviewing at 1am. :)

You reviewed at approximately the same time I posted it, then! We'll call it even.

[cosmicexplorer]

Instead of doing this:

add a flag to tell the resolver whether to fully-prepare partially prepared items. :)

I instead just added an extra method .complete_partial_requirements() in operations/prepare.py (and updated the description), because it seemed that piping a PipSession instance into the resolver would risk it being used for other things that were previously nicely separated. So instead I made use of the existing fields of the RequirementPreparer to just ensure that resolver.resolve() could clearly mark which part did the batch downloading. I hope that seems reasonable -- totally willing to refactor if either of you two had something else in mind!

[pradyunsg]

Hmm... keeping things on RequirementPreparer itself... should get us nearly the same benefits (separation of "collect" vs "download" in prepare_more). I think we can get way without introducing changes on the resolver side?

We'd skip the entire "prepare_more" stage from the resolver during a dry run anyway, so... :)

[cosmicexplorer]

should get us nearly the same benefits (separation of "collect" vs "download" in prepare_more).

Yes! This is a much nicer way to think about it. Will do.

We'd skip the entire "prepare_more" stage from the resolver during a dry run anyway, so... :)

Great point!! (along with the rest)

[cosmicexplorer]

Done, I think! I think this gets us the separation you noted above, paving the way for other methods of downloading requirements, as well as not downloading them at all!

[cosmicexplorer]

ping!

[xavfernandez]

This seems clean enough but would need rebasing :)

[cosmicexplorer]

Rebased!

[cosmicexplorer]

Perhaps @xavfernandez is the right person to ping here?

[cosmicexplorer]

Ping once more! ^_^

[cosmicexplorer]

Let me know if there's someone specific I should contact!

[pradyunsg]

Nah -- I'll merge this once #8936 is done with (later this week). :)

[cosmicexplorer]

ping @pradyunsg! ❤️

[cosmicexplorer]

ping!

[pradyunsg]

Pong! pip 20.3 hasn't been released yet, because, well, 2020.

I will come back to this, once that's done.

[cosmicexplorer]

Not a problem!!! Thanks for the update. I will follow along with the 20.3 updates then!

[uranusjr]

Come on, report your status, Azure.