-
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
Support Bone #2172
Support Bone #2172
Conversation
Please try this. |
There is a merge conflict, could you please resolve it? Thanks. |
resolved! |
|
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. |
@JL-er I think the failing tests should be unrelated to your PR. Could you please merge the latest main branch of PEFT into your branch? This should fix those issues. |
Thank you, it has been merged. |
What do I need to change to fix this. |
The remaining errors are just flakiness of the CI, don't worry. I'll just restart it, it should pass on the 2nd or 3rd try :) |
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.
All the tests are passing now, nice!
For this PR, I focused on the docs and examples and found a few issues. Could you please check those? Thanks. The implementation itself should be good as is.
@JL-er please ping me if the PR is ready for review. |
@BenjaminBossan Yes, it's ready |
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 for the updates, the example is much cleaner now and easier to understand. While running it locally, I still encountered a few issues, but I managed to get it to train in the end. I noted the issues I found, please check them out.
@JL-er LMK if this is ready to review. |
I modified the finetune of bone to be similar to pissa and successfully ran it. I believe there are no issues now. Please accept my PR. |
@BenjaminBossan When I ran pytest tests/ -k "bone", I encountered some errors, but I believe they are unrelated to Bone. |
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 so much for the updates. I like that you simplified the example script. There, I found a small error, please check.
When I ran pytest tests/ -k "bone", I encountered some errors, but I believe they are unrelated to Bone.
Not sure what that is, but my best guess is some outdated packages. Anyway, the CI passes, so I wouldn't worry too much. Regarding the CI, there seems to be some issue with MacOS github runners, not sure what that's about, but it's not caused by your PR.
@BenjaminBossan So what should I do now to ensure that my PR is accepted? |
@JL-er There is nothing to be done on your side. I'll run the CI again tomorrow, hopefully this is a transient problem and the error will pass by then. If the CI issues persist, it's also fine to merge despite the error, as I'm fairly certain they're unrelated. You don't happen to have a Mac to run the tests, do you? |
yes, I don't have a mac. |
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 for adding the Bone method to PEFT and for your patience with the review process. The PR LGTM.
If in the future you find a way to improve memory usage, please open a PR.
yes, I don't have a mac.
The MacOS issues have been resolved by themselves, it was most likely a problem on GitHub's side, now the tests all pass.
@BenjaminBossan Thank you for your help during this time. If there are any updates to Bone, I will update. : ) |
Re pr #2148