Add warnings for incompatible generation parameters

[jsrozner]

What does this PR do?

While testing various generation configurations, I got confused when beam_sample's outputs (i.e. num_beams > 1, do_sample=True) failed to change with different settings of top_k, top_p, temperature. Then I realized that in my call to generate(), do_sample was still set to false even though I was cycling through various settings of top_k, top_p and temperature.

Generate (and its helper methods) already contain some parameter compatibility checks. This adds a few more checks:

• num_beams must be >= 1
• when do_sample is not set, warn the user if any of top_p, top_k, temperature are not None (since they will have no effect)
• if num_beams is 1 (no beam search), warn user if any of early_stopping, length_penalty are not None (since they will have no effect)
• if an invalid set of params num_beams and do_sample are passed, raise ValueError. Note that since we also add a check for num_beams < 1, this final value error will never be raised, but this prevents the generate function from falling off the end of the ifelse chain if something is altered in the future.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• [x ] Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
  No doc changes necessary
• Did you write any new necessary tests?
  ran tests, but no new functionality created, just warning messages

Who can review?

Please tag fewer than 3 people.
Text Generation: @patrickvonplaten

[jsrozner]

Question: the generate() documentation says that topk etc params will default to values in the pretrainedconfig, but top_k, for example is never read from the config file. This differs from, e.g., num_beams, max_length which are read from config if they are not passed in as params to the generate function.

And since, for example, T5PreTrainedModel never overwrites the generate function, I don't see how the defaults in the config (for params like top_k) could actually end up being passed to the generate function?

[patrickvonplaten]

I don't want to add those warnings as we would have to do this for all parameters. IMO, if a user is playing around with top_k, top_p and/or temperature, she/he should know that it only works with do_sample=True.

[patrickvonplaten]

I'm happy however to add a statement for each input param to the doc. E.g. under top_p. Note that top_p is only effective if do_sample=True

[jsrozner]

Ideas:

• What if we also do it for the other params? In that case what would those other params be? I'm happy to add checks for the other params in this function (though that could mean that this function's behavior will differ from other behaviors in the library since warnings are not generally issued for unused parameters. I do think that's one of the weaknesses of a library this general. It's sometimes hard to find where something isn't behaving as expected because an arg gets dropped or is set unexpectedly in another location.)

• A more involved idea would be to group the params as dicts for each type of generation.

But more generally I agree with you that the user should know that if he plays with top_p, top_k it's only valid if do_sample is true. My problem was that in my wandb sweep code, I had forgotten to set do_sample to True, so I was iterating through a ton of topp, topk settings but seeing no changes in output. At first I thought beam_sample was broken. It took me a while to figure out that my do_sample setting to false was nullifying all the other params I was passing in. A warning would have been nice...

[patrickvonplaten]

In my opinion, it's not worth it to clutter the code for such warnings. Another problem is that this warning will not catch if people change top_k and top_p with their config and this is also hard to check since config.top_k for example is set to 50 already. I'd really prefer to just add those statements to the doc.
If we go down the road of adding warnings for every single parameter, it won't become maintainable, e.g. we might merge this PR soon: #8627

It's super useful to add these statements to the doc, but not worth the if-else cascade for the warnings IMO. There is already a lot of stuff happening under-the-hood in generate() and if one wants to change the default generate configs of the model he/she should be aware of what is going on exactly.

[patrickvonplaten]

Same here

[patrickvonplaten]

I don't think it's possible to arrive here if num_beams is set correctly

[jsrozner]

Agreed - once we have the num_beams check this could never happen. (I mentioned that in the PR note!)

It just seems like good code style to avoid having the possibility of falling off the end if changes are ever made to the code above?

[patrickvonplaten]

Ok for me. Can we change the error message though, e.g.:

"No generation method was called. Make sure to correctly set the parameters num_beams and do_sample to call one of greedy_search, sample, beam_seach, or beam_sample."

[patrickvonplaten]

I'm very much on-board with this raise! Can we move it however below line 464?

[jsrozner]

Question: the generate() documentation says that topk etc params will default to values in the pretrainedconfig, but top_k, for example is never read from the config file. This differs from, e.g., num_beams, max_length which are read from config if they are not passed in as params to the generate function.

And since, for example, T5PreTrainedModel never overwrites the generate function, I don't see how the defaults in the config (for params like top_k) could actually end up being passed to the generate function?

Regarding reading from config - am I missing something or do these never get checked?

@patrickvonplaten

[patrickvonplaten]

Question: the generate() documentation says that topk etc params will default to values in the pretrainedconfig, but top_k, for example is never read from the config file. This differs from, e.g., num_beams, max_length which are read from config if they are not passed in as params to the generate function.
And since, for example, T5PreTrainedModel never overwrites the generate function, I don't see how the defaults in the config (for params like top_k) could actually end up being passed to the generate function?

Regarding reading from config - am I missing something or do these never get checked?

@patrickvonplaten

transformers/src/transformers/generation_utils.py

Line 240 in 693ac35

top_p = top_p if top_p is not None else self.config.top_p

[github-actions]

This issue has been automatically marked as stale and been closed because it has not had recent activity. Thank you for your contributions.

If you think this still needs to be addressed please comment on this thread.