-
Notifications
You must be signed in to change notification settings - Fork 2k
docs: Add comprehensive method comparison documentation #2504
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
Conversation
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 @ved1beta for this huge contribution, it is really appreciated. I didn't check the full details yet of this PR, but it looks very useful and will hopefully be helpful for the PEFT users.
To make the PR easier to review, it would be great if you could split it up into separate PRs. I think most of your changes are orthogonal, so it should hopefully not be too much work for you. The 5 sections you described above could be how to slice up this big PR.
Regarding the changes themselves, I have a couple of questions:
1. Documentation Updates
We have to think about how to integrate the documentation. As is, you added a couple of markdown files to the docs folder, but they're not part of the table of contents, so they cannot be found. I wonder if we should integrate them into the existing docs (maybe consider a new section) or if there is another way.
One of our goals with the method comparison suite was to reduce the amount of manual work and automate as much as possible. If a new PEFT method with better results is added, ideally we don't have to rewrite the whole documentation. This is why we added the Gradio app, so that users can quickly explore the results (we plan to automatically create a HF Space for it via CI). There is no final decision on how we want to present the results, but maybe you have some ideas of your own.
2. Method Comparison Suite Enhancements
After my first inspection, it is not quite clear to me what capabilities you added and how to use them. I think it's crucial to document this well.
3. Analysis and Planning
I'm not sure what part you are referring to here.
4. Testing
IMHO, this is the least important part of the PR. Yes, it is good to have more unit tests and we will gladly merge them, but from the point of view of user impact, I would place it at lowest priority.
5. Documentation
Not quite sure how this differs from 1.
Again, I appreciate the work you put into this and I'm sure we will merge most of it, but let's put a bit of structure into this by breaking down the PR into multiple PRs.
PS: In the future, before starting big PRs like this, it's a good idea to first comment on an issue, or open a new one, with your suggestion, and ping us so that we can give early feedback.
| 100M | ~50K | ~200KB | | ||
8000 | | 1B | ~500K | ~2MB | | |
| 7B | ~3.5M | ~14MB | | ||
| 13B | ~6.5M | ~26MB | |
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.
How did you arrive at the numbers you present here and below? Do you perhaps have a script to generate those? If yes, it would be great to add the script.
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.
i have have some scripts but i am not able to run them locally will need to probably run and test them , these are just dummy values for now
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.
You can create a draft PR and add those scripts there, I can check them on my machine.
|
||
## Implementation | ||
|
||
### Basic Usage |
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.
I think these sections have some duplication with already existing docs/examples. I would prefer to refer to those instead.
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.
okk , will goo step by step raising a PR for the docs part for now , adding the 3 md files and then we will create a different section in docs for these files how does this sound ?
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.
That sounds good, thanks.
and thanks a lot for the detailed review <3 |
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. |
Enhance Method Comparison Suite and Documentation
This PR significantly improves the method comparison capabilities and documentation of PEFT. The changes include:
Documentation Updates
Method Comparison Suite Enhancements
Analysis and Planning
These changes will help users better understand the different PEFT methods and make more informed decisions about which method to use for their specific use cases.
Testing
Documentation