[fast-deps] Parallelize wheel download #8771

McSinyx · 2020-08-16T13:43:27Z

Example output:

$ pip --use-feature=2020-resolver --use-feature=fast-deps install scipy
WARNING: pip is using lazily downloaded wheels using HTTP range requests to obtain dependency information. This experimental feature is enabled through --use-feature=fast-deps and it is not ready for production.
Collecting scipy
  Obtaining dependency information from scipy 1.5.2
Collecting numpy>=1.14.5
  Obtaining dependency information from numpy 1.19.1
Downloading 2 files (40.3 MB)
   |████████████████████████████████| 40.3 MB 822 kB/s 
Installing collected packages: numpy, scipy
Successfully installed numpy-1.19.1 scipy-1.5.2

src/pip/_internal/network/download.py

uranusjr · 2020-08-20T07:39:53Z

Sorry for being lazy, but one question without reading the implement: Does this single progress bar implementation only apply to fast-deps, or unconditionally to all downloads?

McSinyx · 2020-08-20T07:47:16Z

It only applies to fast-deps. My bad: I added the news but forgot to update the title and the example implies my config as always.

pradyunsg

LGTM!

McSinyx · 2020-08-26T14:40:53Z

@pfmoore and @uranusjr, does this step in the right direction of what we discussed in GH-8698? If so, may I have an implementation review?

McSinyx · 2020-09-02T05:40:00Z

cdc6c38 provides _should_hide_progress, which would be useful for solving GH-8825. Is there any friction preventing this PR to be merged in the near future, or should I split the mentioned commit to a separate PR to facilitate the process (to be fair I'm not exactly comfortable about the second idea since the commit is quite tangled to other stuff in this PR, and I've burnt myself multiple times rebasing complex cases)?

I suppose that for this to be merged, we only need to consider the following points:

There's no change in pip's behavior without fast-deps
There's no foreseeable bug this introduces to fast-deps

For convenience purposes, I've made a small write-up on the behavior of pip with this patch.

McSinyx · 2020-09-08T07:10:08Z

Gentil ping! Also I hesitate to mark this as something that solves GH-825: should I?

McSinyx · 2020-10-01T15:11:28Z

Gentle ping! Off-topic: Wow it has been 23 days from last ping, time flies like crazy since I got back to school 😞

pradyunsg · 2020-10-01T22:12:10Z

Pong. /cc @cosmicexplorer

I guess updating this PR is a good idea. :P

pradyunsg · 2020-10-01T22:13:07Z

solves GH-825

Nah, let it be for now -- we'll make a big-ish celebratory post once all the little things are wrapped up nicely. :)

McSinyx · 2020-10-02T02:01:52Z

I guess updating this PR is a good idea. :P

Hmmm the conflict was introduced 9 days ago, why didn't brown truck tell me anything? BTW do you have 2FA enabled and if so how do you authenticate pushes? I'm putting a token in .git/config now and it's as safe as storing password as plain text: I wonder how 2FA makes things any more secure.

we'll make a big-ish celebratory post once all the little things are wrapped up nicely.

Woo-hoo 😄

pradyunsg · 2020-10-02T07:57:51Z

BTW do you have 2FA enabled and if so how do you authenticate pushes?

SSH keys - using the ssh variant of the git URLs and adding my public key to the GitHub account.

McSinyx · 2020-10-02T08:33:55Z

Thanks, that's much better!

McSinyx · 2020-10-09T07:12:19Z

Would you mind taking a look at this, @uranusjr? I'd be really happy to get this to land in 20.3!

cosmicexplorer

This is extremely cool!!! 🔥 🔥 🔥 I am currently making others excited about this!!

cosmicexplorer · 2020-10-09T07:28:22Z

src/pip/_internal/network/download.py

@@ -125,12 +133,24 @@ def _get_http_response_filename(resp, link):
    return filename


-def _http_get_download(session, link):
-    # type: (PipSession, Link) -> Response
+def _http_get_download(session, link, head=False):


The result of HEAD and GET seem so different that I would wonder if instead we would want to make this method's error handling into a @contextmanager. This is also driven by the thought that we might want to extract link.url.split('#', 1)[0] into its own named method, because it's not immediately clear what it's doing if you don't know or remember that pypi urls are structured this way.

To avoid breaking other callers, we could keep _http_get_download() undisturbed, making use of the new url splitting method and wrapping the session.get(...) call in the new contextmanager. That is just what first came to my mind when thinking of how to reuse the error-handling mechanisms in this method -- not a blocking review.

I understand the utility of factoring this away into a method though, so def _http_head(session, link):, which wraps its session.head(...) in the error-handling contextmanager could perhaps be a way to keep the _FileToDownload object clean. Just suggestions.

Thanks for the review!

we might want to extract link.url.split('#', 1)[0] into its own named method, because it's not immediately clear what it's doing if you don't know or remember that pypi urls are structured this way.

I also think that this is a good idea—the 1/0 numbers looks really cryptic. I'll add an reference to PEP 503 to clarify it for future readers. Edit: many spots in the codebase uses this same expression. I'd prefer to do this in a follow-up since it does not exactly fit in the scope of this PR.

Regarding splitting a separate method for HEAD, however, I don't feel that it would make the callers' code any cleaner, i.e. this level of abstraction feel just right for me, although it's just personal taste. I'll reword s/head=/just_head=/ to hopefully makes it make more sense.

Regarding head, it won’t make the caller’s code cleaner, but is still a good idea since having a boolean flag switching between behaviours is a smell in general. it is better to extract the session.method(...) call out of this function.

@cosmicexplorer and @uranusjr, does 307c4ae make it look better to you (sorry for the delay though)?

src/pip/_internal/network/download.py

BatchDownloader will need to handle the progress bars by itself. In addition, this commit changes the minimum response size to have a progress bar from 40000 bytes to 4 times chunk size.

By using head requests for information about the file, we can avoid opening too many connections at the same time and making urllib3 connection pool full.

Unfortunately HEAD responses does not carry caching information, so all files will be assumed to be downloaded from the index. Hence, the speed report might be inaccurate if there are cached files.

uranusjr · 2020-10-09T15:27:54Z

How difficult would it be to have some tests around this? pip does not currently have a lot of tests around downloading packages via HTTP (since it currently just uses requests, which is quite mature, and only in very simple ways), which this would significantly change.

BrownTruck · 2021-02-04T16:01:35Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pradyunsg · 2021-03-05T16:43:36Z

Closing as this is significantly out of date, and has merge conflicts. Please do feel free to resubmit an updated PR for this! ^>^

McSinyx force-pushed the parallel-dl branch from 6d75e93 to bf98cb7 Compare August 17, 2020 14:12

McSinyx commented Aug 18, 2020

View reviewed changes

src/pip/_internal/network/download.py Outdated Show resolved Hide resolved

McSinyx force-pushed the parallel-dl branch 6 times, most recently from 46b364c to 83ccf72 Compare August 19, 2020 15:06

eamanu mentioned this pull request Aug 19, 2020

restore a broken slug anchor in user guide #8781

Merged

McSinyx force-pushed the parallel-dl branch 2 times, most recently from 2a76451 to 846ec17 Compare August 20, 2020 04:59

McSinyx marked this pull request as ready for review August 20, 2020 05:02

McSinyx mentioned this pull request Aug 20, 2020

Parallel download: How should we display the progress? #8698

Open

McSinyx changed the title ~~Parallelize wheel download~~ [fast-deps] Parallelize wheel download Aug 20, 2020

McSinyx force-pushed the parallel-dl branch from 846ec17 to 675d47e Compare August 24, 2020 08:34

McSinyx closed this Aug 24, 2020

McSinyx reopened this Aug 24, 2020

pradyunsg approved these changes Aug 25, 2020

View reviewed changes

pradyunsg added the type: enhancement Improvements to functionality label Aug 25, 2020

McSinyx mentioned this pull request Sep 13, 2020

Dedent late download logs #8722

Merged

McSinyx force-pushed the parallel-dl branch from 675d47e to 1e9b865 Compare October 2, 2020 02:15

