Fix loss calculation in TFXXXForTokenClassification models
Author: ydshieh
Pull request: yes

# What does this PR do?

The current loss calculation in `TFFunnelForTokenClassification` (and other TF token classification models) don't align with the loss in `FunnelForTokenClassification`

TF
https://github.com/huggingface/transformers/blob/6ac77534bfe97c00e0127bb4fc846ae0faf1c9c5/src/transformers/models/funnel/modeling_tf_funnel.py#L1709

PT
https://github.com/huggingface/transformers/blob/6ac77534bfe97c00e0127bb4fc846ae0faf1c9c5/src/transformers/models/funnel/modeling_funnel.py#L1470-L1481

(which further uses the `attention_mask`).

This PR aims to fix this. Currently only `TFFunnelForTokenClassification` is fixed. I would like to have some feedback first before fixing all involved models.

## More information 1

Check the loss difference between PT/TF version 300 times,

For `(TF)BertForTokenClassification`
```
without this PR:

  max_diff:   0.022978603839874268
  mean_diff: 0.006492746472358704

with this PR (applied locally):

  max_diff:   1.7881393432617188e-07
  mean_diff: 3.3974647521972653e-08
```

For `(TF)FunnelForTokenClassification`
```
without this PR:

  max_diff:   0.2821081280708313
  mean_diff: 0.05486685564120611

with this PR:

  max_diff:   3.5762786865234375e-07
  mean_diff: 6.020069122314453e-08
```

## More information 2

Current transformers's version doesn't have a PT-TF test that tests the equivalence in the cases where `labels` is passed to the models. In PR #15256, such a test is introduced (to make sure more PT/TF equivalence). The inconsistency of loss calculation in PT/TF `XXXForTokenClassification` models requires the error tolerance to be not smaller than `0.2` in order to pass the test (and this doesn't even guarantee).

https://github.com/huggingface/transformers/blob/6134cc69527aa9df9a4f2bc9e545222456a34524/tests/test_modeling_tf_common.py#L413-L441

It would be better to address this inconsistency, and lower that threshold once this PR is merged.

Comments:
_The documentation is not available anymore as the PR was closed or merged._

> I think in this instance it's the PyTorch model that is incorrect, and it should use the -100s in the labels instead of the attention mask to determine the logits/labels to ignore.

I have the same thought on the role/responsibility of `labels`, but was too afraid to change the PyTorch codes.
I can do it the other way, but the same situation happens for `BertForTokenClassification`.
It would be great if HF can confirm this change (for PyTorch models) is welcomed.


This is a breaking change in case the attention mask does not align with the `-100` labels, but I think that's okay.

The token classification models are the only models that have this feature, which (AFAICT) isn't mentioned in the docs anywhere, so imo it isn't a fully supported feature and it's fine to remove it. 

Would also appreciate @patrickvonplaten's comment.

There are also `FunnelForPreTraining` and `ElectraForPreTraining` which use attention_mask

```
            if attention_mask is not None:
                active_loss = attention_mask.view(-1, discriminator_sequence_output.shape[1]) == 1
                active_logits = logits.view(-1, discriminator_sequence_output.shape[1])[active_loss]
                active_labels = labels[active_loss]
                loss = loss_fct(active_logits, active_labels.float())
```
(I saw it during the search for the last commit)

`BertForPreTraining` doesn't use it for loss in the code

```
        total_loss = None
        if labels is not None and next_sentence_label is not None:
            loss_fct = CrossEntropyLoss()
            masked_lm_loss = loss_fct(prediction_scores.view(-1, self.config.vocab_size), labels.view(-1))
            next_sentence_loss = loss_fct(seq_relationship_score.view(-1, 2), next_sentence_label.view(-1))
            total_loss = masked_lm_loss + next_sentence_loss
```


Before merging, would it be better to add a test? There is a more general test in #15256 (not merged yet, see below), which will fail (due to other PT/TF differences).

I can add that test in this PR, but restrict to `...ForTokenClassification` models here. The test can be made more general along the way of fixing other PT/TF differences.

https://github.com/huggingface/transformers/blob/47a5d26b58afbfae2366328fe4242de9bb662fcb/tests/test_modeling_tf_common.py#L414-L461

Well, `FunnelForPreTraining` and `ElectraForPreTraining` use binary classification for pretraining objective.

```
            - 0 indicates the token is an original token,
            - 1 indicates the token was replaced.
```
and `nn.BCEWithLogitsLoss` doesn't support `ignore_index`.

The best one can do it probably just to make `TFFunnelForPreTraining` and `TFElectraForPreTraining` return loss (currently, they don't compute loss)

(or open a PR in PyTorch repo to modify it ...😲)

Also cc @patil-suraj 

I think what @sgugger said makes sense,  https://github.com/huggingface/transformers/pull/15294#discussion_r794484953, and considering https://github.com/huggingface/transformers/pull/15294#discussion_r794507876, I think I won't have any action for now until some more feedback from HF side (once you get sometime :-) )

Reverted the change to research projects / Tests good / Ready to go :-) / Thanks!

Thank *you* for your contribution :-)