[Generation Config] General issues
Author: ArthurZucker
Pull request: no

# Generation config 
I know it has just been added so it is normal! But the following are missing (and are pretty intuitive w.r.t our other objects such as configs, processors etc): 

- [ ] `GenerationConfig.from_pretrained("openai/whisper-tiny.en" )` where the path does not already have a `generation_config.json`. Currently this only looks for the `generation_config.json` but it should be possible to initialised by default using the `config` if a generation file is not present. (as it is actually done in generate)
- [ ]  Similarly, the `from_config` only supports a config `object` which has to be initialized. 
- [ ] The `generation_config` should be automatically initialised before the `generate` function : this is because other models will call `super().generate` after having played with extra kwargs, and these kwargs are needed for pre-processing ( adding selected logit processors etc). 
- [ ] [edit] when running 'generate()` the generation config should by default be initialized `from_pretrained`, this is the whole point of saving a generation_config file IMO
- [ ] The following warning 
```python 
warnings.warn(
 "You have modified the pretrained model configuration to control generation. This is a"
 " deprecated strategy to control generation and will be removed soon, in a future version."
 " Please use a generation configuration file (see"
 " https://huggingface.co/docs/transformers/main_classes/text_generation)")
``` 
should be discussed as it is more efficient to modify the generation parameters on the fly if you are using a model that requires special arguments (like whisper), which is what `generation_config` was designed for. Indeed you might want to do `translation` but the default task on the hub is `traduction`. For a newbie, he has to go through finding `processor.set_forced_decoder_ids` while he could just do `model.generate(task = 'transcribe')`. The same goes for the `return_timestamp` which uses the same model so no need for a new config/ new model on the hub. 
More generally, I think that if we have models that use prompts and special initial tokens, this is very useful.

cc @sgugger , @gante, @patrickvonplaten and @LysandreJik 

This arises in the refactoring of whisper to have somewhat of a 1-1 with open ai, where you just load the model and can do `generate()`. It also simplifies the pipeline for asr's whisper specific parts

ps: I might be completely wrong about this! Feel free to give me feedbacks! 

Comments:
1. Agree - I think we should/could allow this functionality.
2. Here I don't think we need to change anything. It's quite intuitive for me that the "[from_model_config](https://github.com/huggingface/transformers/blob/4e730b387364c9f46b6b1b0c79fdaf0903c42257/src/transformers/generation/configuration_utils.py#L620)"  API has to load a config type object
3. I don't fully understand this - could you add an example? 
4. Could you maybe send a link to where the warning is thrown? Would make it easier to understand what logic it's talking about 

