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

Map model_type and doc pages names #14944

Merged
merged 5 commits into from
Jan 3, 2022
Merged

Map model_type and doc pages names #14944

merged 5 commits into from
Jan 3, 2022

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Dec 27, 2021

What does this PR do?

As discussed on moon-landing, it's not possible to automatically generate the link to the documentation from the configuration of a model, due to multiple inconsistencies between model type and doc page name.

This PR fixes that and adds a script to make sure we have a model type for every documented model (for instance currently we have a mising model type for models having a tokenizer only, like Wav2Vec2-Phoeneme and they don't appear on the AutoTokenizer doc as a result) and that the doc page name matches the model type.

It breaks some links to the doc, but we (I and Julien) think this is acceptable.

@julien-c
Copy link
Member

very neat!

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Good for me! Thanks for cleaning this up

@LysandreJik
Copy link
Member

Sounds good to me. Will resolve conflicts and merge.


errors = []
for m in model_docs:
if m not in model_types and m != "auto":
Copy link
Member

Choose a reason for hiding this comment

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

@sgugger I've added this m != "auto" clause as otherwise the script would complain that auto was not in the MODEL_NAMES_MAPPING. I don't think adding auto there makes sense so I've added this workaround here.

Will merge like this to prevent further conflicts, but open to discussion if you think another solution should have been implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, thanks for fixing!

@LysandreJik LysandreJik merged commit 8f6373c into master Jan 3, 2022
@LysandreJik LysandreJik deleted the doc_nodel_type branch January 3, 2022 10:08
stevhliu pushed a commit to stevhliu/transformers that referenced this pull request Jan 6, 2022
* Map model_type and doc pages names

* Add script

* Fix typo

* Quality

* Manual check for Auto

Co-authored-by: Lysandre <lysandre.debut@reseau.eseo.fr>
Albertobegue pushed a commit to Albertobegue/transformers that referenced this pull request Jan 27, 2022
* Map model_type and doc pages names

* Add script

* Fix typo

* Quality

* Manual check for Auto

Co-authored-by: Lysandre <lysandre.debut@reseau.eseo.fr>
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