[FIX] add trust remote #794
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Yunnglin, 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 primarily addresses a fix by integrating the trust_remote_code=True parameter into numerous from_pretrained calls across the codebase. This change is crucial for correctly loading models, processors, and tokenizers from remote sources, particularly those that depend on custom code definitions. Additionally, a small security improvement was implemented by preventing the logging of API keys.
Highlights
- Enabled
trust_remote_codefor Model and Processor Loading: Thetrust_remote_code=Trueparameter has been added toAutoModel.from_pretrainedandAutoProcessor.from_pretrainedcalls inevalscope/backend/rag_eval/utils/clip.pyandevalscope/metrics/t2v_metrics/models/clipscore_models/pickscore_model.pyto ensure proper loading of models and processors that might require custom code. - Enabled
trust_remote_codefor Tokenizer Loading: Thetrust_remote_code=Trueparameter has been consistently applied toAutoTokenizer.from_pretrainedcalls across various files, includingevalscope/benchmarks/needle_haystack/needle_haystack_adapter.py,evalscope/metrics/t2v_metrics/models/itmscore_models/fga_blip2_model.py,evalscope/metrics/t2v_metrics/models/itmscore_models/image_reward/blip_pretrain.py,evalscope/metrics/t2v_metrics/models/vqascore_models/lavis/models/blip_models/blip.py,evalscope/metrics/t2v_metrics/models/vqascore_models/mm_utils.py,evalscope/perf/plugin/api/custom_api.py,evalscope/perf/plugin/api/openai_api.py, andevalscope/third_party/thinkbench/eval.py. This ensures that tokenizers requiring custom code can be loaded correctly. - Security Enhancement: API Key Logging: A minor but important change was made in
evalscope/api/model/model.pyto remove theapi_keyfrom a logging statement, preventing the accidental exposure of sensitive information in logs.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces trust_remote_code=True across multiple files to enable loading certain models. While this may fix model loading issues, it introduces a significant security risk by allowing arbitrary code execution from model repositories. My review comments highlight these locations and suggest making this behavior configurable and disabled by default to mitigate the risk. Additionally, the change to remove the API key from log messages is a good security practice.
| self.model = AutoModel.from_pretrained(model_name, trust_remote_code=True).to(self.device) | ||
| self.processor = AutoProcessor.from_pretrained(model_name, trust_remote_code=True) |
There was a problem hiding this comment.
Setting trust_remote_code=True introduces a security risk, as it allows the execution of arbitrary code from the model hub. This could be exploited if a malicious model is used. It's recommended to make this behavior configurable and disabled by default, or at least add a clear warning to users about the potential risks.
| """ Initialize the tokenizer based on the provided tokenizer path.""" | ||
| from modelscope import AutoTokenizer | ||
| self.tokenizer = AutoTokenizer.from_pretrained(self.tokenizer_path) | ||
| self.tokenizer = AutoTokenizer.from_pretrained(self.tokenizer_path, trust_remote_code=True) |
There was a problem hiding this comment.
Using trust_remote_code=True can be a security vulnerability. It permits the execution of code from the model repository, which could be malicious. Consider making this feature optional and off by default, or adding a prominent warning about the security implications when loading models from untrusted sources.
| self.processor = AutoProcessor.from_pretrained(processor_name_or_path, trust_remote_code=True) | ||
| self.model = AutoModel.from_pretrained(model_pretrained_name_or_path, | ||
| trust_remote_code=True).eval().to(self.device) |
There was a problem hiding this comment.
Enabling trust_remote_code=True for both the processor and the model poses a security risk by allowing arbitrary code execution from the model hub. If the models are not from a fully trusted source, this could be dangerous. It would be safer to make this configurable and disabled by default.
| self.tokenizer = AutoTokenizer.from_pretrained( | ||
| 'AI-ModelScope/bert-base-uncased', truncation_side='right', trust_remote_code=True | ||
| ) |
There was a problem hiding this comment.
Setting trust_remote_code=True allows remote code execution and should be used with caution. This can be a security risk if the model source is not completely trusted. I'd suggest making this behavior configurable, so users can disable it when working with untrusted models.
|
|
||
| def init_tokenizer(): | ||
| tokenizer = AutoTokenizer.from_pretrained('AI-ModelScope/bert-base-uncased') | ||
| tokenizer = AutoTokenizer.from_pretrained('AI-ModelScope/bert-base-uncased', trust_remote_code=True) |
There was a problem hiding this comment.
The use of trust_remote_code=True introduces a security risk by allowing code from the model hub to be executed. This should be avoided if possible, or at least made configurable and opt-in, with clear warnings to the user.
| @classmethod | ||
| def init_tokenizer(cls): | ||
| tokenizer = AutoTokenizer.from_pretrained('AI-ModelScope/bert-base-uncased') | ||
| tokenizer = AutoTokenizer.from_pretrained('AI-ModelScope/bert-base-uncased', trust_remote_code=True) |
There was a problem hiding this comment.
Adding trust_remote_code=True can lead to security vulnerabilities by executing code from the model repository. It's recommended to only use this with fully trusted models. Consider making this setting configurable to enhance security.
| from ..utils import download_file | ||
|
|
||
| tokenizer = AutoTokenizer.from_pretrained(tokenizer_path, **tokenizer_dict) | ||
| tokenizer = AutoTokenizer.from_pretrained(tokenizer_path, trust_remote_code=True, **tokenizer_dict) |
There was a problem hiding this comment.
The trust_remote_code=True parameter can be a security risk, as it allows arbitrary code execution from the model hub. This should be used with caution. It would be better to make this configurable and off by default.
| if param.tokenizer_path is not None: | ||
| from modelscope import AutoTokenizer | ||
| self.tokenizer = AutoTokenizer.from_pretrained(param.tokenizer_path) | ||
| self.tokenizer = AutoTokenizer.from_pretrained(param.tokenizer_path, trust_remote_code=True) |
There was a problem hiding this comment.
Using trust_remote_code=True poses a security risk by allowing remote code execution. This is especially concerning in a performance testing plugin where various models might be tested. It's advisable to make this behavior configurable and require users to explicitly enable it.
| if param.tokenizer_path is not None: | ||
| from modelscope import AutoTokenizer | ||
| self.tokenizer = AutoTokenizer.from_pretrained(param.tokenizer_path) | ||
| self.tokenizer = AutoTokenizer.from_pretrained(param.tokenizer_path, trust_remote_code=True) |
There was a problem hiding this comment.
Setting trust_remote_code=True can be a security vulnerability. It allows executing code from the model repository, which could be malicious. This should ideally be a configurable option that is disabled by default to ensure security.
| self.subset_dict = defaultdict(lambda: defaultdict(list)) | ||
| self.think_end_token = '</think>' | ||
| self.tokenizer = AutoTokenizer.from_pretrained(tokenizer_path) | ||
| self.tokenizer = AutoTokenizer.from_pretrained(tokenizer_path, trust_remote_code=True) |
There was a problem hiding this comment.
The addition of trust_remote_code=True introduces a security risk. It allows arbitrary code execution from the model hub, which can be dangerous if the model source is not trusted. Please consider making this configurable or adding a warning.
No description provided.