Add t5 to pipeline(task='summarization')

[patrickvonplaten]

This PR:

• adds T5 to summarization piplines.
• adds warnings and better defaults to Bart/T5 summarization
• removes unnecessary assert in generate() function

[codecov-io]

Codecov Report

Merging #3413 into master will increase coverage by 0.02%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master    #3413      +/-   ##
==========================================
+ Coverage   77.56%   77.58%   +0.02%     
==========================================
  Files         100      100              
  Lines       16970    16993      +23     
==========================================
+ Hits        13162    13184      +22     
- Misses       3808     3809       +1     

Impacted Files	Coverage Δ
src/transformers/modeling_utils.py	91.71% <ø> (-0.02%)	⬇️
src/transformers/pipelines.py	73.05% <93.10%> (+0.52%)	⬆️
src/transformers/modeling_tf_utils.py	84.44% <100.00%> (+0.52%)	⬆️
src/transformers/tokenization_t5.py	95.89% <100.00%> (+0.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e392ba6...23778d1. Read the comment docs.

[sshleifer]

From my perspective, looks good. Would love test coverage to avoid breaking it accidentally!

[sshleifer]

in a previous PR, @thomwolf suggested that if a kwarg (like length_penalty) is on the config, it should only be exposed through generate_kwargs).

[patrickvonplaten]

I would favor to (re-)define the kwargs like max_length, min_length, do_sample, length_penalty in this function because the defaults in the config are defaults for generate() in general, but there are not good defaults for summarization. And to me pipelines is about easy-to-use so I think good general defaults for summarization should be defined here. Happy to discuss.

[thomwolf]

Ok that's quite an interesting case for T5 versus Bart because in Bart the pretrained model are task-specific so it make sense to have the task-specific generation hyper-parameters in the config while for T5 the pretrained model is generic so it would make more sense to have a way to specify task specific generation HP in the config.

In general, I'm not a fan of having model-specific hyper-parameters in general classes like pipelines.

What I would propose is maybe the following for T5 (open to discussion):

• have generic generation HP in the config like now
• also have a dict of class specific generation HP in the config which can override the generic HP.

Ex of config dic:

{
....
max_length: 100,  # generic generation HP
length_penalty: 1.0,
task_specific_generation_params: {
     "summarization": {  # task id (e.g. name of the pipeline?)
          max_length: 140,
          length_penalty: 2.0
      },
     "translation_en_to_de": {
          max_length: 160,
          length_penalty: 3.0
      },
    },
}

What do you think?

[patrickvonplaten]

Yeah, I like that idea!
Two questions:

• Should we enforce that all parameters defined in task_specific_generation_params have to be already defined so that they can only override?
• Do we allow "summarization" and "translation" if the config has no task_specific_generation_params defined?

[patrickvonplaten]

Is there a reason why we would not optionally use pad_to_max_length here? Was not sure if I can add it, but for summarization when using a batched input, this is necessary. @LysandreJik @thomwolf @mfuntowicz

[thomwolf]

Nice work. A few tweaks regarding handling task-specific generation HPs if you can do it (see my comments).

[thomwolf]

Ok for now but we'll probably refactor this in the future to have framework-agnostic ensure_tensor_on_device and get_tensor_length methods (maybe have a base class and framework-specific derived class for instance).

[thomwolf]

Ok that's quite an interesting case for T5 versus Bart because in Bart the pretrained model are task-specific so it make sense to have the task-specific generation hyper-parameters in the config while for T5 the pretrained model is generic so it would make more sense to have a way to specify task specific generation HP in the config.

In general, I'm not a fan of having model-specific hyper-parameters in general classes like pipelines.

What I would propose is maybe the following for T5 (open to discussion):

• have generic generation HP in the config like now
• also have a dict of class specific generation HP in the config which can override the generic HP.

Ex of config dic:

{
....
max_length: 100,  # generic generation HP
length_penalty: 1.0,
task_specific_generation_params: {
     "summarization": {  # task id (e.g. name of the pipeline?)
          max_length: 140,
          length_penalty: 2.0
      },
     "translation_en_to_de": {
          max_length: 160,
          length_penalty: 3.0
      },
    },
}

What do you think?

[thomwolf]

Ok, note that we can now remove the inputs_for_model from pipelines since #3116
Let's do it in another PR cleaning up pipelines later.