-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Decrease memory usage of merge_and_unload
#1944
Decrease memory usage of merge_and_unload
#1944
Conversation
@BenjaminBossan Would be great to get your approval here! |
I checked the PEFT code on why we merge this way. Turns out we used to do it in-place but this PR changed it: #1372. IMO merging should not affect the error reported there, so I think we can undo the change, but I wanted to provide the necessary context. Could you please also update the other merge functions? peft/src/peft/tuners/lora/layer.py Line 671 in ba75bb1
peft/src/peft/tuners/lora/layer.py Line 927 in ba75bb1
Thanks. I plan to make a release soon, aiming for tomorrow. Let's get this fix ready before the release. |
@BenjaminBossan thanks for the pointers, found some additional places where this change is also needed. Hope this looks good to you. |
A release tomorrow with this fix would be ideal. |
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Strange that Windows CI is consistently failing. Hard to imagine that this is caused by the PR but I'm investigating further (but don't have a Windows machine myself). Hopefully this is just a temporary issue. |
Yeah quite weird:
|
Okay, so the same Windows errors as in #1947, which has no code changes. |
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 for restoring the more memory efficient merging behavior. A release tomorrow should be possible.
Addresses #1939