Roberta training crashing due to position_id embedding
Author: david-waterworth
Pull request: no

I've been trying to work out why I keep getting a CUDA assert in a specific mini-batch when training RoBERTa from scratch. I finally tracked it down after switching to CPU.

I don't understand why `padding_idx` is added to `incremental_indices` below? - _edit: I do in that the embedding needs a padding mask but it I'm not sure it's the correct way to do it._

In my case padding_idx=3. And one of my input_ids rows was truncated. Say I have input_ids = [[4,5,6],[4,3,3]], this results in mask=[[1,1,1],[1,0,0]] and incremental index=[[1,2,3],[1,0,0]]. Adding padding_idx then produces [[4,5,6],[4,3,3]].

The issue is `self.position_embeddings = nn.Embedding(config.max_position_embeddings, config.hidden_size)` so for any sequences which are truncated adding anything to the indices results in an index which is greater than the embedding dim.

Perhaps you can argue that max_position_embeddings is supposed to be larger than the largest possible sequence so this doesn't happen? There is a check in `run_mlm.py` that `data_args.max_seq_length > tokenizer.model_max_length` but it seems that in actual fact to avoid a very hard to track down error you must have tokenizer.model_max_length < data_args.max_seq_length

```
def create_position_ids_from_input_ids(input_ids, padding_idx):
    """
    Replace non-padding symbols with their position numbers. Position numbers begin at padding_idx+1. Padding symbols
    are ignored. This is modified from fairseq's `utils.make_positions`.

    Args:
        x: torch.Tensor x:

    Returns: torch.Tensor
    """
    # The series of casts and type-conversions here are carefully balanced to both work with ONNX export and XLA.
    mask = input_ids.ne(padding_idx).int()
    incremental_indices = torch.cumsum(mask, dim=1).type_as(mask) * mask
    return incremental_indices.long() + padding_idx
```

Comments:
One potential fix is to not use the same padding_idx for the position_ids embedding, why not just use 0? The least actual unpadded value of incremental_indices will be one so 0 is a valid pad.

In the `__init__` of `RobertaEmbeddings` (note this code is also duplicated, self.position_embeddings is intiialised twice!)

        self.position_embeddings = nn.Embedding(
            config.max_position_embeddings, config.hidden_size, padding_idx=0  # replaced self.padding_idx with 0
        )

And then modify create_position_ids_from_input_ids

```
def create_position_ids_from_input_ids(input_ids, padding_idx):
    """
    Replace non-padding symbols with their position numbers. Position numbers begin at padding_idx+1. Padding symbols
    are ignored. This is modified from fairseq's `utils.make_positions`.

    Args:
        x: torch.Tensor x:

    Returns: torch.Tensor
    """
    # The series of casts and type-conversions here are carefully balanced to both work with ONNX export and XLA.
    mask = input_ids.ne(padding_idx).int()
    incremental_indices = torch.cumsum(mask, dim=1).type_as(mask) * mask
    return incremental_indices.long() # removed + padding_idx
```

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/master/CONTRIBUTING.md) are likely to be ignored.