Skip to content
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

Merged
merged 6 commits into from
Dec 4, 2023
Merged

Conversation

stevhliu
Copy link
Member

@stevhliu stevhliu commented Nov 28, 2023

Starts implementing some of the things we discussed in our docs plan:

  • clarify the difference between using AutoPeftModel (inference) and PeftConfig + get_peft_model (train) and splits the quicktour more generally along these lines
  • add API reference for AutoPeftModel classes
  • cleans up the index with a Space for discovering what models/methods are supported given a task (let me know if you think this is easier and cleaner)
  • create separate subsection for LoRA guides

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

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, 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/_toctree.yml Show resolved Hide resolved
src="https://stevhliu-peft-methods.hf.space"
frameborder="0"
width="850"
height="450"
Copy link
Member

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?

Copy link
Member Author

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

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.

Copy link
Member Author

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!

Copy link
Member

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.

docs/source/index.md Show resolved Hide resolved
docs/source/quicktour.md Outdated Show resolved Hide resolved
docs/source/quicktour.md Outdated Show resolved Hide resolved
docs/source/quicktour.md Outdated Show resolved Hide resolved
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 for improving the docs, LGTM. I think it would be good to have another approval by Sourab or Younes before merging.

Copy link
Contributor

@younesbelkada younesbelkada left a 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

docs/source/quicktour.md Outdated Show resolved Hide resolved
@stevhliu stevhliu merged commit f7cf460 into huggingface:main Dec 4, 2023
14 checks passed
@stevhliu stevhliu deleted the doc-update-1 branch December 4, 2023 19:00
TaoSunVoyage pushed a commit to TaoSunVoyage/peft that referenced this pull request Dec 14, 2023
* first draft

* fix toctree

* lora subby section

* feedback

* iframe height

* feedback
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.

4 participants