-
Notifications
You must be signed in to change notification settings - Fork 31.9k
Rework automatic code samples in docstrings #20757
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
Conversation
| >>> model.config.id2label[predicted_class_id] | ||
| {expected_output} | ||
| ``` | ||
| >>> predicted_class_ids = torch.arange(0, logits.shape[-1])[torch.sigmoid(logits).squeeze() > 0.5] |
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.
Here the code sample did not make any sense for multi-label classification, so changed it.
src/transformers/utils/doc.py
Outdated
| >>> image = dataset["test"]["image"][0] | ||
| >>> feature_extractor = {processor_class}.from_pretrained("{checkpoint}") | ||
| >>> image_processor = ImageProcessor.from_pretrained("{checkpoint}") |
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.
Adapted names in the vision examples.
|
The documentation is not available anymore as the PR was closed or merged. |
ydshieh
left a comment
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.
LGTM, but maybe I miss sth - why for most of them, we don't add processor_class="AutoTokenizer",
src/transformers/utils/doc.py
Outdated
| ```python | ||
| >>> from transformers import {processor_class}, {model_class} | ||
| >>> from transformers import ImageProcessor, {model_class} |
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.
We have AutoImageProcessor. Any reason not to use it? Or it's just because that PR was merged after you started on this PR?
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.
Just a typo, I forgot the Auto here.
| @add_start_docstrings_to_model_forward(BERT_INPUTS_DOCSTRING.format("batch_size, sequence_length")) | ||
| @add_code_sample_docstrings( | ||
| processor_class=_TOKENIZER_FOR_DOC, | ||
| checkpoint=_CHECKPOINT_FOR_DOC, |
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.
not sure why this is not added say as the last argument and same question below
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.
Because there is no processor_class in the code sample used by this model anymore. It's just in the base model, and I have just left it there in case it's used by other modalities.
ArthurZucker
left a comment
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.
LGTM it simplifies a lot for addition of specific heads without checkpoints.
Are you planning on also updating previously added models ?
amyeroberts
left a comment
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 adding! Looks a lot tidier 🧹 🧹 🧹
LysandreJik
left a comment
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! Thanks!
What does this PR do?
This PR reworks the automatic code sample docstrings in two ways:
First, use the auto-classes for the preprocessing. As was decided internally, we want to document the model class used, but use the auto classes for preprocessing so users are not confused when a given model uses the tokenizer/feature extractor/image processor/processor of another.
Second, we don't want to showcase
hf-internal-testingmodels in the docstrings. Those are tiny random models and it confuses users more than it helps. However when using the standard checkpoint we get doctest problems, so this PR removes the output/loss from the code example when it shouldn't be tested.Two examples are shown with BERT and DeBERTaV2, I can add more models to the PR if it suits everyone.