feat: add --sort arg to delete-cache to sort by size
#2815
Conversation
|
x-linking #1065 |
There was a problem hiding this comment.
Hi @AlpinDale thank you for this very clean PR! I appreciate the code quality and having tests already implemented 🤗 I've left a high-level comment about the CLI interface. I'd prefer something like this:
huggingface-cli delete-cache --sort=size
Let me know what you think !
|
Thanks for the review, @Wauplin. I've added the requested changes, including the alphabetical, lastUpdated, lastUsed sorting options. Please take a look again. |
There was a problem hiding this comment.
Thank you @AlpinDale ! Didn't expect that much but it makes the PR really great! :) I've left some minor comments to address.
| @@ -18,6 +18,7 @@ | |||
| huggingface-cli delete-cache | |||
| huggingface-cli delete-cache --disable-tui | |||
| huggingface-cli delete-cache --dir ~/.cache/huggingface/hub | |||
| huggingface-cli delete-cache --sort | |||
There was a problem hiding this comment.
| huggingface-cli delete-cache --sort | |
| huggingface-cli delete-cache --sort=size |
(to showcase how to use it)
| "--sort", | ||
| nargs="?", | ||
| choices=["size", "alphabetical", "lastUpdated", "lastUsed"], | ||
| const="size", |
There was a problem hiding this comment.
| const="size", |
By default, let's not sort
| delete_cache_parser.set_defaults(func=DeleteCacheCommand) | ||
|
|
||
| def __init__(self, args: Namespace) -> None: | ||
| self.cache_dir: Optional[str] = args.dir | ||
| self.disable_tui: bool = args.disable_tui | ||
| self.sort_by: Optional[str] = args.sort |
There was a problem hiding this comment.
Can you define a SortingOption_T as the beginning of the module?
SortingOption_T = Literal["size", "alphabetical", "lastUpdated", "lastUsed"]and then use it throughout the module like this?
| self.sort_by: Optional[str] = args.sort | |
| self.sort_by: Optional[SortingOption_T] = args.sort |
| for repo in sorted(hf_cache_info.repos, key=_repo_sorting_order): | ||
| sorted_repos = sorted( | ||
| hf_cache_info.repos, | ||
| key=lambda repo: -repo.size_on_disk if sort_by == "size" else 1, |
There was a problem hiding this comment.
Can you move the sorting logic defined above in the TUI part to a separate method so that it can be reused here? So that the "no tui" path can also support all sorting options.
This PR adds the
--sortflag to thedelete-cachecommand.This should allow users to sort the repos by size, including all revisions. The default mode is in descending order, but this behaviour can be changed by passing theascendingargument to the flag.The flag takes 4 arguments:
size,alphabetical,lastUpdated, andlastUsed. Thesizeis currently in descending order.