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

Make docstring match args #4711

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Make docstring match args #4711

merged 1 commit into from
Jun 1, 2020

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Jun 1, 2020

When replying to #4698, I realized some language model docstrings are using arguments that are not present in the function signature. This PR addresses that (for all the ones I found at least).

The alternative would be to change the argument names in the function signatures (if it makes the various model APIs more consistent).

@codecov-commenter
Copy link

Codecov Report

Merging #4711 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4711      +/-   ##
==========================================
- Coverage   77.32%   77.14%   -0.19%     
==========================================
  Files         128      128              
  Lines       21071    21071              
==========================================
- Hits        16294    16256      -38     
- Misses       4777     4815      +38     
Impacted Files Coverage Δ
src/transformers/modeling_bart.py 96.79% <ø> (ø)
src/transformers/modeling_gpt2.py 72.11% <ø> (-14.11%) ⬇️
src/transformers/modeling_openai.py 80.41% <ø> (-1.38%) ⬇️
src/transformers/modeling_transfo_xl.py 77.00% <ø> (ø)
src/transformers/modeling_xlm.py 89.46% <ø> (ø)
src/transformers/modeling_utils.py 89.94% <0.00%> (-0.24%) ⬇️
src/transformers/file_utils.py 73.85% <0.00%> (+0.41%) ⬆️
src/transformers/modeling_tf_utils.py 88.66% <0.00%> (+1.80%) ⬆️

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 6449c49...e7d6cb9. 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.

Nice, thanks @sgugger ! At some point we would like to have labels on every model, rather than labels on some, masked_lm_labels on others and lm_labels on others. We would need to deprecate the current lm_labels and masked_lm_labels while keeping them in the signature to keep backwards compatibility.

For reference: #4055, #4198 (comment) (cc @thomwolf, @patrickvonplaten, @julien-c)

@LysandreJik LysandreJik merged commit 7677936 into huggingface:master Jun 1, 2020
@sgugger
Copy link
Collaborator Author

sgugger commented Jun 1, 2020

I can work on this if there is no one on it. Quick question though: what about the models that have both lm_labels and masked_lm_labels? encode_decoder is one of them for instance, don't know if there are more.

@sgugger sgugger deleted the doc_typos branch June 1, 2020 19:26
@LysandreJik
Copy link
Member

Yes, that's the case for BertForMaskedLM for example. I don't really know the best way to handle this.

As with this update we're trying to have the exact same API for all models so that the training/inference code is model agnostic, I'd say that we should look for the most natural on a case-by-case basis.

For example with the BertForMaskedLM example, I believe the labels should be the masked_lm_labels, as BERT should be used for MLM rather than CLM.

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Jun 2, 2020

the

As far as I know, BertForMaskedLM does not really use lm_labels at the moment. I think it was added to support a causal Bert in an encoder-decoder setting so that the decoder can be trained with a causal mask with the language model objective. Since the encoder-decoder framework is not really released yet, I think we can also add a new BertWithLMHead class so that each class only has one labels argument. It would be a breaking change in terms of the class name for people that already implemented Bert2Bert models, but I think it's worth it for consistency. What do you think?

@sgugger - In the encoder-decoder model I added both lm_labels and masked_lm_labels because Bert has both lm_labels and masked_lm_labels. Normally, encoder-decoder models are trained with a CLM objective so not sure if we even need masked_lm_lables for the encoder-decoder model wrapper.

@thomwolf
Copy link
Member

thomwolf commented Jun 2, 2020

@patrickvonplaten good for me

@sgugger sgugger mentioned this pull request Jun 2, 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.

5 participants