-
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
[docs] Update index and quicktour #1191
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. |
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, Steven, this looks like it will help give a better structure to the PEFT docs and make the intro easier to understand for new users.
I left a couple of comments, please take a look.
docs/source/index.md
Outdated
src="https://stevhliu-peft-methods.hf.space" | ||
frameborder="0" | ||
width="850" | ||
height="450" |
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 no idea about iframes, but can the height be dynamic, so that we don't have to scroll for longer lists?
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.
Tried a couple things but I couldn't get it to work. I just set a larger height for now
| SegFormer | ✅ | | | | | | ||
|
||
<iframe | ||
src="https://stevhliu-peft-methods.hf.space" |
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.
Nice space. Is it somehow updated automatically or would it need to be updated each time we add a new model?
Also, I'd reword the description a little bit:
Discover what models and PEFT methods are supported for a given task
This could give the impression that other models don't work at all. However, almost all models work, if they're not supported, it's only a matter of setting the config correctly. I think this is important to highlight, as some other similar libraries are actually only working with specific model architectures.
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.
Currently, the underlying dataset would need to be updated each time a new model is added or if a new method is supported. Let me know if you have any suggestions for automating it though, that'd be nice!
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.
Technically, it would be possible. For example, we could set up a cron job or GitHub CI to parse the README (assuming that should be the single source of truth) with pandoc, extract the table, and create an update the on the dataset. But that seems like overkill for now. Probably we, the maintainers, just need to remember to update the dataset each time a new model is supported.
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 improving the docs, LGTM. I think it would be good to have another approval by Sourab or Younes before merging.
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.
Impressive work @stevhliu ! Thanks a lot for this great work
* first draft * fix toctree * lora subby section * feedback * iframe height * feedback
Starts implementing some of the things we discussed in our docs plan:
AutoPeftModel
(inference) andPeftConfig
+get_peft_model
(train) and splits the quicktour more generally along these linesAutoPeftModel
classes