[generate] do_sample default back to False #3298
Conversation
| @@ -614,8 +614,8 @@ def generate( | |||
| input_ids=None, | |||
| max_length=None, | |||
| min_length=None, | |||
| do_sample=True, | |||
| early_stopping=False, | |||
| do_sample=None, | |||
There was a problem hiding this comment.
I like it better like this, because all inputs are set no None . It defaults to False because that's how it's set in self.config.do_sample as a default. But up for debate I guess.
There was a problem hiding this comment.
You should document that in the docstring then. When someone sees "Defaults to" in the docstring and then gets different default behavior (because of their config) it would be confusing.
sshleifer
changed the title
patrickvonplaten
force-pushed
the
change_do_sample_default_back_to_False
branch
from
34d4bac to
9857d2e
Compare
There was a problem hiding this comment.
Yes good for me. Just tweak the docstring as mentioned by @sshleifer and me.
Nice work!
For the records, this reverts the decision taken in #2696
cc @minimaxir and @LysandreJik
|
Not 100% sure how this results in a "Prettier API," but agree this isn't a big deal to fix downstream. (my current code explicitly sets If you are creating any demo generation notebooks/tooling like Write With Transformer, I recommend explicitly noting this behavior. |
* change do_samples back * None better default as boolean * adapt do_sample to True in test example * make style
This somewhat reverts the commit:
6c1b235
and the decision taken in #2696
and sets the default sampling behavior of
generate()to greedy - / beam search.Pros:
Falseis the more natural default valueCons:
do_sample=Truedefault value and this commit might break the logic of their code (but would be trivial to change for them)I'm somewhat indifferent whether this PR should be merged, but I think @thomwolf and @sshleifer are in favor of it.
@LysandreJik @thomwolf @sshleifer