Silent failure in generation parameters 
Author: Manalelaidouni
Pull request: no
Labels: name: bug

name: Generation

### System Info

- `transformers` version: 4.44.2
- Platform: Linux-6.1.85+-x86_64-with-glibc2.35
- Python version: 3.10.12
- Huggingface_hub version: 0.24.7
- Safetensors version: 0.4.5
- Accelerate version: 0.34.2
- Accelerate config: 	not found
- PyTorch version (GPU?): 2.4.1+cu121 (False)
- Tensorflow version (GPU?): 2.17.0 (False)
- Flax version (CPU?/GPU?/TPU?): 0.8.5 (cpu)
- Jax version: 0.4.33
- JaxLib version: 0.4.33
- Using distributed or parallel set-up in script?: No

### Who can help?

@zucchini-nlp  @gante 

### Information

- [ ] The official example scripts
- [ ] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [ ] My own task or dataset (give details below)

### Reproduction

Hey! 

I noticed that `top_p` was silenctly failing so I tested the rest of the generation parameters and found that `no_repeat_ngram_size` also silently fails for the same reason:  the condition checks inside of the `_get_logits_processor()` method prevent their respective wrapper classes from executing, which is where the ValueError  are being raised.

For instance,  ``raise ValueError(f"`ngram_size` has to be a strictly positive integer, but is {ngram_size}")`` error is never reached when we set `no_repeat_ngram_size <= 0`.

Here is a simple example with the invalid values where generation proceeds without notifying the user. Ideally, those should raise errors or warnings.

```
from transformers import AutoModelForCausalLM, AutoTokenizer

model_name = "EleutherAI/pythia-14m"
tokenizer = AutoTokenizer.from_pretrained(model_name)
model = AutoModelForCausalLM.from_pretrained(model_name)
prompt = "hey there!"
inputs = tokenizer(prompt, return_tensors="pt")
generation_config = dict(do_sample=True, top_p=5, no_repeat_ngram_size=-1)

outputs = model.generate(input_ids=inputs['input_ids'], attention_mask=inputs['attention_mask'], **generation_config)

response = tokenizer.decode(outputs[0], skip_special_tokens=True)
response
```

### Expected behavior

- To not let things fail silently and proceed with a default value, instead raise a ValueError or issue a warning to the user.

- It would be great if the `generate` method could fail early when invalid values are passed, maybe by checking for them upfront in `_get_logits_processor`  before applying the generation parameters one by one and going through the entire process, this will help avoid wasting compute resources.


I would be happy to open a PR to help address this issue if that’s possible, thank you for all your work!

Comments:
cc @gante 

Hey! Actually these are validated when preparing generation config in 
https://github.com/huggingface/transformers/blob/f2c388e3f946862f657acc1e21b272ec946fc66c/src/transformers/generation/utils.py#L1355C28-L1355C45

But seems like we don't check all generation parameters and perform general check on whether there isn't any clash between kwargs. @gante will say if we need to perform check on everything or not :)

Thank you @zucchini-nlp for the clarification, from what I can see it looks like the `_prepare_generation_config` method you pointed out doesn’t actually validate the parameters; it just updates the config with the provided values.

It would be great to hear @gante's thoughts on whether we should validate all parameters upfront,  perhaps, this could happen in `_prepare_generation_config`.

I'm happy to assist with a PR if needed!

@Manalelaidouni the config validates that incoming params are correct when updating, but the validation is done on a general level. So that we don;t have clashing arguments as generation without sampling but with a top-k param

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

Hi @Manalelaidouni 👋 

Yes, more validation is super welcome! :D `GenerationConfig.validate` would be the right place to add more validation -- this function is run both when we define a `GenerationConfig` object and when we update it in `generate`.

We should, in fact, create bulk validation structures, such as:
- define which variables are integers, check that they are indeed integers
- same, but for floats
- same, but for positive variables
- same, but for [0, 1] floats

(note: we don't want to add pydantic as a dependency :) )

Got it, I’ve opened a PR with your suggestions @gante @zucchini-nlp if there’s any advanced validation testing you have in mind I’m happy to implement it! 😊

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