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.

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

Improve encoder decoder model docs #17815

Merged
merged 10 commits into from
Jun 24, 2022
Merged

Improve encoder decoder model docs #17815

merged 10 commits into from
Jun 24, 2022

Conversation

Threepointone4
Copy link
Contributor

What does this PR do?

This PR improves the documentation of encoder decoder model.

Fixes # (issue)

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.
    Issues link

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@patrickvonplaten

@Threepointone4 Threepointone4 mentioned this pull request Jun 22, 2022
Closed
3 tasks
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 22, 2022 •
edited
Loading

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

NielsRogge
NielsRogge reviewed Jun 22, 2022
View reviewed changes
docs/source/en/model_doc/encoder-decoder.mdx
>>> model = EncoderDecoderModel(config=config)
```

## Initialising [`EncoderDecoderModel`] from a pretrained encoder and a pretrained decoder.
Copy link
Contributor

Choose a reason for hiding this comment

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

Above you use initializing, here initialising.

NielsRogge
NielsRogge approved these changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@NielsRogge NielsRogge 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 improving this! Would be great to also improve the docs of VisionEncoderDecoderModel and SpeechEncoderDecoderModel.

NielsRogge
NielsRogge reviewed Jun 22, 2022
View reviewed changes
docs/source/en/model_doc/encoder-decoder.mdx
>>> # the forward function automatically creates the correct decoder_input_ids
>>> loss = model(input_ids=input_ids, labels=labels).loss
```
Detailed [colab](https://colab.research.google.com/drive/1WIk2bxglElfZewOHboPFNj8H44_VAyKE?usp=sharing#scrollTo=ZwQIEhKOrJpl) for training.
Copy link
Contributor

Choose a reason for hiding this comment

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

This notebook might already be outdated, cc @patrickvonplaten.

Also cc @ydshieh as we were planning on writing a blog post about them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NielsRogge should I remove the Colab link for now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think ok to leave it for now :-)

ydshieh
ydshieh reviewed Jun 22, 2022
View reviewed changes
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you for the improvement, @Threepointone4 !

docs/source/en/model_doc/encoder-decoder.mdx Outdated Show resolved Hide resolved
docs/source/en/model_doc/encoder-decoder.mdx
## Initialising [`EncoderDecoderModel`] from a pretrained encoder and a pretrained decoder.

[`EncoderDecoderModel`] can be initialized from a pretrained encoder checkpoint and a pretrained decoder checkpoint. Note that any pretrained auto-encoding model, *e.g.* BERT, can serve as the encoder and both pretrained auto-encoding models, *e.g.* BERT, pretrained causal language models, *e.g.* GPT2, as well as the pretrained decoder part of sequence-to-sequence models, *e.g.* decoder of BART, can be used as the decoder.
Depending on which architecture you choose as the decoder, the cross-attention layers might be randomly initialized.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be more careful to say the auto-encoding models that provide causal LM implementation.

Also the sentence is super long, it might be a good idea to split it.

ydshieh
ydshieh reviewed Jun 22, 2022
View reviewed changes
docs/source/en/model_doc/encoder-decoder.mdx
... return_tensors="pt",
... ).input_ids

>>> labels = tokenizer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) it might be a good idea to add >>> # Let's use a summary of the above text as the target

@patrickvonplaten
Copy link
Contributor

Agree with the reviews of @ydshieh and @NielsRogge ! @Threepointone4 do you want to apply them ? Think we can merge after :-)

Threepointone4 and others added 7 commits June 23, 2022 18:35
patrickvonplaten
patrickvonplaten approved these changes Jun 24, 2022
View reviewed changes
docs/source/en/model_doc/encoder-decoder.mdx
>>> # the forward function automatically creates the correct decoder_input_ids
>>> loss = model(input_ids=input_ids, labels=labels).loss
```
Detailed [colab](https://colab.research.google.com/drive/1WIk2bxglElfZewOHboPFNj8H44_VAyKE?usp=sharing#scrollTo=ZwQIEhKOrJpl) for training.
Copy link
Contributor

Choose a reason for hiding this comment

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

Think ok to leave it for now :-)

@patrickvonplaten
Copy link
Contributor

Great job @Threepointone4 ! Merging :-)

@patrickvonplaten patrickvonplaten merged commit c2c0d9d into huggingface:main Jun 24, 2022
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 25, 2022
@Threepointone4 @NielsRogge
@ydshieh @younesbelkada
Improve encoder decoder model docs (huggingface#17815)
fd0962b 
* Copied all the changes from the last PR

* added in documentation_tests.txt

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

Co-authored-by: Yih-Dar <[email protected]>

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

Co-authored-by: vishwaspai <[email protected]>
Co-authored-by: NielsRogge <[email protected]>
Co-authored-by: Yih-Dar <[email protected]>
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 29, 2022
@Threepointone4 @NielsRogge
@ydshieh @younesbelkada
Improve encoder decoder model docs (huggingface#17815)
fb320cf 
* Copied all the changes from the last PR

* added in documentation_tests.txt

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

Co-authored-by: Yih-Dar <[email protected]>

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

* Update docs/source/en/model_doc/encoder-decoder.mdx

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

Co-authored-by: vishwaspai <[email protected]>
Co-authored-by: NielsRogge <[email protected]>
Co-authored-by: Yih-Dar <[email protected]>
@NielsRogge NielsRogge mentioned this pull request Jul 23, 2022
Merged
@lappemic lappemic mentioned this pull request May 13, 2024
Open
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

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

@ydshieh ydshieh ydshieh approved these changes

Assignees

@patrickvonplaten patrickvonplaten

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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