Skip to content
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

[Dataset | Model card] When pushing to template repos, work on actual raw contents #1282

Merged
merged 8 commits into from
Jan 16, 2023

Conversation

julien-c
Copy link
Member

@julien-c julien-c commented Dec 26, 2022

Question: do we actually want this feature or not?

Internal Slack convo
cc @Wauplin

Details

Generated commit for model cards (would need to do the same for datasets): https://huggingface.co/templates/model-card-example/commit/901deccf5acead553f9b082aca480d966e61f355

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 26, 2022

The documentation is not available anymore as the PR was closed or merged.

@Wauplin
Copy link
Contributor

Wauplin commented Dec 27, 2022

@julien-c I get the idea of the PR. Not sure what is the optimal way to add a comment to the modelcard. I'm fine with this solution if we don't have any better simple solution. My only concern is that if we have modelcards with a comment in the metadata section, the modelcards module is still unable to process them. So if a user uses the ModelCard feature of hfh at some point, it will remove the comment (is it really an issue though ?).
In any case, if we go with this solution, I would add a comment in push_repocard_example to explain shortly why we are not using the module.

@Wauplin
Copy link
Contributor

Wauplin commented Dec 27, 2022

About the failing tests, I don't know what the issue is. I encountered it as well on Friday 23rd afternoon but weren't sure if it was my fault or not. It seems that on staging we cannot commit a README.md file from the HTTP endpoint. It gets a HTTP 400 Bad request for commit endpoint: fetch failed from moon-landing. It's not about the content of the file but really the filename (I tried it manually and any other filename can be uploaded without any problem.). Maybe something to restart on staging ? cc @julien-c @Pierrci

@julien-c
Copy link
Member Author

maybe ping @SBrandeis about the staging env failure (no rush though)

@julien-c
Copy link
Member Author

Sleeping on this PR a bit, I think it's an okay approach. No big deal if the actual ModelCard strips comments in metadata (which is expected, I guess – YAML => objects conversion doesn't have a concept of comments)

BTW we need to do it for dataset cards too.

Pinging @nateraw for his opinion though, just to be sure

Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julien-c this looks great to me! One small issue I noticed here is that we are pushing the rendered template instead of the template itself, so there's not any clear indication in the pushed model card repo as to where it came from/how it was created.

Maybe we add a clear link somewhere? Either as a yaml comment or perhaps some header/footer within the card? Perhaps we just pass the model_summary jinja template variable when rendering

@@ -51,14 +55,22 @@ def push_model_card_example(overwrite: bool) -> None:
Card is pushed to https://huggingface.co/templates/model-card-example.
"""

card = ModelCard.from_template(ModelCardData())
template = jinja2.Template(ModelCard.default_template_path.read_text())
content = template.render(card_data="{}")
Copy link
Contributor

@nateraw nateraw Jan 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe pass model_summary var and include link to raw template along with perhaps some note that that's where the rendered template came from? https://raw.githubusercontent.com/huggingface/huggingface_hub/main/src/huggingface_hub/templates/modelcard_template.md

Copy link
Member Author

@julien-c julien-c Jan 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, WDYT @Wauplin ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to push directly to this branch @nateraw?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! I see you added a couple commits 11 mins ago - are you done? If so I can push to this branch directly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep i'm done!

@Wauplin
Copy link
Contributor

Wauplin commented Jan 16, 2023

@nateraw @julien-c I have added both model_summary and dataset_summary to explain from where the templates come from. Could you quickly review and we merge it ? :)

Copy link
Member Author

@julien-c julien-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't approve my PR but looks good to me! thx

@Wauplin Wauplin merged commit 8ff9aa5 into main Jan 16, 2023
@Wauplin Wauplin deleted the dataset-card-add-link-to-metadata-spec branch January 16, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants