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

[Feature] Support tag management (create, list, delete) in CLI #2172

Merged
merged 20 commits into from
Apr 9, 2024

Conversation

bilgehanertan
Copy link
Contributor

Resolves #2141

Since I wasn't sure whether to add unit tests about functionalities directly, I have only added parsing tests. Depending on the reviews, I can add tests using the token from constants.

I was also unsure how to handle the token argument tag.py:120; please verify whether this is the proper way to do so.

@Wauplin

@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.

Comment on lines 18 to 27
Usage Examples:
- Create a tag:
$ huggingface-cli tag create my-model 1.0 --tag-message "First release"
$ huggingface-cli tag create my-model 1.0 --tag-message "First release" --revision develop
$ huggingface-cli tag create my-model 1.0 --tag-message "First release" --type dataset
- List all tags:
$ huggingface-cli tag list my-model
- Delete a tag:
$ huggingface-cli tag delete my-model 1.0
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @bilgehanertan, thanks for the PR! I haven't thoroughly reviewed it but implementation-wise the structure looks good. Before moving forward, I'd like to suggest some changes in the CLI format. Here is how I'd envision it:

# Tag model
$ huggingface-cli tag username/my-model 1.0
# Tag model with message
$ huggingface-cli tag username/my-model 1.0 -m "First release"
$ huggingface-cli tag username/my-model 1.0 --message "First release"
# Tag model with revision
$ huggingface-cli tag username/my-model 1.0 --revision develop
# Tag dataset
$ huggingface-cli tag username/my-dataset 1.0 --repo-type dataset

# List all model tags
$ huggingface-cli tag -l username/my-model
$ huggingface-cli tag --list username/my-model
# List all dataset tags
$ huggingface-cli tag -l username/my-dataset --repo-type dataset

# Delete model tag
$ huggingface-cli tag -d username/my-model 1.0
$ huggingface-cli tag --delete username/my-model 1.0

Main changes are:

  • removed create/delete/list commands in favor of -l/--list and -d/--delete to list and delete tags. By default, tag creation is expected.
  • updated --type to --repo-type for consistency
  • I think namespace of the repo should always be explicit.
  • updated --tag-message to --message or -m

With this format, we would be much closer of the git tag command line that is already well established. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Wauplin, thanks for the feedback!
I have adjusted it & tested it; it should look better now.

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 30.00000% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 82.27%. Comparing base (d9150c8) to head (784f4d5).
Report is 10 commits behind head on main.

❗ Current head 784f4d5 differs from pull request most recent head 9609dc3. Consider uploading reports for the commit 9609dc3 to get more accurate results

Files Patch % Lines
src/huggingface_hub/commands/tag.py 30.50% 82 Missing ⚠️
src/huggingface_hub/commands/huggingface_cli.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2172      +/-   ##
==========================================
- Coverage   82.93%   82.27%   -0.66%     
==========================================
  Files         103      104       +1     
  Lines        9703     9823     +120     
==========================================
+ Hits         8047     8082      +35     
- Misses       1656     1741      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Wauplin Wauplin 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 the changes @bilgehanertan! API looks good now. Left some comment to simplify a bit + review some messages but overall looks very promising :) Would you mind doing a last iteration on it? Thanks in advance!

src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
src/huggingface_hub/commands/tag.py Outdated Show resolved Hide resolved
@bilgehanertan
Copy link
Contributor Author

Thanks for the changes @bilgehanertan! API looks good now. Left some comment to simplify a bit + review some messages but overall looks very promising :) Would you mind doing a last iteration on it? Thanks in advance!

@Wauplin all done!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks @bilgehanertan! I played with it locally and it's 🔥!! I pushed a few tweaks for consistency with the CLI. PR looks good to me logic-wise now. Before merging, would you mind documenting this new command on this page: https://huggingface.co/docs/huggingface_hub/guides/cli?

You'll need to update this file to document huggingface-cli tag. Section can go between huggingface-cli delete-cache and huggingface-cli env. Usually in the doc guide with put "real examples". If you are looking for repos with tags on the Hub, on can cite the bigcode/the-stack dataset or the Wauplin/gradio-space-ci Space. Thanks in advance!

@bilgehanertan
Copy link
Contributor Author

Thanks @bilgehanertan! I played with it locally and it's 🔥!! I pushed a few tweaks for consistency with the CLI. PR looks good to me logic-wise now. Before merging, would you mind documenting this new command on this page: https://huggingface.co/docs/huggingface_hub/guides/cli?

You'll need to update this file to document huggingface-cli tag. Section can go between huggingface-cli delete-cache and huggingface-cli env. Usually in the doc guide with put "real examples". If you are looking for repos with tags on the Hub, on can cite the bigcode/the-stack dataset or the Wauplin/gradio-space-ci Space. Thanks in advance!

@Wauplin I have added the necessary guides, hope it looks good!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Nice! That looks great! Thanks a lot for the reactivity ❤️

docs/source/en/guides/cli.md Outdated Show resolved Hide resolved
docs/source/en/guides/cli.md Outdated Show resolved Hide resolved
docs/source/en/guides/cli.md Outdated Show resolved Hide resolved
docs/source/en/guides/cli.md Outdated Show resolved Hide resolved
docs/source/en/guides/cli.md Outdated Show resolved Hide resolved
docs/source/en/guides/cli.md Show resolved Hide resolved
Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

PR's clean and is ready to go I think! Let's wait for the CI to pass and I'll merge. Nice API, nice docs, nice tests, thanks a lot for your contribution and iterations @bilgehanertan! ❤️

@Wauplin Wauplin merged commit 39e8ebc into huggingface:main Apr 9, 2024
14 checks passed
@Wauplin Wauplin changed the title CLI Tag Functionality [Feature] Support tag management (create, list, delete) in CLI Apr 9, 2024
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.

[CLI] Support tag management (create, list, delete)
4 participants