ASR pipeline never takes into account the beam width of ngram
Author: harveenchadha
Pull request: no

I am using the eval.py script mentioned [here](https://github.com/huggingface/transformers/blob/master/examples/research_projects/robust-speech-event/eval.py)

I try to pass the beam_width parameter at lines 84 and 88 but the results are the same no matter whatever beam you give. Am i doing something wrong?

Comments:
Hey @harveenchadha, yeah this is indeed not possible at the moment sadly. We could make this easily possible and I would be fine with adding some code to allow passing those decoding hyper-parameters. I'm just wondering at what point the ASR pipeline becomes too much of a black magic vehicle. @Narsil - would you be fine with allowing one to define inputs to the `processor's` `batch_decode()` method in eval? 

E.g. do you think we could allow users to pass the following parameters: https://github.com/huggingface/transformers/blob/45cac3fade34cb7134b080c5060c250f810db5e2/src/transformers/models/wav2vec2_with_lm/processing_wav2vec2_with_lm.py#L247 right through the pipeline? 

Happy to open a PR if you're ok with it

What I don't think is nice, is blindly passing kwargs,

Because if multiple places want to receive arbitrary kwargs and have the same name, well, bad things happen. IMO the options are:

- Make those arguments available within a config and get their default from there. It's transparent for pipeline (and every object in transformers ecosystem which seems nice)
- Whitelist a subset of commonly used parameters. It's simple, no risk of clashing. But the list can grow very fast and then it becomes hard to use the pipeline because of so  many parameters.
- Pass directly a `decode_kwargs`, `generate_kwargs` as a single argument which is a dict. No clashing, all arguments supported by default, but less convenient to use `pipeline(..., decoder_kwargs = {....})`.
- Support 1 and only 1 `**kwargs` and ban every other kwargs, force them to use a whitelist. This is what currently happens.
I tend to dislike this because 1 location of kwargs is more important than others (and decision is pretty arbitrary). Here `decoder` definitely does not seem like a good fit for this prioritized spot.


For this particular problem I would tend to lean on option 3, wdyt ?

Yes, I see the point and I think it makes sense to add a `decoder_kwargs={}` object.

Regarding the `generate_kwargs` however, if we decide to go for:

```py
def __call__(self, inputs, decoder_kwargs, generate_kwargs, **kwargs)
```

it wouldn't be fully in line with our other generation pipelines for which the generate kwargs can directly be passed to `**kwargs`. E.g.:

```py
generator = pipeline("text-generation")
generater("This is a prompt", max_length=50, do_sample=True)
```

is possible! Wouldn't it be good to have the same behavior for both the Generation and ASR pipeline for consistency and to not surprise the user? Maybe we could go for a: 

```py
def __call__(self, inputs, decoder_kwargs, **kwargs):
```

syntax?

Another option is the 1 and only 1 `**kwargs` design, which I think can work nicely as well. We could heavily mitigate the risk of clashing names though by adding a prefix to the decoder kwargs (they are less important IMO). *E.g.* `decoder_beam_width` would set the decoder's `beam_width` here: https://github.com/huggingface/transformers/blob/5ec368d79ecfa1467358f5b42da2cecbe23d6b0d/src/transformers/models/wav2vec2_with_lm/processing_wav2vec2_with_lm.py#L251

We do more or less the same thing for encoder-decoder models here: https://github.com/huggingface/transformers/blob/5ec368d79ecfa1467358f5b42da2cecbe23d6b0d/src/transformers/models/encoder_decoder/modeling_encoder_decoder.py#L348 

For this approach we'd obviously need good documentation, but it would be relatively save and user-friendly IMO. 

What do you think @Narsil ?

 `decoder_kwargs` is better IMO. It's cleaner from a code perspective and even for the user and the doc (Please refer to this other object which describes its parameters, less likely to go out of sync).

`decoder_beam_width` is very magical, and users tend to read the doc... fast, when they do... (We do need a solid documentation for sure, but regular consistent code and proper errors is almost always superior to doc IMHO). That's another source of confusion in information flow. Right now, there is logic dedicated to feeding the correct kwargs to the  correct function (`preprocess`, `_forward`, `postprocess`) and with magic args it gets confusing pretty quick. You're also not really preventing any clashing since `generate_kwargs` contain `decoder_start_ids` and your `lm_decoder` might one day contain `start_ids` :D 
I really don't like that option.

`generate_kwargs` **are** special, and I don't want to break anything there for other pipelines. But they don't seem that necessary right now for ASR (most models are CTC right ?). I think we can postpone the debate until the need arise right ? (Right now I think you are right, if we wanted to add it, consistency for `generate_kwargs` would trump cleanliness, but I would rather delay that choice)

Cool sounds good! Let's go for `decoder_kwargs(...)` then now

@harveenchadha - would you be interested in opening a PR ? Otherwise maybe @Narsil or @anton-l ? I'm sadly off in a bit and won't have time tomorrow :-/

Off for a week starting tomorrow.

Hi @patrickvonplaten @anton-l  @Narsil ! I'm working on the last changes for the robust speech event and we (dbdmg Italian team) really wanted the decoder_kwargs feature.

Following your discussion, I implemented a 
```python
def __call__(self, inputs, decoder_kwargs, **kwargs):
```
version. You can find it on my latest [commit](https://github.com/g8a9/transformers/commit/1f25864edf0319a9984b3051b420b86e287d949b).

It's my first contribution and I don't know if I added it properly. Does it feel right to you?   

Hi @g8a9 Thanks for taking a stab at it.
Ultimately I don't think your commit is the correct route for this (https://github.com/huggingface/transformers/issues/15484#issuecomment-1028826853)

I created a new PR which hopefully is ok for you to work with (look at the test for usage right now)

https://github.com/huggingface/transformers/pull/15646

Hi @Narsil I've looked at your new PR, seems ok to me.

I was missing the way params should have been handled in the parent class. But I hope it helped nonetheless :)


Definitely very help ful @g8a9 ! 

Sorry guys, couldn't contribute as was not well from couple of days!

No worries! Hope you're feeling better :-)