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

Improve performance docs #17750

Merged
merged 11 commits into from
Jun 23, 2022
Merged

Improve performance docs #17750

merged 11 commits into from
Jun 23, 2022

Conversation

lvwerra
Copy link
Member

@lvwerra lvwerra commented Jun 17, 2022

What does this PR do?

As discussed with @stas00 this PR does the following things:

  • adds files for missing sections so contributors can better find the place to add content.
  • adds a disclaimer at the beginning stating that a lot of general training information is in the single GPU training section
  • fixes the link of CPU inference

Looking at the ToC I was wondering whether we should add subsections like it is done for the tasks to make the main ToC a bit slimmer. Otherwise we have the main performance docs (the entry point) there plus all the other sections (~8-10). What do you think?

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@lvwerra lvwerra requested review from stas00 and sgugger June 17, 2022 10:28
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 17, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@sgugger sgugger 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 working on this. Could we add at least one line in each of those files telling the reader "Coming soon"?
Definitely yes for grouping all of those guides in one subsection, like the Tasks one.

@lvwerra
Copy link
Member Author

lvwerra commented Jun 17, 2022

Sounds good @sgugger, I added the "Coming soon" and restructured the ToC.

@stas00
Copy link
Contributor

stas00 commented Jun 17, 2022

Communications are difficult.

what I proposed is to add normal documents - not "Coming Soon"

inside those documents they should all explicitly point the user to read the first document that is already filled out and that we will expand this other currently mostly empty doc with the missing material.

In other words e.g,, perf_infer_gpu_many.mdx should have:

  1. read perf_train_gpu_one.mdx first
  2. this document will be completed soon.

If the docs remain "Coming Soon" nobody will read them and will miss out on the already rich performance docs we have.

And the impetus for this change was that we went from a complete solution - all performance notes in one doc, to very incomplete solution, making it look like we only have advise for those with 1 gpu and doing training.

The original discussion back in winter was that all performance docs will be filled out, but it was dropped after the first document and no signs of new docs coming any time soon. So this proposal was my attempt to rescue the situation.

title: Inference on Specialized Hardware
- local: perf_hardware
title: Custom hardware for training
title: Performance anc scalability
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo ? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks!

@lvwerra
Copy link
Member Author

lvwerra commented Jun 20, 2022

Ok I added references to each document and a small text explaining what will come there. Is that what you had in mind @stas00?

Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

With a few suggestions to fix this looks good.

In particular I added pointers to 1-gpu train from infer docs since most of it applies and the current infer files are almost empty and don't have the essentials.

docs/source/en/performance.mdx Outdated Show resolved Hide resolved
docs/source/en/performance.mdx Outdated Show resolved Hide resolved
docs/source/en/perf_infer_gpu_one.mdx Outdated Show resolved Hide resolved
docs/source/en/perf_infer_special.mdx Outdated Show resolved Hide resolved
docs/source/en/perf_infer_gpu_many.mdx Outdated Show resolved Hide resolved
@lvwerra lvwerra merged commit 6f29029 into main Jun 23, 2022
@lvwerra lvwerra deleted the improve-performance-docs branch June 23, 2022 12:51
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 25, 2022
* add skeleton files

* fix cpu inference link

* add hint to make clear that single gpu section contains general info

* add new files to ToC

* update toctree to have subsection for performance

* add "coming soon" to the still empty sections

* fix missing title

* fix typo

* add reference to empty documents

* Apply suggestions from code review

Co-authored-by: Stas Bekman <[email protected]>

* Apply suggestions from code review

Co-authored-by: Stas Bekman <[email protected]>

Co-authored-by: Stas Bekman <[email protected]>
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 29, 2022
* add skeleton files

* fix cpu inference link

* add hint to make clear that single gpu section contains general info

* add new files to ToC

* update toctree to have subsection for performance

* add "coming soon" to the still empty sections

* fix missing title

* fix typo

* add reference to empty documents

* Apply suggestions from code review

Co-authored-by: Stas Bekman <[email protected]>

* Apply suggestions from code review

Co-authored-by: Stas Bekman <[email protected]>

Co-authored-by: Stas Bekman <[email protected]>
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.

5 participants