-
Notifications
You must be signed in to change notification settings - Fork 135
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
Support huggingface-cli older than 0.25.0, like on Fedora 40 and 41 #468
Support huggingface-cli older than 0.25.0, like on Fedora 40 and 41 #468
Conversation
Reviewer's Guide by SourceryThe PR modifies the Hugging Face CLI availability check to support older versions of huggingface-cli (pre-0.25.0) by using the '--help' command instead of the 'version' command, which was only introduced in version 0.25.0. Sequence diagram for Hugging Face CLI availability checksequenceDiagram
participant User
participant System
User->>System: Execute ramalama pull
System->>System: run_cmd(["huggingface-cli", "--help"])
alt huggingface-cli available
System->>User: CLI is available
else huggingface-cli not found
System->>User: Print warning message
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @debarshiray - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
How about? import shutil path = shutil.which("huggingface-cli") |
Sure, why not? I am not much of a Python programmer. :) |
Currently, on Fedora 40 and 41, pulling a model from Hugging Face fails with: $ ramalama pull huggingface://afrideva/Tiny-Vicuna-1B-GGUF/tiny-vicuna-1b.q2_k.gguf usage: huggingface-cli <command> [<args>] huggingface-cli: error: argument {env,login,whoami,logout,repo,upload, download,lfs-enable-largefiles,lfs-multipart-upload,scan-cache, delete-cache,tag}: invalid choice: 'version' (choose from 'env', 'login', 'whoami', 'logout', 'repo', 'upload', 'download', 'lfs-enable-largefiles', 'lfs-multipart-upload', 'scan-cache', 'delete-cache', 'tag') Error: Command '['huggingface-cli', 'version']' returned non-zero exit status 2. This is because the 'huggingface-cli version' command is a recent addition in version 0.25.0 [1], and it's not present in any stable Fedora release [2]. Therefore, it might be worth using a slightly different signature to detect the presence of huggingface-cli - one that isn't bound to a huggingface-cli version and is used elsewhere in the code. [1] huggingface_hub commit ebac1b2a1aa4a9f4 huggingface/huggingface_hub@ebac1b2a1aa4a9f4 huggingface/huggingface_hub#2441 [2] https://src.fedoraproject.org/rpms/python-huggingface-hub Fixes: 5749154 ("Replace huggingface-cli with a simple ...") Signed-off-by: Debarshi Ray <[email protected]>
1d14439
to
19f1918
Compare
LGTM |
Currently, on Fedora 40 and 41, pulling a model from Hugging Face fails with:
This is because the
huggingface-cli version
command is a recent addition in version 0.25.0 [1], and it's not present in any stable Fedora release [2].Therefore, it might be worth using a slightly different signature to detect the presence of
huggingface-cli
- one that isn't bound to ahuggingface-cli
version and is used elsewhere in the code.[1] huggingface_hub commit ebac1b2a1aa4a9f4
huggingface/huggingface_hub@ebac1b2a1aa4a9f4
huggingface/huggingface_hub#2441
[2] https://src.fedoraproject.org/rpms/python-huggingface-hub
Fixes: 5749154 ("Replace huggingface-cli with a simple ...")
Summary by Sourcery
Bug Fixes: