Skip to content

separate model card git push from the rest #13514

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 14, 2021
Merged

separate model card git push from the rest #13514

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adb38ca
separate model card git push from the rest
Sep 10, 2021
73ce112
Update src/transformers/trainer.py
elishowk Sep 11, 2021
9c20f81
catch only EnvironmentError in push_to_hub
Sep 14, 2021
5255d1c
point documentation on model card schema to the hub's
Sep 14, 2021
3eabf1d
Merge branch 'master' into fix-trainer-modelcard
Sep 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/transformers/trainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2544,7 +2544,6 @@ def push_to_hub(self, commit_message: Optional[str] = "add model", **kwargs) ->
model_name = Path(self.args.output_dir).name
else:
model_name = self.args.hub_model_id.split("/")[-1]
self.create_model_card(model_name=model_name, **kwargs)
# Needs to be executed on all processes for TPU training, but will only save on the processed determined by
# self.args.should_save.
self.save_model()
Expand All @@ -2553,7 +2552,16 @@ def push_to_hub(self, commit_message: Optional[str] = "add model", **kwargs) ->
if not self.is_world_process_zero():
return

return self.repo.push_to_hub(commit_message=commit_message)
git_head_commit_url = self.repo.push_to_hub(commit_message=commit_message)
# push separately the model card to be independant from the rest of the model
if self.args.should_save:
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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Author

Choose a reason for hiding this comment

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

And without forgetting fixing model card metadata generation like with this issue #13528

Copy link
Collaborator

Choose a reason for hiding this comment

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

@LysandreJik, any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@elishowk elishowk Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think logging at the error level is fine. Can you remove the PR from draft and mrge it?

Copy link
Author

@elishowk elishowk Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@elishowk elishowk Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I resolved the conflict by adding blocking=blocking. Is it okay for you ?

logger.error(f"Error pushing update to the model card. Please read logs and retry. ${exc}")

return git_head_commit_url

#
# Deprecated code
Expand Down