pip downloads packages twice if the data-dist-info-metadata is set

[fiannac]

Description

I'm using a custom package index that implements PEP-658. When I install packages through that index specifying the --no-cache-dir flag pip downloads the package twice.

Expected behavior

No response

pip version

23.0.1

Python version

3.9.7

OS

CentOS 7.9

How to Reproduce

Run pip install --no-cache-dir --index-url PEP-658index -v package

Output

pip install --no-cache-dir --index-url http://localhost:8000/simple/ -v numba
Using pip 23.0.1 from ... (python 3.9)
Looking in indexes: http://localhost:8000/simple/
Collecting numba
  Obtaining dependency information for numba from http://localhost:8000/.../numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata
  Downloading http://localhost:8000/.../numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata (2.8 kB)
Requirement already satisfied: setuptools in /.../site-packages (from numba) (59.5.0)
Requirement already satisfied: llvmlite<0.40,>=0.39.0dev0 in /.../site-packages (from numba) (0.39.1)
Requirement already satisfied: numpy<1.24,>=1.18 in /.../site-packages (from numba) (1.23.5)
Downloading http://localhost:8000/.../numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl (3.5 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.5/3.5 MB 105.5 MB/s eta 0:00:00
Downloading http://localhost:8000/.../numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl (3.5 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 3.5/3.5 MB 120.8 MB/s eta 0:00:00
Installing collected packages: numba
Successfully installed numba-0.56.4

Code of Conduct

• I agree to follow the PSF Code of Conduct.

[uranusjr]

What are you setting in data-dist-info-metadata? If it is set pip downloads the content it points to for metadata. It looks like your attribute points to the same wheel, which should not be the case.

[pradyunsg]

Could you share the HTML file that triggered this, as well as the contents of the METADATA file served by the host?

The metadata file seems to be distinct: numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata

[fiannac]

What are you setting in data-dist-info-metadata? If it is set pip downloads the content it points to for metadata. It looks like your attribute points to the same wheel, which should not be the case.

From PEP658:

The repository SHOULD provide the hash of the Core Metadata file as the data-dist-info-metadata attribute’s value using the syntax hashname=hashvalue, where hashname is the lower cased name of the hash function used, and hashvalue is the hex encoded digest. The repository MAY use true as the attribute’s value if a hash is unavailable.

I am setting the values to true since I do not have access to the hashes. I also think pip is downloading and using the metadata correctly:
Downloading http://localhost:8000/.../numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata (2.8 kB)

[fiannac]

Could you share the HTML file that triggered this, as well as the contents of the METADATA file served by the host?

The metadata file seems to be distinct: numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata

I'm currently just proxying pypi.org, adding data-dist-info-metadata to all the a tags pointing to a wheel dist and unwrapping the wheels on the fly when a client asks for metatadata. So the html is just the project page provided by pypi.org and the metadata is the METADATA file inside the wheel.

[fiannac]

Could you share the HTML file that triggered this, as well as the contents of the METADATA file served by the host?

The metadata file seems to be distinct: numba-0.56.4-cp39-cp39-manylinux2014_x86_64.manylinux_2_17_x86_64.whl.metadata

I've uploaded on this repo an html file that triggers this, you can reproduce that locally following the setps in the readme

[pradyunsg]

Thank you for that @fiannac! That's really useful.

[pradyunsg]

/cc @pfmoore, who is also looking at the PEP 658 + PEP 714 situation in #12078. I reckon this is something we should try to fix in the first release using the new key name.

[pfmoore]

I'll take a look, but I can't promise I'll have time to work out what the problem is here, much less create a fix. So PRs would be welcome.

