Improve generate docstring
#18198
Improve generate docstring
#18198
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
I think it's best to leave the default as they were (since they are ultimately the defaults for the model config) and put a big warning at the top of the arg section of the docstring stating that all of them will be overridden by the model config. What do you think @patrickvonplaten ?
Things like |
|
Thanks for the feedback here @JoaoLages! I understand the reason behind your PR and am inclined to merge it as is - would like to get some input from @gante here as well though before merging |
|
This is a tough one. The change (as it is) is possibly good for generate-savvy users but will make it more confusing for most use cases -- all those config values have their own defaults which are in fact almost always used. We would lose that very useful part of the documentation to make this caveat more visible. In general, we can all agree that defaulting to the config specification is confusing (and a giant source of issues) -- @JoaoLages we are working on a plan to remove them, which is actually the root problem here. This means that documentation changes as a result of this PR will be temporary :) Personally, because of the two paragraphs above, I am more inclined toward @sgugger's suggestion -- the most common situation stays clearly documented, and a temporary warning gets added. @JoaoLages WDYT? |
Totally agree with this statement!
The warning would help 👍 |
|
Awesome, I think we can move forward with it then :) One detail -- this warning should go in FLAX's and TF's docstring as well. If it is not asking too much @JoaoLages, can you copy it to the other frameworks as well? 🙏 |
Actually, the warning is already in the docstring, right? I guess it is not that visible 😅 |
src/transformers/generation_utils.py
Outdated
| @@ -927,43 +927,43 @@ def generate( | |||
| max_new_tokens (`int`, *optional*, defaults to None): | |||
| The maximum numbers of tokens to generate, ignore the current number of tokens. Use either | |||
| `max_new_tokens` or `max_length` but not both, they serve the same purpose. | |||
| min_length (`int`, *optional*, defaults to 10): | |||
| min_length (`int`, *optional*, defaults to `model.config.min_length`): | |||
There was a problem hiding this comment.
| min_length (`int`, *optional*, defaults to `model.config.min_length`): | |
| min_length (`int`, *optional*, defaults to `model.config.min_length` or 10 if the config does not set any value): |
There was a problem hiding this comment.
This is verbose... but possibly the best solution until it gets fixed.
As @JoaoLages mentioned, the warning is already there (PT only). However, because our documentation shows the parameters first and generate has 39 parameters (😅), the warning is not visible at first (see the docs).
There was a problem hiding this comment.
Yes, that's why I'm suggesting this compromise :-)
There was a problem hiding this comment.
I'll make the changes then. Thanks for your great work HuggingFacers! 💪
You were too fast 😂 |
What does this PR do?
The generate docstring is not correct, because it has a lot of defaults that read from
model.configand that is not clearly stated in the method description.Before submitting
Who can review?
@sgugger @patrickvonplaten I believe this one is for one of you two?