Unexpected padding behaviour of `ClapFeatureExtractor`
Author: anmol-wiai
Pull request: no
Labels: name: Good Difficult Issue

### System Info

- `transformers` version: 4.29.2
- Platform: Linux-4.15.0-204-generic-x86_64-with-glibc2.31
- Python version: 3.10.10
- Huggingface_hub version: 0.14.1
- Safetensors version: not installed
- PyTorch version (GPU?): 1.13.1+cu117 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: Yes
- Using distributed or parallel set-up in script?: No

### Who can help?

@ArthurZucker

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [X] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

Adding `padding=True` argument to `ClapFeatureExtractor` changes the padding strategy from `repeatpad`, which is the default, to constant padding ([code](https://github.com/huggingface/transformers/blob/04ab5605fbb4ef207b10bf2772d88c53fc242e83/src/transformers/models/clap/feature_extraction_clap.py#L248)).

Code adapted from: https://huggingface.co/docs/transformers/model_doc/clap#transformers.ClapModel.forward.example

```python
from datasets import load_dataset
from transformers import AutoProcessor

# load data
dataset = load_dataset("ashraq/esc50")
audio_sample = dataset["train"]["audio"][0]["array"]

# load data processor
processor = AutoProcessor.from_pretrained("laion/clap-htsat-unfused")

# pre-process data
inputs1 = processor.feature_extractor(audio_sample, return_tensors="pt")
inputs2 = processor.feature_extractor(audio_sample, return_tensors="pt", padding=True)

print((inputs1["input_features"] - inputs2["input_features"]).max())
# Output: tensor(119.4260)
```

This becomes a problem, for instance, when using `ClapProcessor`. `ClapProcessor` shares `kwargs` between the `tokenizer` and the `feature_extractor` ([code](https://github.com/huggingface/transformers/blob/04ab5605fbb4ef207b10bf2772d88c53fc242e83/src/transformers/models/clap/processing_clap.py#LL87C9-L87C9)). When using text inputs of different length, you need to pass `padding=True` argument to the `tokenizer`, but doing so changes the behaviour of the `feature_extractor`.

### Expected behavior

1. Either don't allow `padding=True` argument. Assert its value to be one of the allowed values - `repeatpad`, `repeat`, and `pad` in case of `ClapFeatureExtractor`.
2. Or, use the default padding strategy if `padding=True`.

As for sharing `kwargs`, I don't think that's a good idea. Would having two arguments, `tokenizer_kwargs` and `feature_extractor_kwargs` be better? 

Comments:
cc @sanchit-gandhi 

Thanks for the clean write-up @anmol-wiai! 

I think splitting arguments make sense for the processor classes (send one set of args to the feature extractor, another set of args to the tokeniser). Previously, I overwrote the most common feature extractor arg to fall out of the shared arg logic: https://github.com/huggingface/transformers/pull/20022 But clearly we need something robust to handle the different sets of possible inputs args for the feature extractor and tokeniser respectively

Probably what we need to do here to prevent breaking changes is have three input arguments:
1. `feature_extractor_kwargs` -> get sent to the feature extractor
2. `tokenizer_kwargs` -> get sent to the tokeniser
3. `kwargs` -> get sent to both (needed to prevent a breaking change)

WDYT about this design @amyeroberts @anmol-wiai?

Hi @sanchit-gandhi,

> Probably what we need to do here to prevent breaking changes is have three input arguments:
> 
> feature_extractor_kwargs -> get sent to the feature extractor
> tokenizer_kwargs -> get sent to the tokeniser
> kwargs -> get sent to both (needed to prevent a breaking change)
>  
I think this is good. One small thing is that `kwargs` can be a little ambiguous. 

If I have two functions like this:
```python
# version 1
def run_func1_and_func2_v1(func1_arg, func2_arg, **kwargs):
    ...

# version 2
def run_func1_and_func2_v2(func1_arg, func2_arg, func1_kwargs, func2_kwargs, **kwargs):
    ...
```
While it's expected than in version 1, `kwargs` could be passed on to subsequent function calls inside `run_func1_and_func2_v1`, is it so obvious in version 2 as well given that `run_func1_and_func2_v2` already has `func1_kwargs` and `func2_kwargs` arguments? 
Would renaming `kwargs` to something like `shared_kwargs`, or `common_kwargs` be more clear? In any case, you should consider deprecating `kwargs`.

---

Another issue that I flagged is about what should `ClapFeatureExtractor` do when it is passed `padding=True` argument. (Sorry for raising two things in a single issue. I got a little confused. I can open a separate issue if that helps.)

If you look at how `padding` argument works for `tokenizer` (code around [this](https://github.com/huggingface/transformers/blob/04ab5605fbb4ef207b10bf2772d88c53fc242e83/src/transformers/tokenization_utils_base.py#LL2366C1-L2366C1) line), it looks something like:
```python
if padding is not False:
    if padding is True:
        # use default strategy - LONGEST
        ...
    elif not isinstance(padding, PaddingStrategy):
        # note that this will raise an error if you pass an non-allowed padding - like "PAD_WITH_HUNDRED"
        padding_strategy = PaddingStrategy(padding)
    elif isinstance(padding, PaddingStrategy):
        # do something
        ...
else:
    # do not pad
    ...
```

In contrast to that, the `ClapFeatureExtractor`'s `padding` argument is used like this (code around [this](https://github.com/huggingface/transformers/blob/04ab5605fbb4ef207b10bf2772d88c53fc242e83/src/transformers/models/clap/feature_extraction_clap.py#LL242C20-L242C27) line):
```python
if padding == "repeat":
    # do something
    ...
if padding == "repeatpad":  # default value of padding = "repeatpad"
    # do something
    ...

# zero pad otherwise - whatever the value of padding is!
waveform = np.pad(waveform, (0, max_length - waveform.shape[0]), mode="constant", constant_values=0)
```

Try this code snippet for example:
```python
from datasets import load_dataset
from transformers import AutoProcessor

# load data
dataset = load_dataset("ashraq/esc50")
audio_sample = dataset["train"]["audio"][0]["array"]

# load data processor
processor = AutoProcessor.from_pretrained("laion/clap-htsat-unfused")

# pre-process data
inputs1 = processor.feature_extractor(audio_sample, return_tensors="pt", padding=True)
inputs2 = processor.feature_extractor(audio_sample, return_tensors="pt", padding="PAD_WITH_HUNDRED")

print((inputs1["input_features"] != inputs2["input_features"]).sum())
# Output: tensor(0)
```

I think this behaviour is unexpected and this should be made consistent with `tokenizer`.

> Would renaming `kwargs` to something like `shared_kwargs`, or `common_kwargs` be more clear?

I think `kwargs` would be fine if we document the function properly, but don't feel too strongly about keeping the name so fine with switching to `shared_kwargs` if we think it adds clarity over a proper docstring

> I think this behaviour is unexpected and this should be made consistent with `tokenizer`

Thanks for the clear explanation - the code snippets you've provided are very clean! I think so too - the CLAP processor functionality is a bit unexpected here. Shall we tackle this in a separate PR to more closely follow the `tokenizer` logic?

> fine with switching to `shared_kwargs` if we think it adds clarity over a proper docstring

I think it does but I am okay with whatever you prefer.

> Shall we tackle this in a separate PR to more closely follow the tokenizer logic?

Yes, these two things are independent and can be dealt with in separate PRs.

Awesome @anmol-wiai! Would you like to open a PR to fix one or both of these issues? Happy to guide you through the integration process, sounds like you have a good idea of what needs to be fixed with getting the CLAP processor consistent with the tokenizer 👍

Hi @sanchit-gandhi, I'm currently a little busy for the next two weeks. However, I can work on this afterwards if that timeframe suits you.

Sounds perfect - feel free to tag me in a PR whenever you get the chance to look at this and I'll get you a review! More than happy to answer any questions / queries here or on the PR, so just ping me if you get stuck 🤗

Still think this would be a nice way of clarifying the CLAP feature extractor kwargs! It's all yours @anmol-wiai if you're still up for it!

Hey @sanchit-gandhi! Sorry, I have been a little busy lately. I was planning to work on it this weekend. Let me create a PR and then I will reach out to you. 

PR still in progress at #24503