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

Add revision to trainer push_to_hub #33482

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

teamclouday
Copy link
Contributor

What does this PR do?

Add revision to trainer's push_to_hub method. See #33481

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Copy link
Contributor

@not-lain not-lain left a comment

Choose a reason for hiding this comment

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

Thanks for adding this

@teamclouday
Copy link
Contributor Author

Thank you for the suggestions! Just made a new commit

@not-lain
Copy link
Contributor

perfect, the failing tests are unrelated to this PR.
cc @SunMarc for review

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @teamclouday ! Did you test the PR ?

@teamclouday
Copy link
Contributor Author

Thanks for adding this @teamclouday ! Did you test the PR ?

I just added test code! Can run it with:

export HUGGINGFACE_CO_STAGING=1; python -m unittest -v tests.trainer.test_trainer.TrainerIntegrationWithHubTester.test_push_to_hub_with_revision

Currently with revision, the code is assuming that branch already exists. Wonder if it's necessary to automatically create new branch if not found.

@not-lain
Copy link
Contributor

Don't forget to format your files with ruff

@teamclouday
Copy link
Contributor Author

Don't forget to format your files with ruff

Thanks for reminder! Just applied the format

@not-lain
Copy link
Contributor

the setup_and_quality tests are still failing
This can be fixed by organizing the imports and you should be all good.

@teamclouday
Copy link
Contributor Author

Just fixed the imports! Should be good now. Sorry about the back and forth

@@ -4474,6 +4475,8 @@ def push_to_hub(
Whether the function should return only when the `git push` has finished.
token (`str`, *optional*, defaults to `None`):
Token with write permission to overwrite Trainer's original args.
revision (`str`, *optional*):
The git revision to commit from. Defaults to the head of the "main" branch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it the branch to commit to - rather than the commit to commit from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this description comes from the hub upload_folder api. I was only going to use it for branches and tested with that, so not sure if it works for other commit hashes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK!

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for adding!

@SunMarc
Copy link
Member

SunMarc commented Sep 17, 2024

Can you rebase on main to make the CI green @teamclouday ?

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@teamclouday
Copy link
Contributor Author

I just rebased to main and CI is green now! Thank you all!

@SunMarc SunMarc merged commit 6c051b4 into huggingface:main Sep 17, 2024
23 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* add revision to trainer push_to_hub

* apply suggestions

* add test for revision

* apply ruff format

* reorganize imports

* change test trainer path
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.

5 participants