Worst case scenario, if no-one comes up with a fix for this, we'll have PEP 658/714 support in 23.2 with this issue unfixed. Does anyone think that's a serious problem (serious enough to pull #12078 from 23.2)? My instinct is to say no, we go ahead with #12078, but a double download does sound like a rather serious performance regression, for something that was supposed to be a performance improvement 🙁

[pfmoore]

I've tried setting a breakpoint in pip._internal.network.download in Downloader.__call__, just before the HTTP response is read and written to the target file. The breakpoint is only triggered once, but the Downloading filename.whl (NN kB) message is printed twice before the breakpoint is hit. So I'm pretty sure that the issue here is simply one of the "Downloading" message being logged twice, rather than the file actually being downloaded twice.

Obviously, it's misleading and needs to be fixed, but I'm reasonably confident that there isn't a performance issue here.

Before I downgrade this from release blocker, can anyone demonstrate that my analysis above is wrong?

Note that #12078 has now been merged, and PyPI is serving (some) metadata files, so this issue can be reproduced using pip main and PyPI. The command line I used was pip install --dry-run --no-deps --no-cache piel -vvv (I used the piel package simply because it has had a very recent release which has metadata available on PyPI).

[fiannac]

I've tried setting a breakpoint in pip._internal.network.download in Downloader.__call__, just before the HTTP response is read and written to the target file. The breakpoint is only triggered once, but the Downloading filename.whl (NN kB) message is printed twice before the breakpoint is hit. So I'm pretty sure that the issue here is simply one of the "Downloading" message being logged twice, rather than the file actually being downloaded twice.

When I run the same command and check the server-side logs, it really looks like the file is being downloaded twice:

Client:
pip install --dry-run --no-deps --no-cache --force-reinstall --index-url http://localhost:8082/simple/ abcabc
Collecting abcabc
  Obtaining dependency information for abcabc from http://localhost:8082/simple/abcabc/abcabc-0.0.7-py3-none-any.whl.metadata
  Downloading http://localhost:8082/simple/abcabc/abcabc-0.0.7-py3-none-any.whl.metadata (92 bytes)
Downloading http://localhost:8082/simple/abcabc/abcabc-0.0.7-py3-none-any.whl (2.4 kB)
Downloading http://localhost:8082/simple/abcabc/abcabc-0.0.7-py3-none-any.whl (2.4 kB)

Server:
127.0.0.1 - - [28/Jun/2023 14:12:21] "GET /simple/abcabc/ HTTP/1.1" 200 -
127.0.0.1 - - [28/Jun/2023 14:12:21] "GET /simple/abcabc/abcabc-0.0.7-py3-none-any.whl.metadata HTTP/1.1" 200 -
127.0.0.1 - - [28/Jun/2023 14:12:21] "GET /simple/abcabc/abcabc-0.0.7-py3-none-any.whl HTTP/1.1" 200 -
127.0.0.1 - - [28/Jun/2023 14:12:21] "GET /simple/abcabc/abcabc-0.0.7-py3-none-any.whl HTTP/1.1" 200 -

[pfmoore]

OK. We'll need a complete reproducer that I can use to replicate the issue locally, as I can't confirm the problem here.

[fiannac]

OK. We'll need a complete reproducer that I can use to replicate the issue locally, as I can't confirm the problem here.

To reproduce the problem check this

[pfmoore]

OK, thanks - I found it (I think).

There's a prepare_linked_requirements_more function in operations.prepare, which looks at req.needs_more_preparation. If I'm understanding things here, it looks like that code path isn't clearing this flag when the "more preparation" has been done. But that logic is incredibly convoluted and I frankly can't follow it.

[pfmoore]

FWIW, the "needs more preparation" flag was added in #8685 but the code was updated for PEP 658 support in #11111.

@uranusjr @pradyunsg you both seem to have reviewed #11111 - do either of you have any recollection what was going on there?

[uranusjr]

I didn’t pay much notice to needs_more_preparation when I reviewed the PR since the code is already there and seems to do something else, but now looking back I guess yeah that flag should not be set when only the metadata is downloaded, since PEP 658 uses a completely different mechanism than the partial download approach and should not “resume” a wheel download. So I guess we can

1. Change the needs_more_preparation resuming message a bit, to say something like Resuming link ... instead so it’s at least clearer to the user this is a continuation to the previous download, and not a separate download.
2. Modify the code to make a metadata-only download not set needs_more_preparation at all.

[uranusjr]

One additional comment, we probably should rename that flag to same something like has_partially_downloaded_artifact instead to better reflect what it does.

[pfmoore]

Phew! I think I found the issue. We weren't recording in the _downloaded dictionary the fact that we downloaded and unpacked the URL in _complete_partial_requirements, which means that the following call to _prepare_linked_requirement downloaded the URL again.

I have a fix in #12120, waiting for the tests to run to see if it actually resolves the issue (it did in my manual tests, but I'm not promising yet that it doesn't break something else!)

[pfmoore]

Test failures seem to be for the case where we're given metadata for source distributions. I'm inclined to downgrade the "release blocker" status if that's the case, but I don't know how we fix the test suite, so I don't have a practical way to achieve that at the moment 🙁

@uranusjr Your comments on making the code more readable make sense (although the issue doesn't seem directly related to needs_more_preparation any more) but I definitely won't be doing anything about them right now - all I have time for is the bare minimum of trying to get things functioning¹. If anyone wants to look at this code any deeper, they'll need to file a separate PR (and it should be for 23.3, not 23.2, as the only thing I want to focus on for 23.2 is getting this issue dealt with).

Footnotes

1. And I'm not even sure I'll manage that - we may end up in a situation where I have to choose between releasing with the performance issue or reverting Fix parsing of JSON index dist-info-metadata values #12078 and losing PEP 658/714 support for another release cycle. ↩

[pfmoore]

By limiting the "double download" fix to wheels, the tests pass. That's an awful hack, but it gets us past the "release blocker" problem, as PyPI isn't yet serving PEP 658 metadata for sdists.

Fixing the sdist issue looks like it will require a complete refactoring of the logic that means we need the method prepare_linked_requirements_more, and I have no idea how to even start on that. So I'm going to limit the fix to wheels, and leave this issue open for someone else to look at the sdist issue.

The risk, of course, is that no-one will look at this issue for sdists, and we'll just end up with the same problem when PyPI does start serving sdist metadata. Maybe it'll be less of an issue as sdists typically aren't huge, but I'm not confident of that (huge data files, or people shipping Javascript runtimes in the sdist, could still be an issue). To be clear I don't personally have any intention of digging into the sdist issue any further - I don't have the time to work out what this code does and produce a full fix.

Edit: The largest sdist I can see in a recent PyPI dump is 520Mb in size. The mean sdist size is about 800Kb.

Another option would be to leave the sdist tests failing, but mark them as xfail. I'm not convinced that's a better option, as it doesn't make it any more likely that someone will look at the problem (possibly less so, because there wouldn't be a huge comment in the source explaining how the current situation is a gross hack 🙂)

[pfmoore]

I've merged my fix, and removed the release blocker label. I'm leaving the 23.2 milestone in case someone else contributes a full fix, but if no-one gets to it, I'll just remove the milestone (I'm leaving it mainly as a reminder to check back on this issue before the release).

[pfmoore]

I'm going to remove the milestone at this point, as I don't expect any further work on this before 23.2 (even if someone comes up with a PR, I won't have time to review it and I won't approve it for 23.2 as we've had enough problems and I won't risk any more being caused by rushing things in).

We can look at any follow-up in future pip releases.