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

Generate: add missing logits processors docs #25653

Merged
merged 7 commits into from
Aug 25, 2023

Conversation

gante
Copy link
Member

@gante gante commented Aug 22, 2023

What does this PR do?

Some logits processor classes were missing on the docs. This PR corrects it by:
1 - Adding all missing logits processors to the docs
2 - Adding missing top-level imports (to stay consistent with previously existing logits processors)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 22, 2023

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

@gante
Copy link
Member Author

gante commented Aug 22, 2023

I've confirmed that the previously missing docs are now properly rendered in the doc preview 👍

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Would be cool if the different logits appear in the small summary to directly jump to one or another:
image
given that there are so many different ones WDYT?

Also I noticed that the logit_process.py is not doctested, might be because some of the examples use big models, but would be cool to support them.

docs/source/en/internal/generation_utils.md Show resolved Hide resolved
@gante
Copy link
Member Author

gante commented Aug 22, 2023

@ArthurZucker re tests -- fully on board. If you're okay with it, I'd like to do it on a separate PR :) Ditto for the side bar on the left, it is in need of an update!

@gante
Copy link
Member Author

gante commented Aug 24, 2023

@ArthurZucker ready for a re-review!

To recap, the previously reviewed version:

  • Added missing logits processor docs

This version also:

  • Updates the left-handed TOC for this page (splits the classes by framework, so it's easier to navigate; I've decided against enumerating the logits processors in the docs since the list is quite big)
  • Sorts the logits processors in the docs by alphabetical order
  • Adds missing output classes to the output class docs section

In parallel, other concerns raised in the PR are being tackled in another places:

@gante
Copy link
Member Author

gante commented Aug 24, 2023

(preview of the left-handed side TOC)
Screenshot 2023-08-24 at 14 21 45

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Great work 🤗 looks a lot better

@gante gante merged commit 85cf90a into huggingface:main Aug 25, 2023
21 checks passed
@gante gante deleted the logits_processors_docs branch August 25, 2023 10:56
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
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.

3 participants