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

Add PyPI cache to test runs #5726

Closed
wants to merge 1 commit into from
Closed

Add PyPI cache to test runs #5726

wants to merge 1 commit into from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Aug 2, 2024

Part of #5713

@zanieb zanieb added the testing Internal testing of behavior label Aug 2, 2024
@zanieb

This comment was marked as outdated.

@@ -166,6 +180,7 @@ jobs:

- name: "Cargo test"
run: |
export UV_INDEX_URL="http://localhost:80/simple"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export UV_INDEX_URL="http://localhost:80/simple"
export UV_INDEX_URL="http://localhost/simple"

Shouldn't be a need to specify the port in this case

@blueraft
Copy link
Contributor

blueraft commented Aug 6, 2024

Can I help here?

@zanieb
Copy link
Member Author

zanieb commented Aug 6, 2024

Feel free yeah. Though I'm not sure how we'll handle the test changes.

@zanieb
Copy link
Member Author

zanieb commented Aug 6, 2024

I presume using ALL_PROXY instead of UV_INDEX_URL will help, but not sure how to make https behave.

@blueraft
Copy link
Contributor

blueraft commented Aug 6, 2024

Though I'm not sure how we'll handle the test changes.

I was thinking of just filtering pypi/localhost url to [PYPI_SERVER].

Running into SSL errors locally.

It works for me locally

@zanieb
Copy link
Member Author

zanieb commented Aug 6, 2024

I don't think that'll be sufficient, but it may be worth a try.

I run into SSL errors when using HTTPS_PROXY, not when using the index URL.

@zanieb
Copy link
Member Author

zanieb commented Aug 6, 2024

Before putting a lot of effort into snapshot correctness, you should probably see if this actually helps performance on the tests that do pass.

@blueraft
Copy link
Contributor

blueraft commented Aug 6, 2024

❯ hyperfine -r 3 'cargo test -p uv --test pip_compile' 'UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile'
Benchmark 1: cargo test -p uv --test pip_compile
  Time (mean ± σ):     29.200 s ±  1.987 s    [User: 76.855 s, System: 25.140 s]
  Range (min … max):   27.788 s … 31.472 s    3 runs

Benchmark 2: UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile
  Time (mean ± σ):     23.838 s ±  0.740 s    [User: 77.656 s, System: 27.261 s]
  Range (min … max):   23.085 s … 24.566 s    3 runs

Summary
  UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile ran
    1.22 ± 0.09 times faster than cargo test -p uv --test pip_compile

What do you think?

@zanieb
Copy link
Member Author

zanieb commented Aug 6, 2024

I was hoping for much more than that, though it's still nice. I wonder where we're spending all of our time in that case?

@blueraft
Copy link
Contributor

blueraft commented Aug 6, 2024

The git dependencies seem to be the slowest to me, which could be cached too but that'd require some changes in the nginx server.

@samypr100
Copy link
Collaborator

❯ hyperfine -r 3 'cargo test -p uv --test pip_compile' 'UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile'
Benchmark 1: cargo test -p uv --test pip_compile
  Time (mean ± σ):     29.200 s ±  1.987 s    [User: 76.855 s, System: 25.140 s]
  Range (min … max):   27.788 s … 31.472 s    3 runs

Benchmark 2: UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile
  Time (mean ± σ):     23.838 s ±  0.740 s    [User: 77.656 s, System: 27.261 s]
  Range (min … max):   23.085 s … 24.566 s    3 runs

Summary
  UV_INDEX_URL=http://localhost/simple cargo test -p uv --test pip_compile ran
    1.22 ± 0.09 times faster than cargo test -p uv --test pip_compile

What do you think?

Curious, was this after the requests got cached in nginx? (all nginx hits)

@samypr100
Copy link
Collaborator

I don't think that'll be sufficient, but it may be worth a try.

I run into SSL errors when using HTTPS_PROXY, not when using the index URL.

Might be better to use an actual http proxy (e.g. squid) if we want to leverage HTTPS_PROXY / HTTP_PROXY env vars, although I haven't tried it. Will try to do some local tests sometime to see if it's any better than the nginx cache solution.

@blueraft
Copy link
Contributor

blueraft commented Aug 7, 2024

Curious, was this after the requests got cached in nginx? (all nginx hits)

Yes tried it a few times.

git dependencies seem to be the slowest to me

Local caching didn't help too much here.

 hyperfine 'uv pip install "flask @ git+http://localhost:1234/github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall' 'uv pip install "flask @ git+https://github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall'
Benchmark 1: uv pip install "flask @ git+http://localhost:1234/github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall
  Time (mean ± σ):      2.904 s ±  0.084 s    [User: 0.866 s, System: 0.411 s]
  Range (min … max):    2.804 s …  3.034 s    10 runs

Benchmark 2: uv pip install "flask @ git+https://github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall
  Time (mean ± σ):      3.327 s ±  0.183 s    [User: 1.300 s, System: 0.581 s]
  Range (min … max):    3.148 s …  3.647 s    10 runs

Summary
  uv pip install "flask @ git+http://localhost:1234/github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall ran
    1.15 ± 0.07 times faster than uv pip install "flask @ git+https://github.com/pallets/flask.git@735a4701d6d5e848241e7d7535db898efb62d400" --no-cache --reinstall

@zanieb
Copy link
Member Author

zanieb commented Aug 7, 2024

FWIW I tried to setup squid today and it's non-trivial because you still need to have it support SSL, which requires a self-signed cert etc. Annoyingly the image at ubuntu/squid doesn't support SSL so you need a custom build.

@zanieb
Copy link
Member Author

zanieb commented Aug 7, 2024

And for posterity, I tried using Caddy and Nginx but both are not really designed for caching a forward proxy.

@samypr100
Copy link
Collaborator

samypr100 commented Aug 8, 2024

Agreed, SSL support is still non-trivial in this situation for transparent proxy cases. We'd need an action that generates a set of certs for you and make them trusted in every job.

@zanieb
Copy link
Member Author

zanieb commented Aug 8, 2024

I used this one-liner for a self-signed cert

openssl req -x509 -out localhost.crt -keyout localhost.key \
  -newkey rsa:2048 -nodes -sha256 \
  -subj '/CN=localhost' -extensions EXT -config <( \
   printf "[dn]\nCN=localhost\n[req]\ndistinguished_name = dn\n[EXT]\nsubjectAltName=DNS:localhost\nkeyUsage=digitalSignature\nextendedKeyUsage=serverAuth")

@samypr100
Copy link
Collaborator

samypr100 commented Aug 8, 2024

I used this one-liner for a self-signed cert

openssl req -x509 -out localhost.crt -keyout localhost.key \
  -newkey rsa:2048 -nodes -sha256 \
  -subj '/CN=localhost' -extensions EXT -config <( \
   printf "[dn]\nCN=localhost\n[req]\ndistinguished_name = dn\n[EXT]\nsubjectAltName=DNS:localhost\nkeyUsage=digitalSignature\nextendedKeyUsage=serverAuth")

Makes sense, I use mkcert these days which makes a Root CA for you as well and helps make specific certs for specific hosts (including client certs). This then allows you do transparent proxying with SSL interception as needed. At least locally it's great, CI maybe not.

@zanieb zanieb added the help wanted Contribution especially encouraged label Aug 8, 2024
@@ -166,6 +180,7 @@ jobs:

- name: "Cargo test"
run: |
export UV_INDEX_URL="http://localhost:80/simple"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export UV_INDEX_URL="http://localhost:80/simple"
export UV_INDEX_URL="http://127.0.0.1:80/simple"

https://www.youtube.com/watch?v=98SYTvNw1kw

and with @samypr100 suggestion

Suggested change
export UV_INDEX_URL="http://localhost:80/simple"
export UV_INDEX_URL="http://127.0.0.1/simple"

@@ -144,6 +144,14 @@ jobs:
if: ${{ github.repository == 'astral-sh/uv' && (needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main') }}
runs-on:
labels: "ubuntu-latest-large"
services:
pypyi-cache:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pypyi-cache:
pypi-cache:

Probably a typo?

@eth3lbert
Copy link
Contributor

Is disabling ssl verification an option?

@zanieb
Copy link
Member Author

zanieb commented Aug 13, 2024

Does that switch to using HTTP instead of HTTPS? I sort of don't think so, it just ignores bad certificates.

@eth3lbert
Copy link
Contributor

eth3lbert commented Aug 13, 2024

Does that switch to using HTTP instead of HTTPS? I sort of don't think so, it just ignores bad certificates.

I primarily focus on the git dependencies. Here are a couple of options I've been exploring lately:

  1. Prepare a local repository pool and fetch from file:// (requires URL replacement in uv-git).
    • Since we use git fetch instead of git clone, we cannot leverage the --reference-if-able option.
  2. Prepare a local repository pool, host it locally with a lb and git-http-backend, then configure git with something similar to this:
[url "https://git.example.com/"]
insteadOf = https://github.com/
sslVerify = false

This way, we don't need to make any further changes, and requests from github.com should be redirected to git.example.com transparently in the background. (You can even use url "file:///path-to-pool" if that's acceptable.)

  1. Similar to option 2, but configure with sslCAInfo = cert file instead of sslVerify = false.

Note: I haven't yet tested options 2 and 3 with authentication-related scenarios. More research is needed.
Additional note: The git clone command doesn't respect url rewrite rules.

@zanieb
Copy link
Member Author

zanieb commented Oct 1, 2024

This continues to look quite challenging, though I think it would be very valuable I don't think there's a clear path forward for this pull request.

@zanieb zanieb closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contribution especially encouraged testing Internal testing of behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants