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

Generation doc #6470

Merged
merged 14 commits into from
Aug 14, 2020
Merged

Generation doc #6470

merged 14 commits into from
Aug 14, 2020

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Aug 13, 2020

Add documentation (and clean docstrings) of GenerationMixin and TFGenerationMixin.

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #6470 into master will decrease coverage by 0.17%.
The diff coverage is 96.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6470      +/-   ##
==========================================
- Coverage   80.55%   80.37%   -0.18%     
==========================================
  Files         153      156       +3     
  Lines       28001    28058      +57     
==========================================
- Hits        22556    22552       -4     
- Misses       5445     5506      +61     
Impacted Files Coverage Δ
src/transformers/configuration_bart.py 93.87% <ø> (-0.36%) ⬇️
src/transformers/configuration_utils.py 96.59% <ø> (ø)
src/transformers/generation_tf_utils.py 86.46% <ø> (ø)
src/transformers/tokenization_bart.py 100.00% <ø> (+4.05%) ⬆️
src/transformers/modeling_encoder_decoder.py 91.66% <87.50%> (+0.64%) ⬆️
src/transformers/tokenization_mbart.py 95.31% <95.31%> (ø)
src/transformers/modeling_gpt2.py 86.68% <97.87%> (+0.71%) ⬆️
src/transformers/__init__.py 99.28% <100.00%> (+0.01%) ⬆️
src/transformers/configuration_auto.py 93.47% <100.00%> (+0.14%) ⬆️
src/transformers/configuration_mbart.py 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05810cd...d9cbc03. Read the comment docs.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

Comment on lines 107 to 110
bos_token_id (:obj:`int`, `optional`):
The id of the `beginning-of-stream` token.
eos_token_id (:obj:`int`, `optional`):
The id of the `end-of-stream` token.
Copy link
Member

Choose a reason for hiding this comment

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

beginning-of-sequence and end-of-sequence?

src/transformers/generation_tf_utils.py Outdated Show resolved Hide resolved
src/transformers/generation_utils.py Outdated Show resolved Hide resolved
patil-suraj and others added 3 commits August 14, 2020 03:21
* add MBartForConditionalGeneration

* style

* rebase and fixes

* add mbart test in TEST_FILES_WITH_NO_COMMON_TESTS

* fix docs

* don't ignore mbart

* doc

* fix mbart fairseq link

* put mbart before bart

* apply doc suggestions
* Use hash to clean the test dirs

* Use hash to clean the test dirs

* Use hash to clean the test dirs

* fix
* add cross attention layers for gpt2

* make gpt2 cross attention work

* finish bert2gpt2

* add explicit comments

* remove attention mask since not yet supported

* revert attn mask in pipeline

* Update src/transformers/modeling_gpt2.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

* Update src/transformers/modeling_encoder_decoder.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Adapted in part from `Facebook's XLM beam search code`_.
r"""
Generates sequences for models with a language modeling head. The method currently supports greedy decoding,
Copy link
Contributor

Choose a reason for hiding this comment

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

we could mayb add here that this applies to all AUTO_MODEL_FOR_CAUSAL_LM and AUTO_MODEL_FOR _SEQ2SEQ -> not sure though whether this is very useful for the user

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are not documented so it's not really useful inside the documentation.

top_p (:obj:`float`, `optional`, defaults to 1.0):
If set to float < 1, only the most probable tokens with probabilities that add up to ``top_p`` or
higher are kept for generation.
repetition_penalty (:obj:`float`, `optional`, defaults to 1.0):
Copy link
Contributor

@patrickvonplaten patrickvonplaten Aug 14, 2020

Choose a reason for hiding this comment

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

Maybe we can link even to the original paper here, where it is explained: https://arxiv.org/pdf/1909.05858.pdf - page 5 first equation, since it might be hard to understand what the penalty does exactly. But maybe this goes to far here...most of these params are explained here: https://huggingface.co/blog/how-to-generate maybe we can add a link above "Parameters".

eos_token_id (:obj:`int`, `optional`):
The id of the `end-of-stream` token.
length_penalty (:obj:`float`, `optional`, defaults to 1.0):
Exponential penalty to the length. 1.0 means no penalty.
Copy link
Contributor

@patrickvonplaten patrickvonplaten Aug 14, 2020

Choose a reason for hiding this comment

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

this param can be confusing as a penalty < 1.0 penalizes length whereas a penalty > 1.0 encourages longer sequences. Maybe we can add a sentence like:
Note: In order to encourage the model to generate shorter sequences, `length_penalty` should be set < 1.0. In order to encourage the model to produce longer sequences, the parameter should be set to > 1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #4915

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.

Looks great!

Would add one sentence for length_penalty to avoid confusion as noted down in this issue: #4915

@sgugger sgugger merged commit 895ed8f into master Aug 14, 2020
@sgugger sgugger deleted the generate_doc branch August 14, 2020 13:46
fabiocapsouza added a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020
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.

None yet

7 participants