-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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 push_to_hub to no_trainer examples #13659
Conversation
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.
Looks good to me, thanks for adding this!
this is great! |
else: | ||
repo_name = args.hub_model_id | ||
repo = Repository(args.output_dir, clone_from=repo_name) | ||
elif args.output_dir is not None: |
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.
Can this arg be None? Wouldn't this lead to a failure later?
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.
Also we assume in the if above that args.output_dir
is not None already no in:
repo_name = get_full_repo_name(Path(args.output_dir).name, token=args.hub_token)
=> maybe raise Error before the repo creation if args.output_dir
is None?
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.
This is asserted here already :-)
if accelerator.is_main_process: | ||
if args.push_to_hub: | ||
if args.hub_model_id is None: | ||
repo_name = get_full_repo_name(Path(args.output_dir).name, token=args.hub_token) |
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.
Does the args.hub_token
always have to be passed or is the locally saved fetched token taken in case args.hub_token
is None?
Also is a nice error message thrown when the token is incorrect? Since push_to_hub
now defaults to True - I think it'd be important to provide a nice error message and maybe also how to disable saving if wished
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.
Does the args.hub_token always have to be passed or is the locally saved fetched token taken in case args.hub_token is None?
Yes, it uses the local token if none is passed.
Note that the default to push_to_hub
is not True, a user has to pass it. I'll add in the README that they should execute huggingface-cli login
before running the script. Will also look at the error message and open an issue on huggingface_hub
if it's not clear enough.
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.
Looks good! Left some comments on how to maybe improve the user experience by catching errors early. Think we can raise an Error early if args.output_dir
is not defined no?
Also, since |
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.
Fantastic, thanks @sgugger! Good usage of the Repository
object :)
@patil-suraj The requirements |
* Add push_to_hub to no_trainer examples * Quality * Document integration * Roll out to other examples
* Add push_to_hub to no_trainer examples * Quality * Document integration * Roll out to other examples
* Add push_to_hub to no_trainer examples * Quality * Document integration * Roll out to other examples
What does this PR do?
This PR adds the
--push_to_hub
flag to theno_trainer
examples in the Transformers repository. For now, it only deals withrun_glue_no_trainer.py
but once everyone has commented, the final version will be duplicated on the other examples.