separate model card git push from the rest #13514
Conversation
elishowk
added
model card
src/transformers/trainer.py
Outdated
| self.create_model_card(model_name=model_name, **kwargs) | ||
| try: | ||
| self.repo.push_to_hub(commit_message="update model card README.md") | ||
| except Exception as exc: |
There was a problem hiding this comment.
Not sure whether it's best to catch the error and log it, or just let it be raised naturally. I don't have a strong opinion on this so let's see what others think!
There was a problem hiding this comment.
Yes me neither IDK, it's an ergonomy issue because at this stage, the training has been pushed, so the question is what should we do with a faulty Readme ?
There was a problem hiding this comment.
And without forgetting fixing model card metadata generation like with this issue #13528
There was a problem hiding this comment.
@LysandreJik, any thoughts?
There was a problem hiding this comment.
If this happens mid-training, I would advocate for a very visible warning that the model card is incorrect, rather than erroring out. However, if it's possible to generate the model card before the training/evaluation starts (I understand some values will be invalid such as evaluation results) and identify a potential failure there, then we could error out.
Definitely fine to just log the error though.
PS: Shouldn't the exception caught be a bit more specific than Exception?
There was a problem hiding this comment.
About the warnings on git push : git push warnings are now logged as a logger.warning cf. https://github.com/huggingface/huggingface_hub/pull/326/files
As explained the docs https://huggingface.co/transformers/main_classes/trainer.html?highlight=trainer#logging, the only possibility (AFAIK) where a user could not see the warning is if her/his script using transformers sets the logLevel to error only. Elsewhere, the user gets the same warning as huggingface-cli users. Am i wrong ?
There was a problem hiding this comment.
I think logging at the error level is fine. Can you remove the PR from draft and mrge it?
There was a problem hiding this comment.
Done, please note that I mixed (may be I'm wring) a commit to update to the docs, because huggingface/model_card is gonna be deprecated soon I think
If not its place here, I can remove it
There was a problem hiding this comment.
I resolved the conflict by adding blocking=blocking. Is it okay for you ?
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
|
For tracking purposes, do we have another issue for |
|
🥳 first PR on transformers, thanks for your help you all ! |
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
What does this PR do?
After model card metadata contents validation was deployed to the Hub, we need to ensure transformer's trainer git push are not blocked because of an invalid README.mld yaml.
as discussed with @julien-c @Pierrci @sgugger and @LysandreJik the first step to match Hub's model card validation system is to avoid failing a whole git push after training, for the only reason that README.md metadata is not valid.
therefore, I tried in this PR to git push the training result independently from the modelcard update, so that the modelcard update failing does not fail the rest, keeping only logging for README.Md push failures.
Relates to display git push warnings huggingface_hub#326
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.