[ROCm][Quantization] fallback trust_remote_code=True in Quark config for some cases#37408
[ROCm][Quantization] fallback trust_remote_code=True in Quark config for some cases#37408xuebwang-amd wants to merge 2 commits intovllm-project:mainfrom
Conversation
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
There was a problem hiding this comment.
Code Review
This pull request adds a fallback mechanism to load model configurations that require trust_remote_code=True. While this improves compatibility with certain models, it introduces a security risk by potentially executing remote code without user awareness. I've added a critical comment to suggest logging a warning when this fallback is triggered to ensure users are informed about the remote code execution.
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
| quant_dtype = quant_config["global_quant_config"]["weight"]["dtype"] | ||
| model_type = self.hf_config.model_type | ||
| if quant_dtype == "fp4" and model_type == "deepseek_v3": | ||
| self.dynamic_mxfp4_quant = True |
There was a problem hiding this comment.
It seems the whole purpose of that overridden function maybe_update_config is to set dynamic_mxfp4_quant to True for deepseek_v3 model famliy, very model specific.
Is that possible to guide the whole block of code of calling get_config only for the deepseek_v3 type of model without impacting other models, like the model you have issue?
There was a problem hiding this comment.
Thanks for the review! Yeah, from model perspective, more exceptional cases can be collected.
|
|
||
| self.hf_config = get_config( | ||
| model=model_name, | ||
| trust_remote_code=True, |
There was a problem hiding this comment.
--trust-remote-code needs to be explicitly provided if it is required, doesn't it, to avoid security risks?
Why would you override this silently?
There was a problem hiding this comment.
Thanks for the review! Yes, --trust-remote-code is needed in the CLI.
Please see updated details #37408 (comment).
|
hi everyone thanks for this PR i want to use mxfp4 minimax m2.5 but unfortunately running into this issue |
|
check this PR: #37698 |
|
Here is a detailed content with re-validation for the PR.
Root causeIt is not a CLI propagation issue: Note:
Minimal compatibility fix of this PRTo keep risk low, the patch is intentionally minimal and Quark-local:
No broad fallback is kept in |
|
Close since #37698 is merged. |
Purpose
Model: amd/MiniMax-M2.1-MXFP4
Transformers: 4.57.6
Error message:
Test Plan & Result
After fixing: