Difference in BART position embeddings with fairseq implementation
Author: gchhablani
Pull request: no

I'm trying to implement PLBART (#13269) which is majorly just a BART model with some minor modifications. I tried to make embeddings equal but there is a mismatch in original BART (fairseq) and current BART implementation.

In the [original code](https://github.com/pytorch/fairseq/blob/9549e7f76994095c92441b81c615a169dc21f478/fairseq/modules/learned_positional_embedding.py#L31), the learned position embeddings is based on the `input_tokens`:

```python
def forward(
    self,
    input: Tensor,
    incremental_state: Optional[Dict[str, Dict[str, Optional[Tensor]]]] = None,
    positions: Optional[Tensor] = None,
):
    """Input is expected to be of size [bsz x seqlen]."""
    assert (positions is None) or (
        self.padding_idx is None
    ), "If positions is pre-computed then padding_idx should not be set."

    if positions is None:
        if incremental_state is not None:
            # positions is the same for every token when decoding a single step
            # Without the int() cast, it doesn't work in some cases when exporting to ONNX
            positions = torch.zeros(
                (1, 1), device=input.device, dtype=input.dtype
            ).fill_(int(self.padding_idx + input.size(1)))
        else:
            positions = utils.make_positions(
                input, self.padding_idx, onnx_trace=self.onnx_trace
            )
    return F.embedding(
        positions,
        self.weight,
        self.padding_idx,
        self.max_norm,
        self.norm_type,
        self.scale_grad_by_freq,
        self.sparse,
    )
```

and `utils.make_positions` does:

```python
def make_positions(tensor, padding_idx: int, onnx_trace: bool = False):
    """Replace non-padding symbols with their position numbers.
    Position numbers begin at padding_idx+1. Padding symbols are ignored.
    """
    # The series of casts and type-conversions here are carefully
    # balanced to both work with ONNX export and XLA. In particular XLA
    # prefers ints, cumsum defaults to output longs, and ONNX doesn't know
    # how to handle the dtype kwarg in cumsum.
    mask = tensor.ne(padding_idx).int()
    return (torch.cumsum(mask, dim=1).type_as(mask) * mask).long() + padding_idx
```

In HuggingFace implementation, the learned position embeddings is based on the `input_shape`:

```python
def forward(self, input_ids_shape: torch.Size, past_key_values_length: int = 0):
        """`input_ids_shape` is expected to be [bsz x seqlen]."""
        bsz, seq_len = input_ids_shape[:2]
        positions = torch.arange(
            past_key_values_length, past_key_values_length + seq_len, dtype=torch.long, device=self.weight.device
        )
        return super().forward(positions + self.offset)
```

The difference shows up when considering `padding_idx`. In HuggingFace implementation, `padding_idx` is considered same as any other token for positions, so it will get it's own position id (because we only care about shape). So `<pad>` at third position will get `4`, and `<pad>` at tenth position will get `11`. In the original impementation, `padding_idx` is considered different from other tokens, and the position for that, the `padding_idx` is used explicity as position. So `<pad>`'s corresponding position id will always be `1`, regardless of the position.

Note that this does not bring any changes to the model output for other tokens, as the `attention_mask` ignores the padding index in both cases.

Do you think this needs fixing to make it as close to the original implementation as possible? Since they are also using `torch.nn.Embedding`, I think it should be easy. Is there any other reason behind the way things have been implemented currently?

Thank you,
Gunjan

Comments:
Hey Gunjan :-)

I don't think that this needs fixing because (as you said) the padding token is masked anyways

Hi @patrickvonplaten :) 
Thanks a lot!