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

Support PEP 658/714 #1457

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Support PEP 658/714 #1457

wants to merge 8 commits into from

Conversation

thejcannon
Copy link

This change is meant to have the PyTorch package index support PEP 658 (and PEP 714) by accomplishing two things:

  • index.html uses both PEP 658/714 tags to denote it supports metadata hosting
  • The METADATA file for any wheel is now uploaded to S3 alongside the wheels in all 3 upload.sh scripts (likely needs to be tested thoroughly

Future improvements would be:

  • Including checksums for the metadata
  • Backporting this change to older wheels (by extracting the METADATA and uploading it to S3)

@facebook-github-bot
Copy link
Contributor

Hi @thejcannon!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

s3_management/manage.py Outdated Show resolved Hide resolved
@malfet
Copy link
Contributor

malfet commented Sep 23, 2023

@thejcannon considering that access to the bucket is public, do you mind sharing the output of this script somewhere (I can upload few .metadata files for existing wheels if you want) or point me to similar html file somewhere?

Also, do you mind rebasing

@vit-zikmund
Copy link

Hi @thejcannon, I'm sure you're taking time to contemplate after @matteius won the PR race (just joking, it was no race!), but all seems like now it's your time to shine!

As the hashing logic is now in place, it shouldn't be too much work to include the hashes for metadata, too, right? I'd love to help, if your free time is scarce these days, but don't want to, you know, start any new race.

@thejcannon
Copy link
Author

Sorry, for the late response. I'm still very much planning on picking this up, but 2 weeks ago I was on vacation, and then paid the vacation price for it (COVID), so my "return to work" has been slow-going.

@thejcannon
Copy link
Author

Alright, I'm back on this today. Thanks for everyone's patience.

@malfet I rebased. I probably should've asked as soon as I saw the message for you to upload some metadata files 🙈

@thejcannon
Copy link
Author

OK @malfet free free to upload .metadata files whenever to kick the tires on this thing.

As far as existing examples go, if you look at https://pypi.org/simple/requests/ and grep for core-metadata you'll see

<a href="https://files.pythonhosted.org/packages/70/8e/0e2d847013cb52cd35b38c009bb167a1a26b2ce6cd6965bf26b47bc0bf44/requests-2.31.0-py3-none-any.whl#sha256=58cd2187c01e70e6e26505bca751777aa9f2ee0b7f4300988b709f44e013003f" data-requires-python=">=3.7" data-dist-info-metadata="sha256=7823e890e9db6f415138badf9744791290ef76e7ec6fd09a3789e8247fffe782" data-core-metadata="sha256=7823e890e9db6f415138badf9744791290ef76e7ec6fd09a3789e8247fffe782">requests-2.31.0-py3-none-any.whl</a>

Note that PEP 658 says you can set the attribute to true to say "yes I have a metadata file", or set it to the sha256 of the metadata to allow the client to cache even harder. Right now the PR sets true, but let me see if we can throw the hash in to be even better.

@thejcannon
Copy link
Author

Alrighty, everything's been updated since the rebase, and now we set the attribute's value to the sha256 of the metadata file.

Should be good to test it out. 🎉

@vit-zikmund
Copy link

Hi @thejcannon, I see the Meta crew has a lot to work on sideways 😒
This could buy us/you some time to throw in this little monkey wrench - PEP 714, though. It's very PEP658 related. I actually feel it's so tied to it, it might not even make sense to support just the 568... Look for yourself, it's a real treat 🤕

@thejcannon
Copy link
Author

IIRC my change just supports both for maximum compatibility. I'll take a closer look this week

@vit-zikmund
Copy link

vit-zikmund commented Dec 6, 2023

You're right! My bad, sorry. You even got in the OP...
Maybe it might be worth updating the PR title, since the change supports the hashes now.

@thejcannon thejcannon changed the title Support PEP 658, without hashes Support PEP 658/714 Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants