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

install pip and uv in separate pip calls #3127

Closed
wants to merge 55 commits into from

Conversation

graingert
Copy link
Member

@graingert graingert commented Nov 2, 2024

[investigating pypy pip cache issues]

There's not much point installing a pinned version of pip if we don't
use that pinned version to get uv
Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (9d6e134) to head (5b03326).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3127      +/-   ##
==========================================
+ Coverage   99.62%   99.68%   +0.06%     
==========================================
  Files         122      122              
  Lines       18340    21901    +3561     
  Branches     1222     1744     +522     
==========================================
+ Hits        18272    21833    +3561     
+ Misses         47       46       -1     
- Partials       21       22       +1     

see 32 files with indirect coverage changes

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Oh I think someone deleted the cache or something. Things work now. Here's a list of what I was going to try (to narrow down things):

  • check the mtimes of the cached files
  • check whether the metadata file seems to match the body file

I'll try them if this happens again.

@A5rocks A5rocks closed this Nov 9, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Nevermind I got fooled. Somehow it's failing elsewhere -- does it take the current actions YAML as input??? Weird.

@A5rocks A5rocks reopened this Nov 9, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Uh huh:

-rw-r--r-- 1 runneradmin 197121   1819 Nov  9 06:29 c:/users/runneradmin/appdata/local/pip/cache/http-v2/a/d/e/e/f/adeef5b9611702687bbddade5f7c0af65110caf98ba1a0070c9d116e
-rw-r--r-- 1 runneradmin 197121 228583 Oct 28 14:08 c:/users/runneradmin/appdata/local/pip/cache/http-v2/a/d/e/e/f/adeef5b9611702687bbddade5f7c0af65110caf98ba1a0070c9d116e.body
-rw-r--r-- 1 runneradmin 197121 238531 Nov  9 06:29 c:/users/runneradmin/appdata/local/pip/cache/http-v2/a/d/e/e/f/adeef5b9611702687bbddade5f7c0af65110caf98ba1a0070c9d116e.body4f_dkhwx.tmp
-rw-r--r-- 1 runneradmin 197121 232580 Nov  1 01:08 c:/users/runneradmin/appdata/local/pip/cache/http-v2/a/d/e/e/f/adeef5b9611702687bbddade5f7c0af65110caf98ba1a0070c9d116e.body9k42hbh3.tmp
-rw-r--r-- 1 runneradmin 197121 234523 Nov  5 22:53 c:/users/runneradmin/appdata/local/pip/cache/http-v2/a/d/e/e/f/adeef5b9611702687bbddade5f7c0af65110caf98ba1a0070c9d116e.bodyd4lemxfj.tmp

that seems ominous

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

OK so my hypothesis is that:

  • in the normal case, pip takes the metadata (which is up to date), sends the request based on it (which I assume includes stuff like if-modified-since or whatever), and then pypi is like "this hasn't been modified since then". pip then takes the cached body (which isn't up to date) and then uses that.
  • in the updating case, somehow something prevents pip from writing to the body file?

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

in the updating case, somehow something prevents pip from writing to the body file?

this doesn't make sense. it's not permissions, yet it happened 3 times in a row. I'll try to find the actions run where it happened (based on timestamps)

... seems like it was https://github.com/python-trio/trio/actions/runs/11693817436 but that just doesn't make sense??? The cache key is different!!! oh huh:

Failed to save: Unable to reserve cache with key setup-python-Windows-x64-python-7.3.17-3.10.14-pip-b032f14d7258df1128974c12e4eb403092367f05c038b849b7fb93f1e0c286df, another job may be creating this cache. More details: Cache already exists. Scope: refs/heads/main, Key: setup-python-Windows-x64-python-7.3.17-3.10.14-pip-b032f14d7258df1128974c12e4eb403092367f05c038b849b7fb93f1e0c286df, Version: bc106ed8a87958c409aa9dd4068a396930fed3f1639ff0f947daec52d5984691

still different cache key but that's an interesting error. though thinking about it, that's irrelevant because the other pypy run managed to save its cache.

ultimately this is because of suppressing OSError in writes: https://github.com/pypa/pip/blob/420435903ff2fc694d6950a47b896427ecaed78f/src/pip/_internal/network/cache.py#L74

that doesn't make sense. pip should change that.

@CoolCat467
Copy link
Member

Could this theoretically be issues stemming from a hash collision? I know the likelyhood of that is astronomically low, but putting that idea out there.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Could this theoretically be issues stemming from a hash collision? I know the likelyhood of that is astronomically low, but putting that idea out there.

I don't think so. But not having suppressed the OS errors would help diagnosing things further :^)

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Just realized https://github.com/python-trio/trio/actions/runs/11754054520/job/32747608845 is another actions run that failed to rename the tmpfile pip makes, and it has the verbose flags. But the logs look like you would expect:

  Updating cache with response from "https://pypi.org/simple/uv/"
  etag object cached for 1209600 seconds
  Caching due to etag

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

I'm like 90% certain (I'm trying to prove this to myself right now) that the issue is that somewhere in pip they open the body file. Then they don't close it. Then this is fine on CPython because that gets deleted when it falls out of scope, but on PyPy the opened file is still open when pip tries to rename the tmpfile to go over it.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

I'm certain of my hypothesis: putting a gc.collect() before the rename fixes everything. I'm just unsure where the body file is leaking from (I mean I know it comes from a method called get_body() and then from _load_from_cache() but IDK where the bodyfile gets stored). I tried deleting the thing that holds it where I think it's responsible but that didn't help.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

Alright, so the "fix" specifically in this one case is that for https://github.com/pypa/pip/blob/420435903ff2fc694d6950a47b896427ecaed78f/src/pip/_vendor/cachecontrol/controller.py#L290, instead of just returning the headers, the file stored in resp needs to be closed.

For some reason, del resp doesn't work -- but resp._fp.close() works. This prevents the OSError.

@graingert
Copy link
Member Author

you should be able to use with resp: ...

@A5rocks
Copy link
Contributor

A5rocks commented Nov 9, 2024

you should be able to use with resp: ...

Seems to work! Other than the fact that resp is potentially None so (in actual code) need to check it first.

@A5rocks
Copy link
Contributor

A5rocks commented Nov 18, 2024

I think we figured this out.

@A5rocks A5rocks closed this Nov 18, 2024
@CoolCat467 CoolCat467 deleted the investigate-pypy-pip-cache-issues branch November 18, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants