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.

Sign up for GitHub

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

Jump to bottom

Responses from https://download.pytorch.org/whl/cpu have cache-control: no-cache set in their headers #130571

Open
aabtop opened this issue Jul 11, 2024 · 6 comments · May be fixed by pytorch/test-infra#6188
Open

Responses from https://download.pytorch.org/whl/cpu have cache-control: no-cache set in their headers #130571

aabtop opened this issue Jul 11, 2024 · 6 comments · May be fixed by pytorch/test-infra#6188
Assignees
@atalman
Labels
module: binaries Anything related to official binaries that we release to users triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@aabtop
Copy link

aabtop commented Jul 11, 2024 •
edited
Loading

🐛 Describe the bug

Not sure if this is intentional, but it makes it difficult for the responses to play well with caching, e.g. this prompted the astral-sh/uv#4967 where it's unclear how to resolve.

FWIW,

% curl -I https://download.pytorch.org/whl/cpu | grep cache-control
cache-control: no-cache,no-store,must-revalidate

and

% curl -I https://pypi.org/simple/ | grep cache-control
cache-control: max-age=600, public

In this particular instance this is manifesting as a problem for me because requests to https://download.pytorch.org/whl/cpu sometimes sporadically fail (e.g. with 503), so being able to reliably cache it would mitigate this considerably.

Versions

Fairly certain this issue is independent of my runtime and reproducible anywhere.

cc @ezyang @gchanan @zou3519 @kadeng @msaroufim @seemethere @malfet @osalpekar @atalman
@malfet malfet added triage review module: binaries Anything related to official binaries that we release to users high priority labels Jul 11, 2024
@BurntSushi
Copy link

BurntSushi commented Jul 12, 2024 •
edited
Loading

As the person who implemented uv's HTTP caching semantics, I think ideally no-cache and no-store would be removed here, but must-revalidate is okay. And of course, copying what PyPI does would be fine too.

@atalman
Copy link
Contributor

atalman commented Jul 15, 2024 •
edited
Loading

These settings are currently set per object for our s3 files using Metadata section:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html?icmpid=docs_amazons3_console#object-metadata.

These settings apply only to index.html files, not whl or tgz files.
Index.html files are updated constantly by this job: https://github.com/pytorch/test-infra/actions/workflows/update-s3-html.yml.
I think setting cache-control: max-age=600, public is a good idea we should probably go for it. @aabtop Do you have any examples of 503 failures that you mentioned in the issue description ?

@albanD albanD added triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module and removed high priority triage review labels Jul 15, 2024
@albanD
Copy link
Collaborator

albanD commented Jul 15, 2024

Lowering priority because it's only for the index.
Still open question how that matches to PyPi.

@malfet
Copy link
Contributor

malfet commented Jul 15, 2024

I think it's reasonable to cache index for 10 min or so

@atalman atalman self-assigned this Jul 16, 2024
@top-oai
Copy link

top-oai commented Jul 18, 2024

@aabtop Do you have any examples of 503 failures that you mentioned in the issue description ?

Sorry for the delay. I'm going back through the links I recorded and actually am having a hard time finding 503 failures, but it's hard to reproduce especially since some auto-retries we do now make it so that this doesn't get flagged anymore. I remember seeing one of them and got it in my head that that was the most common and so put that in the "(e.g. with 503)" statement, but it looks like 500 errors were more common for me. Seems like this is getting into separate issue (from this caching issue) territory, but here's an example:

error: HTTP status server error (500 Internal Server Error) for url (https://download.pytorch.org/whl/cpu/poetry-core/)

which is actually a bit strange since I don't see poetry-core in the index. In any case, I think it might be fine to leave that 500 alone for now since it's a bit funny, and I'll make a separate issue for it if I can isolate it better.

edmorley added a commit to edmorley/test-infra that referenced this issue Jan 18, 2025
@edmorley
Adjust index page Cache-Control headers
37a778d 
This changes the `Cache-Control` value for index pages from:
`no-cache,no-store,must-revalidate`

...to:
`max-age=600, public`

In order to allow the pages to be cached for up to 10 minutes.

The new value was chosen so that it matches that returned for
PyPI index pages:

```
$ curl -sSI https://pypi.org/simple/ | rg cache-control
cache-control: max-age=600, public
```

For explanations of the directives, see:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#response_directives

Fixes #pytorch/pytorch#130571.
@edmorley edmorley linked a pull request Jan 18, 2025 that will close this issue
Open
@edmorley
Copy link

I think setting cache-control: max-age=600, public is a good idea we should probably go for it.

@atalman I've opened pytorch/test-infra#6188 for this :-)

edmorley added a commit to edmorley/test-infra that referenced this issue Feb 3, 2025
@edmorley
Adjust index page Cache-Control headers
59d6334 
This changes the `Cache-Control` value for index pages from:
`no-cache,no-store,must-revalidate`

...to:
`max-age=600, public`

In order to allow the pages to be cached for up to 10 minutes.

The new value was chosen so that it matches that returned for
PyPI index pages:

```
$ curl -sSI https://pypi.org/simple/ | rg cache-control
cache-control: max-age=600, public
```

For explanations of the directives, see:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#response_directives

Fixes #pytorch/pytorch#130571.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@atalman atalman

Labels
module: binaries Anything related to official binaries that we release to users triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adjust index page Cache-Control headers edmorley/test-infra
7 participants
@BurntSushi @edmorley @malfet @albanD @aabtop @atalman @top-oai