-
Notifications
You must be signed in to change notification settings - Fork 800
Refactor CLI implementation using Typer #3365
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
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.
Very excited about this switch! I left some very high level comments on code organization :)
setup.py
Outdated
@@ -28,6 +28,7 @@ def get_version() -> str: | |||
|
|||
extras["cli"] = [ | |||
"InquirerPy==0.3.4", # Note: installs `prompt-toolkit` in the background | |||
"typer", |
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.
I would move typer
to a required dependency otherwise doing pip install huggingface_hub
would install a CLI but unusable (since typer would be missing).
To reduce dependency overhead, especially the rich
package, let's add typer-slim
which is a lighter version without rich / shellingham included. More details here: https://typer.tiangolo.com/?h=typer+slim#typer-slim
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.
About shellingham
I still think it's good to have it. Either by default or in the (soon-to-come?) "hf installer script". It's a pure-python package that helps detecting the current shell to install shell completion correctly.
(for now let's skip it but mentioning it to have it in mind)
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.
agree! it's an oversight from me, fixed in 8ee6892
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 promising assuming commands implementations have not been changed^^
(I did not review the tests yet)
) -> None: | ||
logging.set_verbosity_info() | ||
repo_type = validate_repo_type(repo_type) | ||
api = HfApi(token=token, library_name="hf") |
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.
That's actually a really good idea to track usage of our CLI. I think it'd be good to centralize this (some other HfApi
instantiations are not defining the library_name) but can be done in a later PR. (we have a subticket about analytics specifically)
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.
api = HfApi(token=token, library_name="hf") | |
api = HfApi(token=token, library_name="hf", library_version=__version__) |
might be nice to add huggingface_hub
version as well (would make things easier to works with in Kibana even though we already have the info in hf_hub
user agent)
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.
for CLI, i addressed it directly in this PR (in f2eb79b) but yes let's do the same for other HfApi instantiations
Co-authored-by: Lucain <[email protected]>
…hub into typer-migration
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. |
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.
Pre-approving ✔️ Feel free to ignore my last comments if annoying 😄
Let's get this merge soon! 🎉
(note: I also tested autocompletion locally which worked like a charm)
src/huggingface_hub/cli/hf.py
Outdated
# top level single commands (defined in their respective files) | ||
app.command( | ||
name="download", | ||
help="Download files from the Hub.", | ||
)(download) | ||
app.command( | ||
name="upload", | ||
help="Upload a file or a folder to the Hub.", | ||
)(upload) | ||
app.command( | ||
name="upload-large-folder", | ||
help="Upload a large folder to the Hub. Recommended for resumable uploads.", | ||
)(upload_large_folder) | ||
app.command( | ||
name="env", | ||
help="Print information about the environment.", | ||
)(env) | ||
app.command( | ||
name="version", | ||
help="Print information about the hf version.", | ||
)(version) | ||
app.command( | ||
name="lfs-enable-largefiles", | ||
help="Configure your repository to enable upload of files > 5GB.", | ||
hidden=True, | ||
)(lfs_enable_largefiles) | ||
app.command( | ||
name="lfs-multipart-upload", | ||
help="Upload large files to the Hub.", | ||
hidden=True, | ||
)(lfs_multipart_upload) | ||
|
||
|
||
# command groups | ||
app.add_typer(auth_cli, name="auth") | ||
app.add_typer(cache_cli, name="cache") | ||
app.add_typer(repo_cli, name="repo") | ||
app.add_typer(repo_files_cli, name="repo-files") | ||
app.add_typer(jobs_cli, name="jobs") |
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.
Fine for me yes! Thanks for looking into this :)
Let's get this merged once the CI is green! 🔥 |
* Refactor CLI implementation using Typer (#3365) * migrate CLI to typer * (#3364) disable rich in all cases * update tests * make typer-slim a required dep * use Annotated * fix linting issues * fix tests * refactoring * update docs * use built in types * fix mypy * call whoami directly * lint * Apply suggestions from code review Co-authored-by: Lucain <[email protected]> * import Annotated from typing * Use Enums * set verbosity globally * refactor scan cache and update version docstring * centralize where Typer is defined * no need for ... * rename enum * no need for extra param name * docstring * revert * centralize arguments and options definition * add library version when initializing HfApi * add auto-completion * sort commands alphabetically * suggestions * centralize jobs params and HfApi initialization * fix --------- Co-authored-by: Lucain <[email protected]> * update docs --------- Co-authored-by: Lucain <[email protected]>
Resolves #3363 and #3364.
This PR migrates the
hf
CLI from built-inargparse
toTyper
using idiomatic, function based commands. this significantly reduces boilerplate, making the CLI easier to maintain, less error prone, and faster to extend with newHfApi
features.What changed
new dependencies:
typer
, latest version is 0.17.4, it should be stable enough and it's widely adopted.rich
installed or not.Main entrypoint:
cli/hf.py
now uses typer.Typer and mounts:download
,upload
,upload-large-folder
,env
,version
(all defined in their respective files).auth
,cache
,repo
(withtag
subgroup),repo-files
,jobs
(withscheduled
anduv
subgroups).lfs-enable-largefiles
,lfs-multipart-upload
.tests/test_cli.py
has been updated accordingly. cf the documentation to write tests for typer cli: https://typer.tiangolo.com/tutorial/testing/#about-the-appcommand-decorator.