[WIP] Warning when passing padded input ids but no attention mask
Author: patrickvonplaten
Pull request: yes
Labels: name: WIP

# What does this PR do?

<!--
Congratulations! You've made it this far! You're not quite done yet though.

Once merged, your PR is going to appear in the release notes with the title you set, so make sure it's a great title that fully reflects the extent of your awesome contribution.

Then, please replace this with a description of the change and which issue is fixed (if applicable). Please also include relevant motivation and context. List any dependencies (if any) that are required for this change.

Once you're done, someone will review your PR shortly (see the section "Who can review?" below to tag some potential reviewers). They may suggest changes to make the code even better. If no one reviewed your PR after a week has passed, don't hesitate to post a new comment @-mentioning the same persons---sometimes notifications get lost.
-->

<!-- Remove if not applicable -->

One of the most common mistake users make in Transformers IMO is that `input_ids` are padded, but no `attention_mask` is provided (we see many examples of this). As discussed multiple times, we **don't** want to infer the `attention_mask` automatically as this creates a lot of unmaintainable, "not-possible-to-deal-with" complexity.

A while ago, we discussed to throw a warning in this case, making sure it's done only once to not spam the user when calling the model multiple times. I'm not sure we found a good conclusion, but IMO it's important that we warn the user as too users (IMO) think the attention_mask is inferred from the padding tokens. This PR is tries to solve this and shows how it'd be implemented for just BERT. We would have to implement it for all other models then as well. Would very much like to hear your opinion here @sgugger @LysandreJik @patil-suraj  . Note that this PR will touch a lot of important functions / files, so it'd be very important to make the warning as clear as possible. 
I do however have a strong conviction that we should display such a warning.

No the warning function can display the following warning messages for a toy BERT example of passing just three input ids.

Possible warning messages:

1. Pad token present, no attention mask, eos, bos, sep all different from pad (that's **VERY** likely an error IMO):

**Displayed warning:**
```
The input IDs tensor([[0, 1, 1]]) contains the `pad_token_id` 0, but NO `attention_mask` is passed.
Padding the input IDs without passing an `attention_mask` leads to unexpected, possibly incorrect outputs.
```

2. Pad token present, no attention mask, eos or bos or sep same as pad:

**Displayed warning:**
```
The input IDs tensor([[0, 1, 1]]) contains the `pad_token_id` 0, but NO `attention_mask` is passed.
We strongly recommend passing an `attention_mask` to avoid possibly incorrectly computing the attention weights. 
You can ignore this warning, if your `pad_token_id` 0 is identical to your `sep_token_id` 0 AND your input is NOT padded.
```

3. Pad token present, no attention mask, two or more of eos, bos, sep identical to pad (don't think this exists actually):

**Displayed warning:**
```
The input IDs tensor([[0, 1, 1]]) contains the `pad_token_id` 0, but NO `attention_mask` is passed.
We strongly recommend passing an `attention_mask` to avoid possibly incorrectly computing the attention weights. 
You can ignore this warning, if your `pad_token_id` 0 is identical to your `bos_token_id` 0 AND your input is NOT padded.
We strongly recommend passing an `attention_mask` to avoid possibly incorrectly computing the attention weights. 
You can ignore this warning, if your `pad_token_id` 0 is identical to your `sep_token_id` 0 AND your input is NOT padded.
```

4. Otherwise no warning.

Also note that the warning only appears at the first forward call.

## 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](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? Please add a link
      to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes? Here are the
      [documentation guidelines](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] Did you write any new necessary tests?


## 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.

<!-- Your PR will be replied to more quickly if you can figure out the right person to tag with @

 If you know how to use git blame, that is the easiest way, otherwise, here is a rough guide of **who to tag**.
 Please tag fewer than 3 people.

Models:

- albert, bert, xlm: @LysandreJik
- blenderbot, bart, marian, pegasus, encoderdecoder,  t5: @patrickvonplaten, @patil-suraj
- longformer, reformer, transfoxl, xlnet: @patrickvonplaten
- fsmt: @stas00
- funnel: @sgugger
- gpt2: @patrickvonplaten, @LysandreJik
- rag: @patrickvonplaten, @lhoestq
- tensorflow: @LysandreJik

Library:

- benchmarks: @patrickvonplaten
- deepspeed: @stas00
- ray/raytune: @richardliaw, @amogkam
- text generation: @patrickvonplaten
- tokenizers: @n1t0, @LysandreJik
- trainer: @sgugger
- pipelines: @LysandreJik

Documentation: @sgugger

HF projects:

- datasets: [different repo](https://github.com/huggingface/datasets)
- rust tokenizers: [different repo](https://github.com/huggingface/tokenizers)

Examples:

- maintained examples (not research project or legacy): @sgugger, @patil-suraj
- research_projects/bert-loses-patience: @JetRunner
- research_projects/distillation: @VictorSanh

 -->


Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_17444). All of your documentation changes will be reflected on that endpoint.

Relevant issues:
https://github.com/huggingface/transformers/issues/4083
https://github.com/huggingface/transformers/issues/278
https://github.com/huggingface/transformers/issues/16136

I think the way you implemented it is clean and adds nice warnings. I agree with the idea behind it, and the better warnings we send, the better the models will perform for users.

I think handling it like it is done here based off of configuration attribute is not going to work very well across models, however. I feel like having the method be configurable by passing optional bos/eos tokens would likely make the method more versatile to the models which do not conform to the default approach.

> I think handling it like it is done here based off of configuration attribute is not going to work very well across models, however. I feel like having the method be configurable by passing optional bos/eos tokens would likely make the method more versatile to the models which do not conform to the default approach.

Hmm, don't really agree here. Note that `pad_token_id`, `bos_token_id`, `eos_token_id`, `sep_token_id` **must** be present in every model's config since it's in `configuration_utils.py`. 
Also we never pass any of the above attributes through the forward method, so one would only ever pass `self.config.pad_token_id` to the method. Wdyt @LysandreJik ? Also very curious to hear @sgugger's opinion here

Sounds good, I'm likely worrying for nothing then. Good for me like this, very easy to add kwargs afterwards anyway!

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

I think this would be an impactful addition! @ydshieh, would you be interested in continuing this PR?

> I think this would be an impactful addition! @ydshieh, would you be interested in continuing this PR?

Sure. I will take a look and see if there is anything blocking. 

You can search `elif input_ids is not None:` that is in the base model classes like `BertModel` (already done by @patrickvonplaten), `GPT2Model` etc.

You don't need to replace all of them - it would be super nice already for a few of the most used modes 🚀  Thank you!