Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Potentially a bug #1198

Closed
ngocbh opened this issue Nov 30, 2023 · 3 comments
Closed

Potentially a bug #1198

ngocbh opened this issue Nov 30, 2023 · 3 comments

Comments

@ngocbh
Copy link
Contributor

ngocbh commented Nov 30, 2023

https://github.com/huggingface/peft/blob/2b901ee57230559aaf39867c7698f6aca3617162/src/peft/tuners/tuners_utils.py#L260C32-L260C32

inject_adapters have its own model argument but, in line 260 and 263, it accesses model parameters using self.model. I guess it's not a safe choice.
@BenjaminBossan
Copy link
Member

Thanks for bringing this up. The model and self.model are indeed the same object (check model is self.model), so the code works correctly when used as intended. I agree, however, that this is confusing and it would be better if model was used throughout. If you want to create a PR to fix that, we'd be happy to merge it.

ngocbh pushed a commit to ngocbh/peft that referenced this issue Dec 1, 2023
Fix model argument issue (huggingface#1198)
6774965
@ngocbh ngocbh mentioned this issue Dec 1, 2023
Merged
@ngocbh
Copy link
Contributor Author

ngocbh commented Dec 1, 2023

Hi @BenjaminBossan, I created a PR to fix this issue.

ngocbh pushed a commit to ngocbh/peft that referenced this issue Dec 9, 2023
Update fix for model argument issue (huggingface#1198)
16f8d89
BenjaminBossan pushed a commit that referenced this issue Dec 11, 2023
@ngocbh
FIX Use model argument consistently (#1198) (#1205)
5c13ea3 
Some methods were using model and self.model interchangeably. This was
fine, as they were referring to the same object, but is also confusing.
Now model is used consistently.
@BenjaminBossan
Copy link
Member

Resolved in #1205, thanks @ngocbh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@BenjaminBossan @ngocbh