The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.
Hi @Freed-Wu , thanks for your PR. The example snippet in the description is very promising !
However, I have not yet been able to reproduce it. Did you do some steps from https://docs.iterative.ai/shtab/use/#cli-usage ?
What I tried is pip install ".[completion]"
. Should I then run something similar to
shtab --shell=zsh huggingface_hub.commands.huggingface_cli --error-unimportable \
| sudo tee /usr/local/share/zsh/site-functions/_shtab
to make it work ? Or should I copy-paste the output of huggingface-cli --print-completion zsh
somewhere ?
I am running Ubuntu 22.04 using zsh. Thanks in advance for your help 🙏
About the code review itself, seems good to me. I have a few comments but it can wait. Install will have to be documented somewhere :)
Sorry for late.
huggingface-cli --print-completion bash | sudo tee /usr/share/bash-completion/completions/huggingface-cli
huggingface-cli --print-completion zsh | sudo tee /usr/share/zsh/site-functions/_huggingface-cli
huggingface-cli --print-completion tcsh | sudo tee /etc/profile.d/huggingface-cli.csh
You can refer other projects using shtab https://github.com/andreafrancia/trash-cli#install-shell-completions
Ok nice thanks for the answer. In the end, I made it work by running
# Not under `/usr/share/zsh` but under ` /usr/local/share/zsh`
huggingface-cli --print-completion zsh | sudo tee /usr/local/share/zsh/site-functions/_huggingface-cli
Ok nice thanks for the answer. In the end, I made it work by running
# Not under `/usr/share/zsh` but under ` /usr/local/share/zsh` huggingface-cli --print-completion zsh | sudo tee /usr/local/share/zsh/site-functions/_huggingface-cli
Good. Now can you merge this branch? And how about huggingface/datasets#5269?
@Freed-Wu before merging, I've added a few comments on the code.
Also I would be good if it could be documented somewhere -at least in the CLI help itself-. My biggest concern comes from the additional line that needs to be run to configure autocomplete on the machine. I have a few questions about that:
shtab
but you have not configured it with your shell yet. Please refer to ... to configure it". Otherwise it will cause some friction for users that do pip install huggingface_hub[completion]
and expect completion to work out of the box.In the end, I am very much enthusiastic about the idea of having this autocompletion in the CLI. My main concern is about the maintenance/support that we would need to provide to our users to make it as smooth as possible for them.
And how about huggingface/datasets#5269?
About datasets
, I'll let @lhoestq answer. In general, let's settle this completely for huggingface_hub
and then think about other libraries.
OK, fixed.
Hi @Freed-Wu , thanks for making the changes ! What about the concerns about user experience (#1207 (review)) ? Do you have an opinion about it ?
detect from Python code if the autocompletion is set on the machine/in the shell
I don't know and I think it shouldn't be done by python, For example, bash user
should install bash-completion and
zsh user must autoload -Uz compinit && compinit
in their zshrc, it is hard to detect if user done this work by python.
Do you know if there is a way to programmatically setup the shell from Python code
As I know, it is usually done by package manager, such as
Other software distribution, such as
msys2,
termux,
nixpkgs,
homebrew
are same.
About datasets, I'll let @lhoestq answer.
I see. It depends on you.
@Freed-Wu So basically for now your suggestion would be to ship this PR "as it is" and leave the installation to users themselves ?
Most if not all of our users are installing huggingface_hub
via a pip
. Not sure that we want to become a CLI-first library as this would require more maintenance cost on our side. Typically, I don't want to start maintaining distributions of huggingface_hub
through nix, brew,...
What I wonder is how to notify users that a autocomplete feature is available if they install it properly. I think we should at least explain "somewhere" how to use the output of huggingface-cli --print-completion
as I don't expect users to know about it. Any ideas or reference to another library doing this ?
Also, would it be possible to have write-access to Freed-Wu:shtab
branch ? I'd like to push a commit to fix a few code quality things.
EDIT: could you just merge https://github.com/Freed-Wu/huggingface_hub/pull/1 please ?
would it be possible to have write-access to Freed-Wu:shtab branch ? I'd like to push a commit to fix a few code quality things.
You can
I don't want to start maintaining distributions of huggingface_hub through nix, brew,...
It is the duty of other software distributions.
Any ideas or reference to another library doing this ?
I don't known. :(
Base: 82.57% // Head: 83.98% // Increases project coverage by +1.41%
🎉
Coverage data is based on head (
a79c659
) compared to base (664cfdd
).
Patch coverage: 0.00% of modified lines in pull request are covered.
@@ Coverage Diff @@
## main #1207 +/- ##
==========================================
+ Coverage 82.57% 83.98% +1.41%
==========================================
Files 44 45 +1
Lines 4338 4353 +15
==========================================
+ Hits 3582 3656 +74
+ Misses 756 697 -59
Impacted Files | Coverage Δ | |
---|---|---|
src/huggingface_hub/commands/_shtab.py | 0.00% <0.00%> (ø) |
|
src/huggingface_hub/commands/huggingface_cli.py | 0.00% <0.00%> (ø) |
|
src/huggingface_hub/hf_api.py | 89.42% <0.00%> (+1.25%) |
⬆️ |
src/huggingface_hub/repository.py | 79.52% <0.00%> (+10.92%) |
⬆️ |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.
@Freed-Wu, yep indeed for the permissions on your branch. It was a config mistake on my side. It is now pushed.
About merging the PR, I'm still not a fan of the current state. For now we have:
--print-completion
option
shtab
is not installed, it suggests to do pip install huggingface_hub[completion]
shtab
is installed, it prints a long list of commands, without explaining what to do with ithuggingface-cli --print-completion
:
/usr/share/zsh/site-functions/_huggingface-cli
or /usr/local/share/zsh/site-functions/_huggingface-cli
?")huggingface_hub
library is updatedshtab
is installed--print-completion-help
command to explain a bit the procedure. Still not easy to know where to redirect the user. I have read the official documentation but the shtab --shell=zsh shtab.main.get_main_parser --error-unimportable
is not directly applicable in our case (even a shtab --shell=zsh -u huggingface_hub.commands.huggingface_cli.get_main_parser
wouldn't feel right if we had it).All of this feels a bit clunky to me. I'm not sure we want to push a feature that will only be easily usable by a few users knowing about it. Do you see my point ? 😕
I know your concern. To the best of my knowledge, a solution is perhaps write a Makefile, or a shell script and write some code in setup.py to let pip install XXX[completion]
to do this work. (about /usr/share/zsh or /usr/local/share/zsh, it is easy to detect which situation is right.) However, I think it is too complex. As I know, write a makefile for a python project is only used for those project containing C/C++ code, such as <github.com/pytorch/pytorch>, becuase they need compile these C code. For this project, I haven't seen any similar project to do this work. If you want to, you can write many code in setup.py like
if os.path.exists('/usr/share/zsh'):
subprocess.call(['XXX', '--print-completion'])
...
with open('/usr/share/zsh/site-functions/_huggingface-cli') as f:
f.write(context)
Like the above.
I think it is complex. However, it depends on you eventually.
Hi @Freed-Wu , thank you for your feedback and for all the time you invested in helping us on this PR.
However we decided internally that we will put this PR on hold for now. The proposed solution is great but comes with its drawbacks. As we are not yet sure which direction we want to give to huggingface-cli
(see discussion here), we prefer not to chose a tool that we might end up not using. For example we might want in the future to list repos or files from the Hub directly from the autocomplete, which is not yet possible with shtab
.
Thanks again for your help and I'll keep you posted when we move forward for the CLI 🙂
(EDIT: for reference, here is the slack discussion (internal link))
list repos or files from the Hub directly from the autocomplete
It can refer https://github.com/andreafrancia/trash-cli/blob/master/trashcli/shell_completion.py#L43.
trash-put filename
will put filename
to trash bin. --trash-dir
can allow
user to select trash bin, such as ~/.local/share/Trash
(by default) or other
/root_of_disk/.Trash-userid
.
trash-put --trash-dir <TAB>
can complete trash bin paths.
Its solution is to provide trash-list --trash-dirs
to provide trash bin paths.
we will put this PR on hold for now.
I have said that it depends on you 😄 About your concern (install shell completion by pip
automatically not by user manually, I'll report to shtab) And I advise fixing #1206, because it is not related to your concern.
I'll keep you posted
OK. Remember post me in the future 👍
Hey, thanks for the feedback! A few points:
shtab
into options.extras_require
. It's a tiny & pure Python package (no C-extensions/platform-dependencies), so should be in install_requires
(so pip install thing
is enough; no need to do pip install thing[completion]
& no need of this awkward code)thing --print-completion=bash
*.txt
example or even more advanced things)pip install thing
auto-detecting the shell(s) on the local machine and running the completion post-install setup: this is exceedingly complex, and setup varies a lot depending on platform and the user choices. For example, what happens if someone pip install
s into a throwaway conda
environment on a machine that has both zsh
and bash
? Some trickery with environment files? There's just too many edge cases.
Thanks @casperdcl for your input !
iterative/shtab#122 aka pip install thing auto-detecting the shell(s) on the local machine and running the completion post-install setup: this is exceedingly complex, and setup varies a lot depending on platform and the user choices. For example, what happens if someone pip installs into a throwaway conda environment on a machine that has both zsh and bash? Some trickery with environment files? There's just too many edge cases.
Yes that's actually what I'm the most afraid of as we don't want to implement/support/debug this type of edge cases.
For now I'll keep this as it is (e.g. no completion in huggingface_hub
). We will come back to your inputs when we decide to move forward on the CLI tools more seriously.
so pip install thing is enough; no need to do pip install thing[completion] & no need of this awkward code
Not defense -- I have a little concern about it. Do you hear much news which some evil developers inject evil code in their project like faker.js? I believe you, but not every one is same as me. For them, reducing the unnecessary dependencies is a good solution to avoid anything like faker.js happens again. The libraries which can let all one believe are only built-in libraries.
dynamic completions: possible via defining custom shell functions
Agree 👍
I agree you need to maintain your own docs (see e.g. https://dvc.org/doc/install/completion#what-shell-do-you-have) since you can't expect end-users to know what to do
I support writing docs. In any situation, a rich documentation is not a bad thing, absolutely. Although end-user may not read them. (Packagers who package software to apt, pacman, nix, homebrew must read docs).
auto-detecting the shell(s) on the local machine and running the completion post-install setup: this is exceedingly complex
I agree it is complex, however I don't think it is impossible. It is just very trivial to take many different edge situations into consideration.
For now I'll keep this as it is
Support keep current situation. A final solution is better than a current solution, which other current solutions are worse than. Certainly, if any better solution occur, I also support.
Another solution is just to publish the shell completions with package, man page and other things in GitHub release, then let packager or end-user download them directly, not generate by themselves. This is an example. The package script just downloads these files and install them to the correct path. Now python -m build
cannot generate these shell completions, which is trouble.
No defense again 😄
At the risk of starting a tangential discussion:
reducing the unnecessary dependencies is a good solution to avoid anything like [
left-pad
/colors
/faker
]
Completely disagree, sorry. Minimising attack surface is done by pinning dependencies, not marking them as optional.
done by pinning dependencies
PYPI seems not to allow user uploading package with same version to override the old package (Perhaps user can delete the old project then create a new project with same name to escape the limitation) so pin version can reduce the risk. (If not, pin the sha256 of the package). git tag is fragile by git tag 1.0.0 -f
. Perhaps many projects try to reduce the dependencies due to other reasons, not only reducing risk.
the risk of starting a tangential discussion
I am sorry that I open a tangential discussion. Now I stopped.
@Wauplin Sorry to disturb you, however, I found a solution to solve your following problem:
When doing huggingface-cli --print-completion:
Either the user knows what to do about it user still has to find where to put the config (for example: "Is it /usr/share/zsh/site-functions/_huggingface-cli or /usr/local/share/zsh/site-functions/_huggingface-cli ?") user has to update the completion script each time the huggingface_hub library is updated Either the user doesn't know what to do about it: user will most probably expect completion to work just after shtab is installed we could have a --print-completion-help command to explain a bit the procedure. Still not easy to know where to redirect the user. I have read the official documentation but the shtab --shell=zsh shtab.main.get_main_parser --error-unimportable is not directly applicable in our case (even a shtab --shell=zsh -u huggingface_hub.commands.huggingface_cli.get_main_parser wouldn't feel right if we had it).
The ideal solution is: When user pip install XXX
, they will get the shell completions out of box, not need to generate them by --print-completion
. In fact, python support packaging some data files to wheel format. For example, If you use setuptools
:
# pyproject.toml
[tool.setuptools.data-files]
"share/applications" = ["/the/path/of/XXX.desktop"]
"share/icons/hicolor/36x36/apps" = ["the/path/of/icon.png"]
"share/man/man1" = ["/the/path/of/foo.1"]
"share/bash-completion/completions" = ["/the/path/of/bash_completion.sh"]
"share/zsh/site-functions" = ["/the/path/of/_zsh_completion"]
Then user pip install --user foo
will get:
~/.local/lib/python3.10/site-packages/foo
~/.local/share/applications/XXX.desktop
~/.local/share/icons/hicolor/36x36/apps/icon.png
~/.local/share/man/man1/foo.1
~/.local/share/bash-completion/completions/bash_completion.sh
~/.local/share/zsh/site-functions/_zsh_completion
Or user sudo pip install foo
will get:
/usr/lib/python3.10/site-packages/foo
/usr/share/applications/XXX.desktop
/usr/share/icons/hicolor/36x36/apps/icon.png
/usr/share/man/man1/foo.1
/usr/share/bash-completion/completions/bash_completion.sh
/usr/share/zsh/site-functions/_zsh_completion
It will be out of box. User doesn't need to generate shell completion scripts by themselves and search which path is correct path to put the generated shell scripts. When they install foo
, the shell completion scripts will be installed together.
So we can write a setup.py
to when we build the packages, generate these shell completion scripts to a path, then tell tool.setuptools.data-files
these paths.
This is some examples:
Other tools I am familiar like scikit-build-core
, python-meson
also support this feature but use different methods. So I think your problem have been solved by these tools.
Login to write a write a comment.