Refactor CLI implementation using Typer #3365
Conversation
| def _api() -> HfApi: | ||
| return HfApi() |
There was a problem hiding this comment.
not really relevant, let's do a whoami() call directly
| dir: Optional[str] = typer.Option( | ||
| None, | ||
| "--dir", | ||
| help="Cache directory to scan (defaults to Hugging Face cache).", | ||
| ), |
There was a problem hiding this comment.
| dir: Optional[str] = typer.Option( | |
| None, | |
| "--dir", | |
| help="Cache directory to scan (defaults to Hugging Face cache).", | |
| ), | |
| dir: Annotated[Optional[str], typer.Option(help="Cache directory to scan (defaults to Hugging Face cache).")] = None |
from what I understand in https://typer.tiangolo.com/tutorial/options/help/, the recommended way to use Typer is to define options with an Annotated annotation.
Also, providing --dir for the dir argument is redundant (and therefore can be avoided)
(same comment on all commands)
| help="Increase verbosity (-v, -vv, -vvv).", | ||
| ), | ||
| ) -> None: | ||
| _run_scan(cache_dir=dir, verbosity=verbose) | ||
|
|
||
|
|
||
| def _run_scan(cache_dir: Optional[str], verbosity: int) -> None: | ||
| try: | ||
| t0 = time.time() | ||
| hf_cache_info = scan_cache_dir(cache_dir) | ||
| t1 = time.time() | ||
| except CacheNotFound as exc: | ||
| cache_dir = exc.cache_dir |
There was a problem hiding this comment.
I would avoid defining a new internal alias here. A good thing with Typer is that we shouldn't need to define twice some arguments type annotations (and therefore reduce risks of conflicts)
| help="Increase verbosity (-v, -vv, -vvv).", | |
| ), | |
| ) -> None: | |
| _run_scan(cache_dir=dir, verbosity=verbose) | |
| def _run_scan(cache_dir: Optional[str], verbosity: int) -> None: | |
| try: | |
| t0 = time.time() | |
| hf_cache_info = scan_cache_dir(cache_dir) | |
| t1 = time.time() | |
| except CacheNotFound as exc: | |
| cache_dir = exc.cache_dir | |
| help="Increase verbosity (-v, -vv, -vvv).", | |
| ), | |
| ) -> None: | |
| try: | |
| t0 = time.time() | |
| hf_cache_info = scan_cache_dir(cache_dir) | |
| t1 = time.time() | |
| except CacheNotFound as exc: | |
| cache_dir = exc.cache_dir |
| @@ -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.
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.
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)
Resolves #3363 and #3364.
This PR migrates the
hfCLI from built-inargparsetoTyperusing idiomatic, function based commands. this significantly reduces boilerplate, making the CLI easier to maintain, less error prone, and faster to extend with newHfApifeatures.What changed
new dependencies:
typer, latest version is 0.17.4, it should be stable enough and it's widely adopted.richinstall or not.Main entrypoint:
cli/hf.pynow uses typer.Typer and mounts:download,upload,upload-large-folder,env,version(all defined in their respective files).auth,cache,repo(withtagsubgroup),repo-files,jobs(withscheduledanduvsubgroups).lfs-enable-largefiles,lfs-multipart-upload.tests/test_cli.pyhas been updated accordingly. cf the documentation to write tests for typer cli: https://typer.tiangolo.com/tutorial/testing/#about-the-appcommand-decorator.