3. An example would be the following: 
```python 
class WhisperForConditionalGeneration:
    ...
    # redefine generate with custom kwargs like `task`, `return_timestamps` and `is_multilingual` 
    def generate(
        self,
        inputs: Optional[torch.Tensor] = None,
        generation_config= None,
        logits_processor = None,
        stopping_criteria = None,
        prefix_allowed_tokens_fn = None,
        synced_gpus = False,
        return_timestamps = None,
        task = None,
        is_multilingual = None,
        **kwargs
    ):
        # At this point we want the generation config to be initialized, otherwise we have to copy past the initialization
        # scheme, and it will be run again when calling super. Also modifying self.generate_config
        # here update self.generation_config or generation_config
        self.generation_config.return_timestamps = return_timestamps if return_timestamps is not None else False
        self.generation_config.task = task if task is not None else False
        self.generation_config.is_multilingual = is_multilingual if is_multilingual is not None else False

        if self.generation_config.forced_decoder_ids and task is not None:
            if self.generation_config.is_multilingual:
                self.generation_config.forced_decoder_ids[1][1]= generation_config.task_to_id[generation_config["task"]]
            else:
                raise  ValueError("A task or language were given but the model was trained on english and can thus only transcribe from english to english.")
        if return_timestamps:
            logits_processor = [WhisperTimeStampLogitsProcessor(self.generation_config)]
        return super().generate(inputs, self.generation_config, logits_processor, stopping_criteria, prefix_allowed_tokens_fn, synced_gpus, **kwargs)
```
This is not possible because the initialisation of the config only takes place in the generate. But this also means that if a `generation_config.json` file exist (meaning someone went through the trouble of saving a generation config and pushing it to the hub` it is not used automatically either. You have to instantiate it. This is not really good as for example some arguments are only necessary for `generate()` in this case, `no_timestamps_token_id`, and cannot be set through the pipeline (the pipeline would have to support `generation_config` as well). Again you would have to do a `GenerateConfig.from_pretrained("...")` which should be automatically done (it's the purpose of having a save file).

4. Sorry, the warning comes from [here](https://github.com/huggingface/transformers/blob/main/src/transformers/generation/utils.py#L1186)

Before going down at the individual points, I think it is worth agreeing on high-level design choices :) The goal of this refactor was to separate the two types of configurations, which were being held in the same file and class. It was also made to be retrocompatible with existing uses. 

Beyond these two basic points, there was a design decision to nudge users into treating these two configurations separately, so they can evolve in isolation and have minimal cross-dependencies. I've also made an intentional effort to avoid using the term `config` in `GenerationConfig`, when referring to the model config. If we agree that this is a desirable property, then some short-term pain should be endured.

1. While it is simple to implement, it will only be useful in the transition phase -- in the future, no generation parameters are held in the model config. If we do implement it, `GenerationConfig.from_pretrained()` will acquire a dependency on another class and it will remove the incentive for the users to use separate files (both undesirable IMO). It will also make the name of the function a lie, as we will not be returning from a "pre-trained Generation Config". Side-note: the existing `GenerationConfig.from_model_config()` does this cross-class loading while keeping responsibilities isolated.
2. (I'm assuming you're writing about `GenerationConfig.from_model_config()`, as there is no `GenerationConfig.from_model_config()`) Happy to expand it :) What format would you like to get here, a dict?
3. It is pre-initialized [here](https://github.com/huggingface/transformers/blob/91ff7efeeb3e6bb10d83702db24108bb6583e013/src/transformers/modeling_utils.py#L1036) from the model config, for retrocompatibility. It is then overwritten [here](https://github.com/huggingface/transformers/blob/91ff7efeeb3e6bb10d83702db24108bb6583e013/src/transformers/modeling_utils.py#L2505) from the generation config file, if it exists. Hopefully, many versions from now, the initialization from the model config will be removed 🤞
4. I don't get this point -- we do use a `GenerationConfig` loaded from `from_pretrained` by default 🤔 If the question is that all `.generate()` calls should be using a `GenerationConfig` initialized that way, then we go back to the question in 1. :)
5. The warning only applies when the user modifies the model configuration to achieve a different generation behavior. This is actually a problem that relates to 1.: if we do load the generation config from the model config, and the user follows this pattern [change the model config], how can we ensure correctness? The generation config would no longer match the model config regarding generation parameters. My take here was to raise a warning and slowly kill this behavior, as it is making the separation of concerns impossible at the moment unless the logic you see at the start of `.generate()` is added everywhere. This is also why changes in the model config no longer work to parameterize `.generate()` if you call specific generation methods (like `greedy_search()`)
E.g.
```py
# modify generation properties through ad hoc model config changes
model = BartForConditionalGeneration.from_pretrained("hf-internal-testing/tiny-random-bart", max_length=10)
# or
model = BartForConditionalGeneration.from_pretrained("hf-internal-testing/tiny-random-bart")
model.config.max_length = 10
# both will raise that warning at generation time, since that is no longer the job of model config
model.generate(...)

# however, this will NOT raise the warning, since it's the generation config's job
model = BartForConditionalGeneration.from_pretrained("hf-internal-testing/tiny-random-bart")
model.generation_config.max_length = 10
model.generate(...)
```

Two additional comments/points:
6. You touched the point of having multiple `tasks`. That is not yet implemented, but highly desirable! At the very least, to make sure the right pipeline can load the right generation config file.
7. I'm noticing that are missing the functionality to save the generation config when `model.save_pretrained()` is called, which I forgot to add 🤦 This will ensure that ALL new saved models will have a `generation_config.json` if they can call `.generate()`. EDIT: https://github.com/huggingface/transformers/pull/21264


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.

Closing as I have not more comments! Things look good for now!

Just to clarify the intention here...does the warning mean that we are supposed to override the model.config.do_sample parameter on the model object whenever we change temperature values in the generate keyword arguments from 0 to non-zero? (that seems to be what is needed unless I am missing something)

For example for GPT-Neo I was passing `do_sample=temperature > 0` so I don't get logitprocessor errors when we choose zero temp

Hey @slundberg 👋 

`temperature` and `do_sample` control two different things, please refer to the [documentation](https://huggingface.co/docs/transformers/main_classes/text_generation#transformers.GenerationConfig) :) Argument validation is yet to be added.

Hey! Got it. I see that do_sample controls the firing of the `sample` method vs the `greedy` method (or the beam search equivelents), but since setting temperature=0 implies greedy decoding, some common APIs (like OpenAI) automatically set sample vs greedy based on the temperature. If I expose an interface that does the same (sets do_sample based on the temperature given by the user), then I run into this warning unless I change the actual model object that was passed (which is not ideal). Not sure if this makes sense.

I see -- yeah, if that's the intended behavior (use greedy decoding when temperature is 0*), then it makes sense! From your past two messages, I'm assuming you are controlling it through `model.config` [`model.config.do_sample=temperature>0`], which raises the warning. Would you be able to control it through `.generate()` [`.generate(do_sample=temperature>0)`] or through `model.generation_config` [`model.generation_config.do_sample=temperature>0`]?

*While they are analytically equivalent, if you apply a very small temperature you'll get `-inf` numbers. ATM it results in an exception in the sampling phase, because of the `-inf`, in the feature we will raise an exception at the argument validation phase. In `.generate()`, we'd rather be explicit with exceptions than implicit with subtle corrections like switching into greedy decoding, so that the programmer has full control over what's happening :)