8000 docs: Add comprehensive method comparison documentation by ved1beta · Pull Request #2504 · huggingface/peft · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 3 commits into from

Conversation

ved1beta
Copy link

Enhance Method Comparison Suite and Documentation

This PR significantly improves the method comparison capabilities and documentation of PEFT. The changes include:

Documentation Updates

  • Added comprehensive documentation for LoRA, LoRA-FA, and Bone methods
  • Included detailed performance characteristics and use cases
  • Added code examples and implementation details
  • Improved documentation structure for better developer experience

Method Comparison Suite Enhancements

  • Implemented new evaluation metrics for more comprehensive method comparison
  • Added support for new method configurations
  • Created a robust test suite for method comparison
  • Added configuration management system
  • Implemented sample generation capabilities

Analysis and Planning

  • Added detailed analysis of current documentation structure
  • Created comprehensive TODO list for future improvements
  • Added method comparison analysis document

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

  • Added comprehensive test suite for method comparison
  • Included test cases for new metrics and configurations
  • Added test coverage for sample generation

Documentation

  • Updated developer guides with new method documentation
  • Added configuration examples
  • Improved method comparison documentation

Copy link
Member
@BenjaminBossan BenjaminBossan left a 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.

8000
| 100M | ~50K | ~200KB |
| 1B | ~500K | ~2MB |
| 7B | ~3.5M | ~14MB |
| 13B | ~6.5M | ~26MB |
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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
Copy link
Member

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.

Copy link
Author

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good, thanks.

@ved1beta
Copy link
Author

and thanks a lot for the detailed review <3

@ved1beta ved1beta mentioned this pull request Apr 23, 2025
Copy link

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.

@ved1beta ved1beta closed this May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0