-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix model argument issue (#1198) #1205
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
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.
Looks good! What do you think @BenjaminBossan ?
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, Ngoc, for working on this fix.
Right now, in _mark_only_adaption_prompts_as_trainable
, you allow model
to be None
and then do model = model or self.model
. I would prefer to make model
a required argument and remove the mentioned line. This way, it's not possible to accidentally forget to pass model
and get unexpected results.
If this change is made, _mark_only_adaption_prompts_as_trainable
could even be a standalone function, but that's not really a big concern.
Furthermore, there is one place where this method still needs to be adjusted, which should also make the CI pass:
peft/src/peft/tuners/mixed/model.py
Line 132 in 5bad88b
def _mark_only_adapters_as_trainable(self) -> None: |
Apart from that, this change looks good.
The reason I want to If you still prefer to make |
I'm not quite sure I understand this. Do you mean that PEFT users may want to call |
Sorry for the delay, I fixed the issue according to your suggestions. |
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.
Looks good, thanks.
A quick fix to issue #1198 that makes sure one model is used throughout
inject_adapter
function inBaseTuner
.