-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feat: add support for Conv2D DoRA #1516
Conversation
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. |
@BenjaminBossan would love to get your thoughts on the changes I have introduced so far here. |
Thanks a lot Sayak, this already looks pretty good. Probably the main thing to get right is the dimensions across which to normalize, which does look correct to me. Maybe there is also some room to speed up the application of the norm, not sure. As a next step, let's add a test case for Conv2d + DoRA. This should be as easy as adding an example or two here: peft/tests/test_custom_models.py Lines 58 to 70 in 7e84dec
Let's do this and check if the tests pass, then plan the next steps. |
@BenjaminBossan I think this is ready for a review now. |
Cc: @nbasyl if you want to give this a look as well. |
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.
This looks pretty good, thanks a lot for enabling DoRA with Conv2d. Curious to see if we can get nice improvements for diffusion models.
Implementation-wise, I have only a few minor comments. On top of those, could you please document the new changes here:
- The docstring of
use_dora
inside oftuners/lora/config.py
still mentions that only linear layers are supported. - Same for the help of
use_dora
in the same file. docs/source/developer_guide/lora.md
also needs updating with regard to the supported layers.
Before merging, it would be nice to have another review.
Edit: Please also merge with recent main, as there could be merge conflicts with #1518 otherwise.
@BenjaminBossan resolved your comments. Thank you! |
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 a lot, Sayak, the PR looks great. Let's see if Shih-yang also has time to take a look. Otherwise, I'd be glad to have a second pair of eyes by @younesbelkada or @pacman100.
The PR looks great, I have skimmed through the code and did not notice any problems. @sayakpaul, Thanks for the effort! |
Thanks so much Sayak. |
TODO
_apply_dora()
in theforward()
of Conv2Dmerge()
unmerge()