-
Notifications
You must be signed in to change notification settings - Fork 80
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 Huggingface Integration #916
Conversation
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
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.
Before I start reviewing this in earnest, I would need at least the following 2 pieces of information to be added to the PR:
- Documentation: (it is absolutely fine to have a bullet point list of items that link to the main HF docs)
- Tests
I believe both of these were present in the previous PR.
Hi @Wauplin and @NielsRogge - this PR looks good from my end. Do you have any feedback? |
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.
Hey there 👋 Thanks for the ping!
I left a few comments from an outsider point of view. I do think that the CLI should be more opinionated (understand "have less options and decide things for the user") otherwise we pretty much end up with a CLI close to what huggingface-cli upload
and huggingface-cli download
do.
Co-authored-by: Lucain <[email protected]>
@pranayasinghcsmpl some lint fixes (unused variables and whatnot) will be needed for this PR. Thanks for taking care of it! Thanks for your comments and suggestions, @Wauplin! |
Support ticket generated with Codacy to explore the coverage issue. |
Codacy folks suggested not to use coverage reporter for anything coming in from other forks 🙄 Anyway, we should be good to go from my end. @Wauplin is this PR good to merge for you? |
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.
Integration looks good yes :) I left a few comments but nothing blocking on my side.
Thanks for the iterations!
Co-authored-by: Lucain <[email protected]>
Co-authored-by: Lucain <[email protected]>
Co-authored-by: Lucain <[email protected]>
Co-authored-by: Lucain <[email protected]>
@Wauplin I think this PR is good to merge from my side. Could you please confirm and resolve your comments? I think the remaining parts can be taken up in separate issues/PRs. Thoughts? |
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.
Hi @pranayasinghcsmpl, thanks for the ping!
I think the logic looks good now :) I'll let you resolve the comment you want (most of them are already resolved, right?) and merge this PR 🤗
Thank you all for all the help and feedback during this effort! 😄 |
Fixes #727
Proposed Changes
Checklist
CONTRIBUTING
guide has been followed.typing
is used to provide type hints, including and not limited to usingOptional
if a variable has a pre-defined value).pip install
step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].