Skip to content

Add missing ckpt in config docs #16900

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 25, 2022
Merged

Add missing ckpt in config docs #16900

merged 12 commits into from
Apr 25, 2022

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 22, 2022 •
edited
Loading

What does this PR do?

As discussed on Slack, I worked on the Config files to add missing information about checkpoints, or correct them.

  • I tried to check the mentioned checkpoints are actually on the Hub
  • also tried to make sure the checkpoints are for the target architecture
  • I didn't verify the statement Instantiating a configuration with the defaults will yield a similar configuration to that of the Speech2Text2 [mentioned checkpoint]
    • in particular, the hyperparameters like hidden_dim, num_layers might be different
    • it says similar, so I think it is fine (..?)

@patrickvonplaten Could you take a look on the speech models?
@NielsRogge Could you take a look on the vision models?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 22, 2022 •
edited
Loading

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

ydshieh
ydshieh commented Apr 25, 2022
View reviewed changes
src/transformers/models/speech_to_text_2/configuration_speech_to_text_2.py
@@ -31,7 +31,7 @@ class Speech2Text2Config(PretrainedConfig):
This is the configuration class to store the configuration of a [`Speech2Text2ForCausalLM`]. It is used to
instantiate an Speech2Text2 model according to the specified arguments, defining the model architecture.
Instantiating a configuration with the defaults will yield a similar configuration to that of the Speech2Text2
[facebook/s2t-small-librispeech-asr](https://huggingface.co/facebook/s2t-small-librispeech-asr) architecture.
[facebook/s2t-wav2vec2-large-en-de](https://huggingface.co/facebook/s2t-wav2vec2-large-en-de) architecture.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

facebook/s2t-small-librispeech-asr has model_type": "speech_to_text, but here is for the model Speech2Text2 (i.e. speech_to_text_2)

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

@ydshieh ydshieh marked this pull request as ready for review April 25, 2022 09:05
sgugger
sgugger approved these changes Apr 25, 2022
View reviewed changes
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 fixing all of those! It would be awesome to have some kind of quality script to check we don't introduce new faulty checkpoints.

NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 25, 2022

Thanks for fixing all of those! It would be awesome to have some kind of quality script to check we don't introduce new faulty checkpoints.

Yes, I do have some (draft) check locally. I plan to add it in another PR (unless it's necessary to do so in this PR).

NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
NielsRogge
NielsRogge reviewed Apr 25, 2022
View reviewed changes
@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 25, 2022

Thank you @NielsRogge I should try to use the correct names, as defined in MODEL_NAMES_MAPPING.

NielsRogge
NielsRogge approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@NielsRogge NielsRogge left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for this PR, awesome that this gets improved.

Left some comments, just for consistency, I would always use the template:

"will yield a similar configuration of that of the - snake-cased model name - checkpoint name architecture".

@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 25, 2022

Thanks a lot for this PR, awesome that this gets improved.

Left some comments, just for consistency, I would always use the template:

"will yield a similar configuration of that of the - snake-cased model name - checkpoint name architecture".

I will add this to the check I currently have (locally, but will push to another PR), thanks!

patrickvonplaten
patrickvonplaten approved these changes Apr 25, 2022
View reviewed changes
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.

Looked at all the speech models - looks good to me!

@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 25, 2022

Merge now. Thanks for the review.

With this PR, all configs are good except the following (which are expected, since those composite models don't have full default config arguments - they rely on the encoder and decoder configs.)

  • DecisionTransformerConfig
  • VisionEncoderDecoderConfig
  • VisionTextDualEncoderConfig
  • CLIPConfig
  • SpeechEncoderDecoderConfig
  • EncoderDecoderConfig
  • RagConfig

@ydshieh ydshieh merged commit 3e47d19 into huggingface:main Apr 25, 2022
@ydshieh ydshieh deleted the add_missing_ckpt_in_config_class_doc branch April 25, 2022 15:31
@ydshieh ydshieh mentioned this pull request Apr 29, 2022
Merged
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
@ydshieh @sgugger
@NielsRogge @elusenji
Add missing ckpt in config docs (huggingface#16900)
0501cfe 
* add missing ckpt in config docs

* add more missing ckpt in config docs

* fix wrong ckpts

* fix realm ckpt

* fix s2t2

* fix xlm_roberta ckpt

* Fix for deberta v2

* Apply suggestions from code review

Co-authored-by: Sylvain Gugger <[email protected]>

* use only one checkpoint for DPR

* Apply suggestions from code review

Co-authored-by: NielsRogge <[email protected]>

Co-authored-by: ydshieh <[email protected]>
Co-authored-by: Sylvain Gugger <[email protected]>
Co-authored-by: NielsRogge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NielsRogge NielsRogge NielsRogge approved these changes

+2 more reviewers

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

@sgugger sgugger sgugger approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ydshieh @HuggingFaceDocBuilderDev @patrickvonplaten @sgugger @NielsRogge