-
Notifications
You must be signed in to change notification settings - Fork 653
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
Xet upload workflow #2887
Xet upload workflow #2887
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
…hub into xet-upload-workflow
…hub into xet-upload-workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few missing doc strings, but otherwise looks great!
…hub into xet-upload-workflow
…hub into xet-upload-workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! 🔥
Implementation looks good to me. I just have a question about when Xet is enabled but user is not xet authorized. Wondering when could that happen and what the expected behavior here (my understanding is that currently it fails to upload without any workaround possible).
If the server returns malformed responses or if the user is unauthorized to upload to xet storage. | ||
[`HTTPError`](https://requests.readthedocs.io/en/latest/api/#requests.HTTPError) | ||
If the LFS batch endpoint returned an HTTP error. | ||
**How it works:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**How it works:** | |
**How it works:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that the whole section would be better suited for https://github.com/huggingface/huggingface_hub/pull/2899/files#diff-b1fa48a1bcb46429d1a7d311268539b7fe814e4158608cce9c14d5d60a9c4a51 i.e. in official docs rather than the docstring. And keep things simple in the docstring with a "check out this guide to learn more about how it works".
Can be done in a later PR or in the docs PR directly (let's not block this one just for documentation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, actually i put it there mainly for users who will read the code since we have a "blackbox" call to hf_xet.upload_files
(same for downloading). but yes I agree it's better to have it in a documentation and then link it in the docstring. will do that in a following PR 👍
I think if xet is enabled in the repo and |
Ah yes, I forgot the workaround in this case would be to uninstall hf_xet if really they are not authorized to use it. But shouldn't happen except if doing something unexpected. Then it's all fine to raise the unauthorized error :) |
…ace_hub into xet-upload-workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
Co-authored-by: Lucain <lucain@huggingface.co>
…ace_hub into xet-upload-workflow
Let's wait for a final review from @bpronan and/or @rajatarya and then I think we should be good to merge! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 💯 correct. The user would only see this case (xetEnabled on repo + Uninstalling |
…hub into xet-upload-workflow
I think we can merge this one to the |
Partially resolves #2713
This PR contains only the Xet upload workflow implemented in xetpoc_huggingface_hub (internal).
The main branch for xet storage integration is xet-integration.
Main changes:
Note: since it's a common part for download and upload, this has been pushed directly into the xet-integration branch.
to try it in from this branch: