Replace scatter operations in TAPAS by native PyTorch
Author: NielsRogge
Pull request: no
Labels: name: Good First Issue

name: Good Second Issue

### Feature request

TAPAS (my first 🤗 contribution 😄 ) still relies on the [torch_scatter](https://github.com/rusty1s/pytorch_scatter) library, as the model uses some scatter operations on tensors. Back then PyTorch didn't have these operations available.

Now they have: https://pytorch.org/docs/stable/generated/torch.Tensor.scatter_.html#torch.Tensor.scatter_.

So we should replace [this line](https://github.com/huggingface/transformers/blob/b8112eddecfd524038e3c10970c06a444a32aa9d/src/transformers/models/tapas/modeling_tapas.py#L1800) by native PyTorch.

To confirm everything is working fine, one should run the following tests and make sure they pass (to be run from the root of this repository):

```
RUN_SLOW=yes pytest tests/models/tapas/test_modeling_tapas.py
```
Subsequently, all `is_scatter_available` mentions can be removed from the code base.

### Motivation

By replacing this, our TAPAS implementation doesn't rely on a third-party library anymore.

### Your contribution

I can look into this, but marking it as a good first/second issue.

Update: I have an attempt here: https://github.com/huggingface/transformers/compare/main...NielsRogge:transformers:fix_tapas_scatter?expand=1.

However, training tests didn't pass due to an issue in the backward pass.

Comments:
@NielsRogge I would love to pick up this issue from where you left if that is okay for you

Awesome, feel free to take over my branch and see whether you can make all tests pass

@Bearnardd Thank you!

You can start from this (updated) branch/PR (which is updated with the `main` branch with a tiny fix).
If you want to start from @NielsRogge branch, you have to rebase on (updated) `main` first - there will be a few conflicts to resolve.

https://github.com/huggingface/transformers/pull/20107
https://github.com/huggingface/transformers/tree/fix_tapas_scatter


@ydshieh Thanks for the guidance! I will start working on this problem after the work :)

Hi @NielsRogge - I have done a bit of debugging today and I have found the following roots of failing tests:

1. `tests/models/tapas/test_modeling_tapas.py::TapasUtilitiesTest::test_reduce_sum_vectorized` fails because in pytorch version of `scatter_reduce` the `src` and `index` tensors are required to have the same number of dimensions which is not the case in the above test.
2. `tests/models/tapas/test_modeling_tapas.py::TapasModelTest::test_training` fails in backward pass with the following error: `RuntimeError: one of the variables needed for gradient computation has been modified by an inplace operation` which is happening because inside `segment_reduce` function we are making a view of the `segment_means` variable (which does not copy it) which results in modification in place. Simply cloning segment_means before viewing it seems to be working but I do not know what is your policy about cloning tensors in regard to memory management

Hi @Bearnardd - that's awesome! Thanks a lot for looking into it.

Regarding point 1 - you can update that test accordingly to account for the PyTorch version of `scatter_reduce`.

Regarding point 2 - you can clone the tensor and open a PR, we can discuss it there ;)

Hi @NielsRogge - I guess this issue should be closed?

Yes, thanks for reminding us :-) @Bearnardd 

Closed by #20149