Conversation
Signed-off-by: Jordan Mecom <jm@squareup.com>
7097602 to
f31b179
Compare
📝 WalkthroughWalkthroughThe changes propagate a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/auto_deploy/llm_args.py (1)
354-369:⚠️ Potential issue | 🟠 MajorForward
trust_remote_codeto internal factories inEagleOneModelFactory.The issue is confirmed:
EagleOneModelFactoryacceptstrust_remote_code(via**kwargspassed to the parent class) but does not forward it to the internalAutoModelForCausalLMFactory(line 295) andEagleDrafterFactory(line 305). Both factories support this parameter, but will default toFalse, silently ignoring the user's setting whenmodel_factory="eagle_one_model"is specified.Pass
trust_remote_code=self.trust_remote_codeto both factory instantiations:self.target_factory = AutoModelForCausalLMFactory( ... trust_remote_code=self.trust_remote_code, ) self.draft_factory = EagleDrafterFactory( ... trust_remote_code=self.trust_remote_code, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/auto_deploy/llm_args.py` around lines 354 - 369, EagleOneModelFactory is not forwarding the trust_remote_code flag to its internal factories, causing AutoModelForCausalLMFactory and EagleDrafterFactory to default to False; update the EagleOneModelFactory constructor where self.target_factory and self.draft_factory are created (references: EagleOneModelFactory, AutoModelForCausalLMFactory, EagleDrafterFactory) to pass trust_remote_code=self.trust_remote_code into both factory instantiations so the user's trust_remote_code setting is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tensorrt_llm/_torch/auto_deploy/llm_args.py`:
- Around line 354-369: EagleOneModelFactory is not forwarding the
trust_remote_code flag to its internal factories, causing
AutoModelForCausalLMFactory and EagleDrafterFactory to default to False; update
the EagleOneModelFactory constructor where self.target_factory and
self.draft_factory are created (references: EagleOneModelFactory,
AutoModelForCausalLMFactory, EagleDrafterFactory) to pass
trust_remote_code=self.trust_remote_code into both factory instantiations so the
user's trust_remote_code setting is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b42f1866-1069-4092-9dfe-5e07293498de
📒 Files selected for processing (4)
tensorrt_llm/_torch/auto_deploy/llm_args.pytensorrt_llm/_torch/auto_deploy/models/factory.pytensorrt_llm/_torch/auto_deploy/models/hf.pytests/unittest/auto_deploy/singlegpu/models/test_hf.py
|
/bot run |
| model_kwargs=self.model_kwargs, | ||
| tokenizer=None if self.tokenizer is None else str(self.tokenizer), | ||
| tokenizer_kwargs=self.tokenizer_kwargs, | ||
| trust_remote_code=self.trust_remote_code, |
There was a problem hiding this comment.
Our current default behavior is True for remote code trust. This will change the default to False. I suggest we overwrite the default of the LlmArgs from False to true for the AutoDeploy LlmArgs in particular.
Otherwise, you would have to go through all the failures and existing deployments and make sure to add trust_remote_code is True.
|
PR_Github #42186 [ run ] triggered by Bot. Commit: |
|
PR_Github #42186 [ run ] completed with state
|
Summary
trust_remote_codefrom AutoDeployLlmArgsinto the model factoryAutoConfig.from_pretrained,from_config, andfrom_pretrainedinstead of forcingTrueWhy
AutoDeploy accepted
trust_remote_code=Falseat the public API, but the Hugging Face model factory ignored that choice and hardcodedtrust_remote_code=Truein several load paths. That meant an operator could explicitly disable remote code execution and still end up loading model-defined Python during AutoDeploy initialization.This change makes AutoDeploy respect the callers trust boundary consistently across config, tokenizer, and model loading.
Summary by CodeRabbit
Refactor
Tests