-
Notifications
You must be signed in to change notification settings - Fork 53
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
DOC Autogeneration of getting started code #132
Conversation
Model card created from this branch, the code is automatically generated: https://huggingface.co/merve/test-code-generation |
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.
Overall LGTM.
One minor thing I wonder: Do we want to assume that users will always load the metadata from config or are there good reasons not to? If there are reasons not to, perhaps we should keep an example of manually adding the get_started_code
.
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@BenjaminBossan I think it looks a bit ugly and our example already includes metadata from config file so that's why I wanted to remove it (it will be a duplication if we add that too). |
Yes, I'm aware that in the example, it doesn't make sense. What I meant is to document it some other place so that users have a chance of discovering it (as is, the only way to discover the feature is by reading the template). If we can assume, however, that the vast majority of users create the section automatically, then maybe it's not necessary. |
@adrinjalali what do you suggest? |
I think since w/o the config file people won't really would know what to do with the model file, it's safe to assume the config file is there, especially for an example in the skops library, which generates that file. |
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.
otherwise LGTM
skops/card/_model_card.py
Outdated
f"import pickle\nwith open({model_file}, 'rb') as file:\n clf =" | ||
" pickle.load(file)" |
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 think we can also add a predict
example here, with the data available in the config file.
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.
Addressed. new version of card: https://huggingface.co/merve/test-code-generation
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, this LGTM. I'd suggest at the same time we also remove the <details>
tag around it, this is something I'd say should be visible to users.
@adrinjalali done, new model card: https://huggingface.co/merve/test-code-generation |
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.
That looks gorgeous, thanks for giving an example one on the Hug @merveenoyan , it really does help with the review.
This PR solves #126.
I also made a fixture out of metadata creation from config file so that it can be used repeatedly. Let me know if this is not good.