Fix TF generate (probably)

[ydshieh]

What does this PR do?

We have CI failures for TFBertEncoderDecoderModelTest.test_bert2bert_summarization and TFGPT2EncoderDecoderModelTest.test_bert2gpt2_summarization.

The error message is

 >       if generation_config.min_length is not None and generation_config.min_length > generation_config.max_length:
 E       TypeError: '>' not supported between instances of 'int' and 'NoneType'

These 2 tests pass max_length=None to generate:

output_ids = model.generate(input_ids=input_dict["input_ids"], max_length=None).numpy().tolist()

and this line (in generate)

transformers/src/transformers/generation/tf_utils.py

Line 613 in 63b204e

model_kwargs = generation_config.update(**kwargs) # All unused kwargs must be model kwargs

change generation_config.max_length from 20 (the default value) to None, and finally we get error at

transformers/src/transformers/generation/tf_utils.py

Line 719 in 63b204e

if generation_config.min_length is not None and generation_config.min_length > generation_config.max_length:

This PR check if generation_config.max_length is not None before doing comparison - the 2 tests pass with this change.
But we need @gante to see if this is the right fix.

[ydshieh]

@ArthurZucker The PR #20944 failed the test

tests/models/encoder_decoder/test_modeling_tf_encoder_decoder.py::TFBertEncoderDecoderModelTest::test_bert2bert_summarization

while one commit before 7cb596fa works well - assuming the changes in this PR is applied.

With #20944, the outputs from the above is somehow gibberish.

We can wait this PR being merged , then could you take a look of this issue 🙏 ?
(Or if you want to look it earlier - you just have to pull this branch) Better to wait, as I am not sure if there are more recent commits affect this test.

Here is the traceback

E       AssertionError: Lists differ: ['sa sa sa university sa sa sigma sa sa th[501 chars] sa'] != ["sae was founded in 1856, five years befo[236 chars]hs."]
E       
E       First differing element 0:
E       'sa sa sa university sa sa sigma sa sa th[500 chars]a sa'
E       "sae was founded in 1856, five years befo[235 chars]ths."
E       
E       Diff is 897 characters long. Set self.maxDiff to None to see it.

[HuggingFaceDocBuilderDev]

The documentation is not available anymore as the PR was closed or merged.

[ArthurZucker]

Basically this is because there hast to be a past argument passed down instead of pat_key_values. I'll open another PR to fix these, but #21296 should be the fix

[ydshieh]

Basically this is because there hast to be a past argument passed down instead of pat_key_values. I'll open another PR to fix these, but #21296 should be the fix

Thank you. We are going to have 0 test failures soon!

[gante]

Instead of adding this extra if to handle max_length=None, I'd like to keep disallowing max_length=None -- enabling it may allow users to enter uncharted territory when current length > model's maximum input length 😅

The fix should be to remove max_length=None in the test -- the right value will be fetched from the config, like in the PT test.

[ydshieh]

@gante Thanks! Updated the PR :-)

[gante]

Thanks for fixing 💛

[ydshieh]

@gante I merged this PR as it is. However, we could potentially improve the code in generate to validate (more) the arguments to avoid such failures. Will let you to decide as you know much more :-)