cosmicexplorer approved these changes Oct 9, 2020

View reviewed changes

McSinyx added 8 commits October 9, 2020 21:20

Encapsulate connection error handling

30a5780

Refactor _prepare_download for reusability

74d9409

BatchDownloader will need to handle the progress bars by itself. In addition, this commit changes the minimum response size to have a progress bar from 40000 bytes to 4 times chunk size.

Define _FileToDownload to store intermediate info

c7db18c

Parallelize BatchDownloader

6ee5d6e

Only open the connection before downloading

1c529a7

By using head requests for information about the file, we can avoid opening too many connections at the same time and making urllib3 connection pool full.

Handle cached files separately

2499c8a

Display parallel download progress

156ebf9

Finalize parallel download UI

4bb1655

Unfortunately HEAD responses does not carry caching information, so all files will be assumed to be downloaded from the index. Hence, the speed report might be inaccurate if there are cached files.

McSinyx force-pushed the parallel-dl branch from 1e9b865 to 4bb1655 Compare October 9, 2020 14:56

Stop coupling HEAD and GET requests

307c4ae

McSinyx force-pushed the parallel-dl branch from 499064d to 307c4ae Compare October 19, 2020 15:10

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 4, 2021

pradyunsg closed this Mar 5, 2021

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fast-deps] Parallelize wheel download #8771

[fast-deps] Parallelize wheel download #8771

McSinyx commented Aug 16, 2020 •

edited

Loading

uranusjr commented Aug 20, 2020

McSinyx commented Aug 20, 2020

pradyunsg left a comment

McSinyx commented Aug 26, 2020

McSinyx commented Sep 2, 2020

McSinyx commented Sep 8, 2020

McSinyx commented Oct 1, 2020

pradyunsg commented Oct 1, 2020

pradyunsg commented Oct 1, 2020

McSinyx commented Oct 2, 2020 •

edited

Loading

pradyunsg commented Oct 2, 2020

McSinyx commented Oct 2, 2020

McSinyx commented Oct 9, 2020

cosmicexplorer left a comment

cosmicexplorer Oct 9, 2020

cosmicexplorer Oct 9, 2020

McSinyx Oct 9, 2020 •

edited

Loading

uranusjr Oct 9, 2020

McSinyx Oct 19, 2020 •

edited

Loading

uranusjr commented Oct 9, 2020 •

edited

Loading

BrownTruck commented Feb 4, 2021

pradyunsg commented Mar 5, 2021

[fast-deps] Parallelize wheel download #8771

[fast-deps] Parallelize wheel download #8771

Conversation

McSinyx commented Aug 16, 2020 • edited Loading

uranusjr commented Aug 20, 2020

McSinyx commented Aug 20, 2020

pradyunsg left a comment

Choose a reason for hiding this comment

McSinyx commented Aug 26, 2020

McSinyx commented Sep 2, 2020

McSinyx commented Sep 8, 2020

McSinyx commented Oct 1, 2020

pradyunsg commented Oct 1, 2020

pradyunsg commented Oct 1, 2020

McSinyx commented Oct 2, 2020 • edited Loading

pradyunsg commented Oct 2, 2020

McSinyx commented Oct 2, 2020

McSinyx commented Oct 9, 2020

cosmicexplorer left a comment

Choose a reason for hiding this comment

cosmicexplorer Oct 9, 2020

Choose a reason for hiding this comment

cosmicexplorer Oct 9, 2020

Choose a reason for hiding this comment

McSinyx Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

uranusjr Oct 9, 2020

Choose a reason for hiding this comment

McSinyx Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

uranusjr commented Oct 9, 2020 • edited Loading

BrownTruck commented Feb 4, 2021

pradyunsg commented Mar 5, 2021

McSinyx commented Aug 16, 2020 •

edited

Loading

McSinyx commented Oct 2, 2020 •

edited

Loading

McSinyx Oct 9, 2020 •

edited

Loading

McSinyx Oct 19, 2020 •

edited

Loading

uranusjr commented Oct 9, 2020 •

edited

Loading