- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.4k
 
          [misc] fix: Allow HF model ID with use_shm
          #3663
        
          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
Conversation
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.
Code Review
This pull request aims to allow using Hugging Face model IDs with use_shm. The implementation correctly adds logic to download models from the Hub. However, I've found a critical issue where this logic is incorrectly tied to the use_shm flag, causing it to fail when use_shm is false. Additionally, the error handling is too broad and silences all exceptions, which will make debugging difficult. I've provided suggestions to fix these issues.
| 
           ci unrelated  | 
    
| # Save to a local path for persistence. | ||
| local_path = copy_local_path_from_hdfs(src, cache_dir, filelock, verbose, always_recopy) | ||
| 
               | 
          ||
| if use_shm and isinstance(local_path, str) and not os.path.exists(local_path): | 
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.
Why only download model when use_shm=True?
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.
Didn't want to cause issues with other paths using the method, if you want I can have all calls
No description provided.