-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add trust_remote_code to GRPOConfig #4186
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
base: main
Are you sure you want to change the base?
Add trust_remote_code to GRPOConfig #4186
Conversation
) | ||
trust_remote_code: bool = field( | ||
default=False, | ||
metadata={"help": "Whether to trust remote code when loading custom models e.g. from the Hugging Face Hub."}, |
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.
metadata={"help": "Whether to trust remote code when loading custom models e.g. from the Hugging Face Hub."}, | |
metadata={"help": "Whether to trust remote code when loading custom models from the Hugging Face Hub."}, |
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.
trust_remote_code
matters when loading things from local files too. The docs of vLLM describe it similarly.
Trust remote code (e.g., from HuggingFace) when downloading the model and tokenizer.
https://docs.vllm.ai/en/latest/api/vllm/index.html#vllm.LLM
That said, the docs of transformers
only mention the hub. Do you think e.g.
should be deleted? I don't have a strong opinion so will follow your preference.
Whether or not to allow for custom models defined on the Hub in their own modeling files. This option should only be set to True for repositories you trust and in which you have read the code, as it will execute code present on the Hub on your local machine.
trl/trainer/grpo_trainer.py
Outdated
# Disable caching if gradient checkpointing is enabled (not supported) | ||
config = AutoConfig.from_pretrained(model_id) | ||
config = AutoConfig.from_pretrained(model_id, trust_remote_code=self.args.trust_remote_code) | ||
architecture = getattr(transformers, config.architectures[0]) |
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.
When using remote code, the idea is that that model is not included in transformers, right? So maybe you need something like this instead:
if hasattr(transformers, config.architectures[0]):
architecture = getattr(transformers, config.architectures[0])
model = architecture.from_pretrained(model_id, **model_init_kwargs)
else:
model = AutoModelForCausalLM.from_pretrained(model_id, trust_remote_code=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.
Thanks, you are right. I haven't tested the code path of AutoConfig as I was passing an already loaded model to GRPOTrainer. I will try rewriting it like your code.
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.
I fixed it. Now it works when model
passed to GRPOTrainer is str.
8376f61
to
90c91b0
Compare
What does this PR do?
This PR adds
trust_remote_code
toGRPOConfig
and makesGRPOTrainer
use it when making the model and its related objects to support custom models.Fixes #4129
Before submitting
Pull Request section?
to it if that's the case.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.