feat: enhance Auto Class to support remote and local path#5
feat: enhance Auto Class to support remote and local path#5Tohrusky merged 26 commits intoEutropicAI:mainfrom
Conversation
Summary of ChangesHello @Tohrusky, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major feature enhancement to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant enhancement, allowing models to be loaded from remote git repositories or local file paths. The implementation involves refactoring model caching and remote file handling into a new cccv.util.remote module and updating the AutoConfig and AutoModel classes. The registration process for custom models has also been clarified by removing the register methods from Auto* classes in favor of direct registry usage. My review focuses on improving security, robustness, and developer experience by suggesting better warning mechanisms, documenting security implications and external dependencies, and making the git URL parsing more resilient.
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Auto Class to support loading models from remote repositories and local paths, in addition to the existing registered model names. The key changes include refactoring model loading logic, improving error handling with proper warnings, and updating import statements.
- Adds support for remote git repository URLs and local directory paths in
AutoModel.from_pretrained() - Refactors cache/remote functionality from
cccv.cache_modelstocccv.util.remotewith enhanced git cloning capabilities - Updates error handling to use
warnings.warn()instead ofprint()statements throughout the codebase
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cccv/auto/config.py |
Implements new logic to handle remote URLs, local paths, and registered config names |
cccv/auto/model.py |
Updates from_pretrained() to use new config loading and removes registration methods |
cccv/util/remote.py |
New module containing relocated and enhanced remote file/git functionality |
cccv/cache_models/__init__.py |
Removed - functionality moved to cccv.util.remote |
example/remote.py |
New example demonstrating remote model loading |
| Various test files | Updated import paths and added remote model testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant enhancement by enabling AutoModel and AutoConfig to load models from remote git repositories and local paths. This greatly improves the flexibility of the library. The refactoring to centralize registration logic within the registries is a commendable design choice, and the move from print statements to warnings.warn for non-critical issues is a positive step towards better logging practices.
However, there are some critical issues in the error handling logic, where exit() and os._exit() are used instead of raising exceptions. This can lead to silent failures or abrupt termination of the application. Additionally, the registry's behavior for duplicate registrations has become too strict, potentially hindering the new dynamic loading feature. I've also suggested adding a security warning to the docstring for the new loading mechanism.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful new feature, allowing AutoModel and AutoConfig to load models from remote git repositories and local paths, in addition to pre-registered names. The implementation is robust, handling git cloning, dynamic module registration, and configuration parsing. I'm also very impressed with the numerous code quality improvements throughout the repository, such as replacing print/exit calls with proper exceptions and warnings, which significantly enhances the library's reliability and usability. The changes are well-tested and documented. I have a few suggestions to further improve consistency and documentation.
| elif True: | ||
| print(strVariable, type(objValue)) | ||
| print(f"[CCCV] {strVariable}, {type(objValue)}") |
There was a problem hiding this comment.
This elif True: acts as a catch-all that simply prints information about unhandled types. This can hide bugs or unsupported cases. It would be more robust to raise an exception for unexpected types to ensure the program fails fast and provides a clear error.
| elif True: | |
| print(strVariable, type(objValue)) | |
| print(f"[CCCV] {strVariable}, {type(objValue)}") | |
| else: | |
| raise TypeError(f"Unsupported type for cuda_kernel variable '{strVariable}': {type(objValue)}") |
| elif isinstance(objValue, torch.Tensor): | ||
| print(strVariable, objValue.dtype) | ||
| print(f"[CCCV] {strVariable}, {objValue.dtype}") | ||
|
|
||
| elif True: | ||
| print(strVariable, type(objValue)) | ||
| print(f"[CCCV] {strVariable}, {type(objValue)}") |
There was a problem hiding this comment.
Similar to the previous comment, these elif blocks handle unexpected tensor dtypes and other types by printing to the console. This can mask errors. It's better to raise a TypeError to explicitly signal that an unsupported type was passed to the function.
| elif isinstance(objValue, torch.Tensor): | |
| print(strVariable, objValue.dtype) | |
| print(f"[CCCV] {strVariable}, {objValue.dtype}") | |
| elif True: | |
| print(strVariable, type(objValue)) | |
| print(f"[CCCV] {strVariable}, {type(objValue)}") | |
| elif isinstance(objValue, torch.Tensor): | |
| raise TypeError(f"Unsupported tensor dtype for cuda_kernel variable '{strVariable}': {objValue.dtype}") | |
| else: | |
| raise TypeError(f"Unsupported type for cuda_kernel variable '{strVariable}': {type(objValue)}") |
| """ | ||
| Get a config instance of a pretrained model configuration. | ||
| Get a config instance of a pretrained model configuration, can be a registered config name or a local path or a git url. | ||
|
|
||
| :param pretrained_model_name: The name of the pretrained model configuration | ||
| :param pretrained_model_name_or_path: | ||
| :param model_dir: The path to cache the downloaded model configuration. Should be a full path. If None, use default cache path. | ||
| :return: | ||
| """ |
There was a problem hiding this comment.
The new functionality to load from a path or URL is great! To help users adopt it, it would be very beneficial to expand this docstring to describe the expected directory structure. For example:
- The path should point to a directory.
- The directory must contain a
config.jsonfile. config.jsonmust havename,arch, andmodelkeys.- Any custom Python code (e.g., for architectures or models) should be in
.pyfiles in that directory to be auto-registered. - The model weights file path is determined by the
pathkey inconfig.json. Ifpathis omitted, it defaults to a file in the same directory with a name matching thenamekey.
| def get_cache_dir(model_dir: Optional[Union[Path, str]] = None) -> Path: | ||
| if model_dir is None or str(model_dir) == "": | ||
| model_dir = str(CCCV_CACHE_MODEL_DIR) | ||
| print( | ||
| f"[CCCV] Using default cache model path {model_dir}, override it by setting environment variable CCCV_CACHE_MODEL_DIR" | ||
| ) | ||
| if not os.path.exists(model_dir): | ||
| os.makedirs(model_dir) | ||
| return Path(model_dir) |
There was a problem hiding this comment.
This file uses print() for informational messages (e.g., here on line 35, and also on lines 81, 96, 97, 141, 151). In a library, it's a best practice to use the logging module instead of print(). This gives the users of your library control over the verbosity and output stream. It would be great to refactor these print() calls to use logging.info() or logging.debug().
1 participant
No description provided.