Provide file hashes in the URLs to avoid unnecessary file downloads (bandwidth saver)

[matteius]

Supply sha256 query parameters using boto3 to avoid hundreds of extra Gigabytes of downloads each day during pipenv and poetry resolution lock cycles.

Fixes point 1 in pytorch/pytorch#76557
Fixes #1347

[facebook-github-bot]

Hi @matteius!

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 [email protected]. Thanks!

[matteius]

I just signed the Contributor License Agreement,

[matteius]

@malfet Could I get this on your radar?

[matteius]

@seemethere or @atalman -- Could I get this PR on your radar?

[thejcannon]

FWIW I don't think this completely fixes pytorch/pytorch#76557, just the first item in the list.

Item 2, I think, very much should still be tackled (and I'm willing to give it a shot). Both of those together I think would be sufficient.

Edit: I have a branch now that has support for 2, however would love to get in contact with someone from the infra team since backporting and checksums (and backporting checksums) make this even better

[thejcannon]

Also, if I look at the support on PyPI I see URLs like this:

<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>

which makes me think we should make these links look like those (specifically using # in the URL)

[matteius]

@thejcannon I was only tackling the first item on the list -- which is going to be mutually beneficial for pipenv. I am not that familiar with PEP 658 and that should be pursued in a separate PR. In the case of pipenv, this change alone should allow us to go from downloading all of the matching distributions to just the one relevant to the system (at least I think it will). Checking on the # in the URL now ..

[thejcannon]

Gotcha. I started on the other bullet points and have a branch.

Two questions for you:

• I haven't used pipenv, can you help my understanding of why the hashes of wheels help? Is the assumption that the index could theoretically serve a different binary blob for a given package+version at some later point in time? Therefore without a hash pipenv (and others) re-download to be the most correct?
• How did you test the S3 checksum grabbing? When I issue a similar command (aws s3api get-object-attributes ...) I get denied permissions 😞

[matteius]

can you help my understanding of why the hashes of wheels help?

@thejcannon Yes, the issue is consider this lock file entry for pytorch:

        "torch": {
            "hashes": [
                "sha256:4b4bf40d844fdcf9b7261e62aff9eede7c89c2b904343b3f9dc12393763779cb",
                "sha256:566cbfad47b25ef6b1023518982179439d8e53069f3d2341e8adcd1e4b2ef179",
                "sha256:588188a37e2f2c9bb8f807e0d49ac4d66b9ef23f4417233247c5d2f796c4f42d",
                "sha256:78ae2260ac1902c5a224fe1b7994072bbfa7ef8a1734d8697ca74249fa6391c5",
                "sha256:7c23faff945e98c6e242ce41a480e7fe48426140d5994ea3b9886c02145d58cf",
                "sha256:bbe445308a43960efeabc4f4a4b6c5d0db209ef5d5148672c750943d1ea42f54",
                "sha256:bcb611272d7aa5c2f7c1648b2227303a023b1de46aabccf1846a8aca1bfb5cd8",
                "sha256:ccbd72f39d76fb7e7db6513bcc8e4ee684348f9865bd8cddca6ce2dd07b3ccd0"
            ],
            "index": "downloadpytorch",
            "version": "==1.10.2+cu111"
        },

To generate this proper lock file entry the lock phase had to download every single wheel to get the hash value from the registry, but with the above change (assuming the AWS ACL is correct and I didn't make a mistake) it will have the hashes and scrape them from the html page instead.

The hashes are used during the install phase to verify that the package downloaded matches what was determined when the lock file was was locked or last updated. So you can see where if each of those wheels is ~1.5 GB that is ~12 GB of downloads that would be avoided during lock phase. The idea is that all hashes related to that version are included because the lock file is shared across system platforms so the install phase may use different package+hash based on the system but everything in that list was authorized.

How did you test the S3 checksum grabbing?

I didn't really, I don't have anything in AWS but am familiar with using it and boto3, and wrote that code just from reading how to get the hash in documentation, but its possible I could have made a mistake:
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3/client/get_object_attributes.html

It says:

To use GetObjectAttributes, you must have READ access to the object.

[matteius]

Hmm, I guess those docs also say:

ChecksumSHA256 (string) –
The base64-encoded, 256-bit SHA-256 digest of the object. This will only be present if it was uploaded with the object.

That seems possibly troublesome.

[matteius]

Which links to:
https://docs.aws.amazon.com/AmazonS3/latest/userguide/checking-object-integrity.html

https://aws.amazon.com/blogs/aws/new-additional-checksum-algorithms-for-amazon-s3/

It seems that:
1.) The bucket needs to be configured to use the sha-256 checksum (md5 is default)
2.) Historical files probably wouldn't get this by default without being "copied" after the setting is changed, possibly also explicitly specifying the sha-256 algorithm in the copy or put_object commands.

[thejcannon]

Ah ok your explanation makes things crystal clear now, thank you.

---

I also reached out to some folks at Meta to help me with the S3 side of PEP 658, we can also see if checksum enabling (including historical files) is something their willing to enable.

[thejcannon]

FYI @matteius

• I think you should probably tag this PR as fixes Pip packaging and publishing improvements in pytorch wheels for better integration with poetry #1347 😛
• I got in contact with some folks @malfet @seemethere who are going to look into adding the checksums to the objects in S3 (currently the Checksum is not an attribute attached to the objects, so your S3 command will always fail)
• Depending on the timing of the merges and checksums, please consider updating this PR with support for metadata files as well: Support PEP 658/714 #1457 ❤️

[thejcannon]

While I'm around I thought I'd review the code 😋

Exciting stuff!

[thejcannon]

Why not just let this propogate? Otherwise it'll just repeatedly print the error and generate a sub-optimal index.

[matteius]

My thinking was that the index page should still generate as it does now even if there is no hash available to provide. I should definitely remove the print statements. I am not sure what exceptions would be raised before the Checksum values get added to the objects.

[thejcannon]

(Did this mean to get commented below? on https://github.com/pytorch/builder/pull/1433/files/b3799a8eb42b56118a4bc0658902875734888721#r1261472864? If not I'm not following the thread here. If the credentials aren't found we really can't know if there's a checksum or not)

[matteius]

I mean its the same code block in my mind -- I am just saying I don't know what exceptions we want to catch and handle for the case of the checksum isn't available yet -- maybe we don't care about NoCredentialsError but I think we need to handle something

[thejcannon]

I think it's safe to assume:

• The runner has valid credentials to the object
• The runner has the correct permissions to access the attributes
• S3 is alive and well and will return good JSON.

Because otherwise if these assumptions aren't held, there's a chance the script generates an index.html without the checksums and we're back to square 1.

So I think the only thing to handle here is "this object has no checksum attribute".

Which means the code looks roughly like:

response = CLIENT.get_object_attributes(
    Bucket=BUCKET,
    Key=s3_key,
    ObjectAttributes=["Checksum"]
)
checksum = response.get("Checksum", {}).get("ChecksumSHA256", None)

without any try/except.

FWIW I tested your code against a work bucket and got TypeError: expected string or bytes-like object. Switching the argument for Bucket to the string bucket name then gives me a valid response.

So probably use BUCKET.name as well.

---

Here's some local testing...

>>> import boto3
>>> BUCKET=S3.Bucket("<bucketname>")
>>> CLIENT = boto3.client("s3")
>>> response = CLIENT.get_object_attributes(Bucket=BUCKET.name,Key="<object-without-checksum>",ObjectAttributes=['Checksum'])
>>> list(response)
['ResponseMetadata', 'LastModified', 'VersionId']
>>> response.get("Checksum", {}).get("ChecksumSHA256", None)
>>> response = CLIENT.get_object_attributes(Bucket=BUCKET.name,Key="<object-WITH-checksum>",ObjectAttributes=['Checksum'])
>>> list(response)
['ResponseMetadata', 'LastModified', 'Checksum']
>>> response.get("Checksum", {}).get("ChecksumSHA256", None)
'7Uc3AtM1Dlc4XiQ8A6NkQdV9JMU4liPtz7wOd+RRCTE='

[thejcannon]

Resolved.

[thejcannon]

This is a wide exception. I think the try: block and relevant exceptions likely should be scoped to be explicit about what is expected to fail and what isn't.

Otherwise, a simple bug could manifest as an index not having hashes. When the alternative is an error with a stacktrace given to the user, and no index generated.

[thejcannon]

Resolved.

[thejcannon]

Is there any way to leverage the call in from_S3 to also include this information?

If not, perhaps this should be done there so the returned objects already have the checksum attached and the downstream code can choose to use it or not?

That would isolate the S3 retrieval code to be in one logical place.

[matteius]

Is there any way to leverage the call in from_S3 to also include this information?

From what I can tell they are different clients/endpoints that serve different purposes.

If not, perhaps this should be done there so the returned objects already have the checksum attached and the downstream code can choose to use it or not?

From my perspective, the from_S3 appears to just be tracking a list of strings which is the object s3_key with List[str] so I am not sure it makes sense to tackle a larger refactor just to add all the additional calls to pre-lookup these checksums and store them in memory until their point of use.

That would isolate the S3 retrieval code to be in one logical place.

Isn't it already isolated to this one file manage.py where the clients are in scope for the whole file?

Maybe I am being dense and don't see the obvious way to accomplish this without spending a couple more hours on it, plus the more complexity I add, well I have no real way of testing it. Looking for any guidance on the right exceptions to catch or not catch above, but I think we need to catch something to cover the case that s3 isn't ready or able to provide the hashes.

[thejcannon]

OK, so looks like you should be able to do this in from_s3 (which is ideal, because we'll want the checksums for metadata files in #1457 as well:

>>> import boto3
>>> BUCKET=S3.Bucket("<bucketname>")
>>> obj = next(iter(BUCKET.objects.filter(Prefix="<some prefix>")))
>>> response = obj.meta.client.head_object(Bucket=BUCKET.name, Key=obj.key, ChecksumMode="ENABLED")
>>> response["ChecksumSHA256"]
'7Uc3AtM1Dlc4XiQ8A6NkQdV9JMU4liPtz7wOd+RRCTE='

And then for an object without a checksum:

>>> response = obj.meta.client.head_object(Bucket=BUCKET.name, Key=obj.key, ChecksumMode="ENABLED")
>>> list(response)
['ResponseMetadata', 'AcceptRanges', 'LastModified', 'ContentLength', 'ETag', 'ContentType', 'ServerSideEncryption', 'Metadata']

[thejcannon]

Resolved.

[matteius]

Build is failing with:

Unknown parameter in input: "ChecksumMode", must be one of: Bucket, IfMatch, IfModifiedSince, IfNoneMatch, IfUnmodifiedSince, Key, Range, VersionId, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5, RequestPayer, PartNumber```

[matteius]

#1548

[vit-zikmund]

Tagging @malfet again, as these inner comments are notoriously hard to discover. Hope it's no big deal 😇

[thejcannon]

If you don't want to put more man-hours into this, I wouldn't mind picking this up to be done alongside/after #1457.

I want the same bandwidth savings for myself, my engineers, and everyone else. So I'm really eager to get this in! 😄

[matteius]

If you don't want to put more man-hours into this

I'd like to see it through, thanks for your guidance -- I'll push some more commits soon.

[thejcannon]

Bitchin' And thanks for your patience. Hopefully by the time the Meta folks stop by this'll be an easy approval 👍

[matteius]

@thejcannon This is how I would track the checksums, could you have a look and see if it makes sense to you too?

[thejcannon]

Looking really good. Likely the last wave of comments 👍

[thejcannon]

This still has objects as a list. I don't see it being converted to a dict anywhere?

I think you can just change it on line 343 to a dict and here to objects[sanitized_key] = sha256

[thejcannon]

The RHS should be str | None

Suggested change

	def __init__(self: S3IndexType, objects: Dict[str, str], prefix: str) -> None:
	def __init__(self: S3IndexType, objects: Dict[str, str | None], prefix: str) -> None:

[thejcannon]

I think there's a couple other places this should be done as well.

[thejcannon]

flake8-simplify (link) suggests just iterating over the dict instead of explicitly calling .keys(). We could talk about the likely very itty bitty performance bump, but lol.

Is there a reason you prefer .keys()?

[thejcannon]

The return type of this function needs updating

[matteius]

I believe this is fixed now.

[thejcannon]

Also the return type needs updating

[matteius]

If we could get some stats from Meta about the S3 cost savings (did some napkin math, its probably way wrong, but I estimate could be north of 200,000$ a month savings if 200,619*16*0.08 is a meaningful calculation (the average monthly downloads from pypi torch, but I assume the gpu specific indexes are more popular) -- anyway would be interesting to have insight into 😁

[vit-zikmund]

Hey @matteius & @thejcannon , I just wanted to say thanks for trying to push all this further. All I had time for so far was the analysis in #1347, I hope it was at least somewhat helpful 😄
While I'm not in any particular position for suggesting anything for the PR here, here's something that should hopefully make the change thinner and a bit better to manage.
While the obj[ect] in the code is just a str with a local path (or an s3 key), you could define a dataclass to represent the object with more attributes like this:

@dataclasses.dataclass(frozen=True)
class S3Object:
  key: str
  checksum: str | None

def __str__(self):
  return self.key

By overriding the __str__ method it gets to be printed as before. This might not be enough in some places and one will need to reach for the obj.key, but that shouldn't be a big hassle. This is extensible beyond just the checksum and one can keep all the previous sets & lists you switched for a dict. This dataclass might also bear a metadata: str | bool | None property, which could hold the metadata presence boolean (or better its checksum) for @thejcannon's PEP 658 PR.

[thejcannon]

Oh yeah. I LOVE me some dataclasses (no seriously). I think there's obviously some trepidation in refactoring code you don't-quite-own, so that kind of change is likely a good idea to have in a dedicated PR and perhaps encouraged to be merged before this or https://github.com/pytorch/builder/pull/1457/files#diff-b4c713cb4f974318a824c8d87db6e78eec6b4b9e129b3e5a83f4268bfb3320b0.

[vit-zikmund]

that kind of change is likely a good idea to have in a dedicated PR and perhaps encouraged to be merged before this or https://github.com/pytorch/builder/pull/1457/files#diff-b4c713cb4f974318a824c8d87db6e78eec6b4b9e129b3e5a83f4268bfb3320b0.

Thanks for your kind words @thejcannon. I don't want to hijack this thread more than I feel necessary (crossed that already), but with all due respect I don't see any reason in pushing such a standalone refactoring change in my own PR given there's been over a month of zero activity on #1347 after I tagged 2 owners (hopefully politely enough). I read you write you got in contact with them, so I hope there's some real collaboration going on behind what we see here. I never got that far and thus will crawl back to my lair and wait for you to play the big game to the successful end, wishing you (us) all the best.

Either way, you both guys have a partially conflicting & related PRs now, so if luck has it one will need to rebase & resolve one's PR on top of the other's. My dataclass suggestion that can tie these two together lies in the field to be picked up or not, no problem. Let's focus on getting anything that makes sense to work and stop all these agonizing downloads. Thank you! 🤞

[thejcannon]

I read you write you got in contact with them, so I hope there's some real collaboration going on behind what we see here. I never got that far and thus will crawl back to my lair and wait for you to play the big game to the successful end, wishing you (us) all the best.

I wouldn't be so quick to disparage yourself in a community of peers. I only got in contact with some folks by nagging a very nice Meta employee I met at PyCon earlier this year. Then they suggested we have the discussion(s) out in the open, so also nothing going on behind the scenes. As much excitement as we all have I'm sure the slow wheel of Enterprise is churning on, being methodical and deliberate. So now we just wait I suppose (until I feel the need to nag some more 😉 )

Your thorough comment on #1347 and your eye for detail above only proves your input in these discussions is valuable. So I would happily review (and outsiders-approve) a PR you put forth for your great idea. Then it'd just be a race to see who gets accepted first 😈

[matteius]

I took a shot at the refactor to use the dataclass.

[vit-zikmund]

Hey @matteius, I'm really honored you gave my suggestion a chance! 🙇 It looks great. The resulting diff is indeed smaller and better to read 😉

I've took the liberty to check the result and drop a few comments/suggestions, most of which try to make the new code occupy a tiny bit less lines.

Thank you once again!

[vit-zikmund]

Nice 👍 Now to get some real stakeholders to notice ⌛

[thejcannon]

@matteius congrats

@malfet wanna review the accompanying PR? #1457

[malfet]

@thejcannon first I need to fix all the bugs introduced by this change (first of them, is that str | None is not a valid type annotation), but sure, will try to review this one soon

[thejcannon]

from __future__ import annotations?

Also consider that these kinds of bugs are introduced by not having sufficient checks gate the PR