-
Notifications
You must be signed in to change notification settings - Fork 632
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
Add autcompletion to huggingface-cli (fix #1197) #1207
Conversation
Freed-Wu
commented
Nov 20, 2022
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 ! What I tried is
to make it work ? Or should I copy-paste the output of 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 |
Good. Now can you merge this branch? And how about huggingface/datasets#5269? |
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.
@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:
- Do you know if there is a way to detect from Python code if the autocompletion is set on the machine/in the shell ? I'm asking because ideally I would like to add a warning "hey it seems you have installed
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 dopip install huggingface_hub[completion]
and expect completion to work out of the box. - Do you know if there is a way to programmatically setup the shell from Python code ? This would be perfect as no extra step would be required by the user, therefore reducing friction.
- Do you know if completion has to be re-configured each time we add a new command to huggingface_hub ? I suppose so but asking to be sure. If yes, configuring it programmatically would be even more useful otherwise users will not get notified about new features.
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.
``` $ huggingface-cli repo create -<TAB> Name for your repo. Will be namespaced under your username to build the repo id. option --help show this help message and exit -h show this help message and exit --organization Optional: organization namespace. --space_sdk Optional: Hugging Face Spaces SDK type. Required when --type is set to "space". --type Optional: repo_type: set to "dataset" or "space" if creating a dataset or space, default is model. --yes Optional: answer Yes to the prompt -y Optional: answer Yes to the prompt ```
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 ? |
I don't know and I think it shouldn't be done by python, For example, bash user
As I know, it is usually done by package manager, such as
Other software distribution, such as are same.
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 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 |
Also, would it be possible to have write-access to |
You can
It is the duty of other software distributions.
I don't known. :( |
Codecov ReportBase: 82.57% // Head: 83.98% // Increases project coverage by
Additional details and impacted files@@ 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
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. |
@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:
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 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. (EDIT: for reference, here is the slack discussion (internal link)) |
It can refer https://github.com/andreafrancia/trash-cli/blob/master/trashcli/shell_completion.py#L43.
I have said that it depends on you 😄 About your concern (install shell completion by
OK. Remember post me in the future 👍 |
Good.
|
Hey, thanks for the feedback! A few points:
|
Thanks @casperdcl for your input !
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. |
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.
Agree 👍
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).
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.
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 No defense again 😄 |
At the risk of starting a tangential discussion:
Completely disagree, sorry. Minimising attack surface is done by pinning dependencies, not marking them as optional. |
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
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:
The ideal solution is: When user # 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
Or user
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 So we can write a This is some examples: Other tools I am familiar like |