-
Notifications
You must be signed in to change notification settings - Fork 613
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 additional repo card utils from modelcards
repo
#940
Conversation
The documentation is not available anymore as the PR was closed or merged. |
This PR is ready for a first round of reviews. The docstrings may be out of date a bit (which is why I'll leave this as draft), but the tests are all in. The example colab in my message above shows the usage. I don't want to add anything in docs for now in case there are big changes from review I need to make. |
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.
Thanks for your contribution @nateraw, excited to see it land in hfh
.
I've added a few high-level comments so that it complies with the rest of the codebase for it to be merged. I didn't dive in the code to try it out (yet).
Thanks for providing a notebook to play around with; could you also add this to the doc, perhaps in its own file? Having it be documented alongside some nice code examples like you have in the notebook will make it even easier to play with.
You can add a new file here: https://github.com/huggingface/huggingface_hub/tree/main/docs/source
And link it from the _toctree.yml
file which is in that same folder.
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.
Played with it a little bit, I really like it!
This looks to be in good shape.
Aside from small comments, I wonder why you didn't keep the notion of ModelCard
inheriting from RepoCard
(which applies to all repo types) which i quite liked in https://github.com/nateraw/modelcards ?
And same for CardData
, IMO we should have a repo-type-agnostic implem of CardData
and then children dataclasses that define the relevant fields
My other question was, do we really want to bundle the (or a?) model card template in the library, or should we host it on the Hub and load the template from there too.
I think there might be advantages and drawbacks to both approaches.
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.
Thanks for this PR! It's very cool! 🔥 I left some questions and high-level suggestions, but this is in a great state already!
- First of all, thanks for sharing the colab! It makes it super easy and nice to play with the API very quickly. I think a lot of it can be converted to some nice docs 🦾
- I agree with @julien-c on having different classes for each type of repo card, with
ModelCard
,SpacesCard
, etc. - I think having the main template bundled in the library is nice, so I would keep as is. If people want to provide new templated in the future we could have some additional functionality to use templates from the Hub, which might motivate people to create their own templates, but I think for now focusing in the current one is better!
- I didn't review the tests yet as they are probably going to change a bit :)
Thanks again! 🚀
* Preserve newlines in the existing card files * make style * ✅ add test * 💄 apply style Co-authored-by: nateraw <[email protected]>
* Preserve newlines in the existing card files * make style * ✅ add test * 💄 apply style Co-authored-by: nateraw <[email protected]>
c3bf9bd
to
a26c958
Compare
Ok folks, I've:
|
Codecov Report
@@ Coverage Diff @@
## main #940 +/- ##
==========================================
+ Coverage 82.18% 82.80% +0.62%
==========================================
Files 35 35
Lines 3593 3752 +159
==========================================
+ Hits 2953 3107 +154
- Misses 640 645 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Very cool PR! This is very very nice, and the documentation for it is very helpful!
Co-authored-by: Omar Sanseviero <[email protected]> Co-authored-by: Lucain <[email protected]>
Co-authored-by: Lucain <[email protected]> Co-authored-by: Omar Sanseviero <[email protected]>
Co-authored-by: Lucain <[email protected]>
3ea9acf
to
8ea02af
Compare
Ok folks, I'm ready to call this good to go. Any small updates to docs/etc can be done in future small PRs, as long as there's nothing major lingering that anybody is super opinionated about. Feel free to give this a final look over. @Wauplin, as a sanity check, can you please double check I didn't mess anything up when I rebased? one of my most recent commits ended up showing me codecov twice, causing tests to fail for some reason, but it resolved itself when I pushed to this branch again. Should be fine, just would like another pair of eyes on it. Thank 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.
Great work!!
@nateraw I double-checked everything and I think we are good to go. |
This PR aims to add repo card utilities built out in nateraw/modelcards to this repo. Note that there are some lingering issues we may want to resolve and update both there and here before merging.
Some todos:
modelcards
to this reposetup.py
Latest walkthrough of this PR in Colab:![Open In Colab](https://camo.githubusercontent.com/96889048f8a9014fdeba2a891f97150c6aac6e723f5190236b10215a97ed41f3/68747470733a2f2f636f6c61622e72657365617263682e676f6f676c652e636f6d2f6173736574732f636f6c61622d62616467652e737667)