Add Summarization to Pipelines #3128

sshleifer · 2020-03-04T23:21:23Z

Choices:

This is not TextGenerationPipeline, so it only supports bart-large-cnn.
It doesn't return the input back to the caller because it is annoyingly long.

codecov-io · 2020-03-05T00:12:18Z

Codecov Report

Merging #3128 into master will increase coverage by 0.05%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master    #3128      +/-   ##
==========================================
+ Coverage   78.02%   78.08%   +0.05%     
==========================================
  Files          98       98              
  Lines       16670    16689      +19     
==========================================
+ Hits        13007    13031      +24     
+ Misses       3663     3658       -5

Impacted Files	Coverage Δ
src/transformers/__init__.py	`98.91% <ø> (ø)`
src/transformers/pipelines.py	`72.53% <95.00%> (+1.57%)`	⬆️
src/transformers/tokenization_utils.py	`91.99% <0.00%> (+0.14%)`	⬆️
src/transformers/modeling_tf_utils.py	`88.37% <0.00%> (+0.17%)`	⬆️
src/transformers/modeling_utils.py	`94.14% <0.00%> (+0.27%)`	⬆️

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 3814e16...a123599. Read the comment docs.

sshleifer · 2020-03-05T05:05:53Z

src/transformers/pipelines.py

@@ -1324,7 +1421,7 @@ def pipeline(

    # Use default model/config/tokenizer for the task if no model is provided
    if model is None:
-        models, config, tokenizer = tuple(targeted_task["default"].values())
+        models, config, tokenizer = [targeted_task["default"][k] for k in ["model", "config", "tokenizer"]]


this removes a dependency on dict ordering

Py36 dicts are ordered (and we might move to Python3.6 "soon") so maybe just document this gotcha, but do not change this? Or do you need it for your tests to pass?

In my code, the order changes in py3.5 if the value for config (one of the keys in the dict) is set to bart-large-cnn. Since we still support python 3.5, it seems safer (and more explicit) to avoid reliance on dict ordering to me.

julien-c · 2020-03-05T05:37:33Z

src/transformers/pipelines.py

+        length_penalty=2.0,
+        max_length=140,
+        min_len=20,
+        no_repeat_ngram_size=3


Do we really want to expose all those options? or a minimal subset? (the other pipelines do not expose any option)

That's a good point.
I feel like selecting just a subset of the generate method arguments is a bit arbitrary.

What about something like what we usually do:

good defaults for people who just want something to work out-of-the-box (like the one here), and

full customizability for the others, e.g. by having generate_kwargs that is directly transmitted to the model.generate() method and linking to the generate() method doc in the the docstring for the list of the arguments (that will likely evolve in the future as better decoding mechanisms are designed).

Love the idea of passing through generate_kwargs.
In terms of exposing options, I was preparing to support different kwargs for bart-large-xsum, bc the author wrote "Use beam=6, lenpen=1.0, max_len_b=60, min_len=10 for Xsum Generation" in https://github.com/pytorch/fairseq/blob/master/examples/bart/README.summarization.md#L121

That's also a good point. We don't want to expose two different pipelines for two trained models.

Here is what we should do if it's not already the case: all the parameters of the generate() method are actually stored in the configuration of the pretrained models (or more precisely, they wil be once #3140 is merged, see here what I'm talking about) so you should update the configuration of the two pretrained summarization weights on S3 to have the best decoding parameters as selected by the authors and not hard-code any specific default values here in the pipeline.

cc @patrickvonplaten since it's related to his work

julien-c · 2020-03-05T05:40:46Z

src/transformers/pipelines.py

@@ -1324,7 +1421,7 @@ def pipeline(

    # Use default model/config/tokenizer for the task if no model is provided
    if model is None:
-        models, config, tokenizer = tuple(targeted_task["default"].values())
+        models, config, tokenizer = [targeted_task["default"][k] for k in ["model", "config", "tokenizer"]]


Py36 dicts are ordered (and we might move to Python3.6 "soon") so maybe just document this gotcha, but do not change this? Or do you need it for your tests to pass?

thomwolf

Ok good first draft, a few comments and questions.

Let's merge #3140 first to be able to use defaults generation configuration.

thomwolf · 2020-03-05T21:46:18Z

src/transformers/pipelines.py

+        modelcard (:obj:`str` or :class:`~transformers.ModelCard`, `optional`, defaults to :obj:`None`):
+            Model card attributed to the model for this pipeline.


I don't think we have this argument anymore

We have it for task-specific pipelines, just not for the pipeline factory. Though I agree it should be deleted for the task-specific pipelines as well.

thomwolf · 2020-03-05T21:47:22Z

src/transformers/pipelines.py

+            max_length: (`optional`) int
+                The max length of the sequence to be generated. Does not include tokens in input_ids.
+
+            num_beams: (`optional`) int
+                Number of beams for beam search. Must be between 1 and infinity. 1 means no beam search. Default to 1.
+            repetition_penalty: (`optional`) float
+                The parameter for repetition penalty. Between 1.0 and infinity. 1.0 means no penalty. Default to 1.0.
+            length_penalty: (`optional`) float Exponential penalty to the length. Default to 1.
+            num_return_sequences: (`optional`) int.
+                The number of independently computed returned sequences for each element in the batch. Default to 1.
+            min_len: (`optional`) int
+            no_repeat_ngram_size:  (`optional`) int. ban ngrams of this length from being repeated in the generated text


As discussed under @julien-c's comment above, remove all these arguments and replace them by default values in the pretrained model config.json.

thomwolf · 2020-03-05T21:48:43Z

src/transformers/pipelines.py

+            https://huggingface.co/transformers/model_doc/bart.html#transformers.BartForMaskedLM.generate
+
+        """
+        assert return_tensors or return_text


It's good practice to have an associated error message with assert when possible (here it is possible).

thomwolf · 2020-03-05T21:49:55Z

src/transformers/pipelines.py

+                    record["summary_token_ids"] = summary
+                if return_text:
+                    record["summary_text"] = self.tokenizer.decode(
+                        summary, skip_special_tokens=True, clean_up_tokenization_spaces=False


We could expose clean_up_tokenization_spaces as an argument I think

thomwolf · 2020-03-05T21:51:03Z

src/transformers/pipelines.py

+    def _forward(self, *args, **kwargs):
+        raise NotImplementedError("Should not be called")


Why do we have this method?

The parent implements it, so I didn't want to inherit a method that isn't supported and have users expect it to work.
I think since the method is private, I'll just delete this.

thomwolf · 2020-03-05T21:52:02Z

src/transformers/pipelines.py

+        "default": {
+            "model": {"pt": "bart-large-cnn", "tf": None},
+            "config": None,
+            "tokenizer": ("bart-large-cnn", {"use_fast": False}),


I don't think we have to prevent the (future) use of fast tokenizers here. Do we?

cc @mfuntowicz

All the other pipelines had that so I continued the practice, happy to go the other way.

In the 2.5.1, we disabled the use of fast tokenizers by default, and to be consistent, did the same for pipelines. Roberta still need some work to give the same output as Python.

=> I would recommend to stay like this (ie disabled by default) for now

Also, for use_fast to work, we need to provide a fast backend, inheriting from RobertaTokenizerFast (very similar example: https://github.com/huggingface/transformers/blob/master/src/transformers/tokenization_distilbert.py#L75).

sshleifer · 2020-03-15T02:42:53Z

I addressed all comments, and am ready for review @julien-c @thomwolf.

Weilin37 · 2020-03-23T20:58:17Z

Is this pipeline ready to go? When I tried to run an example it said that the summarization pipeline is not one of the options.

sshleifer · 2020-03-23T21:43:16Z

Hey, @Weilin37 .
Could you send a snippet of code so that I can reproduce your error?
Thanks!

julien-c · 2020-03-23T21:48:24Z

@Weilin37 are you running from master?

Weilin37 · 2020-03-24T18:21:37Z

@Weilin37 are you running from master?

Hi, yes it is resolved now. I thought I upgraded but it didn't

* passing * Undo stupid chg * docs * undo rename * delete-cruft * only import if you have torch * Dont rely on dict ordering * Fix dict ordering upstream * docstring link * docstring link * remove trailing comma for 3.5 compat * new name * delegate kwarging * Update kwargs

sshleifer added 5 commits March 4, 2020 17:59

passing

f1d4930

Undo stupid chg

d46a79a

docs

0ed6d2d

undo rename

8862122

delete-cruft

160e767

sshleifer requested review from julien-c, LysandreJik, mfuntowicz and patrickvonplaten March 4, 2020 23:26

sshleifer added 3 commits March 4, 2020 18:29

only import if you have torch

2fbdd0e

Dont rely on dict ordering

bfa1bad

Fix dict ordering upstream

597b362

sshleifer commented Mar 5, 2020

View reviewed changes

julien-c requested changes Mar 5, 2020

View reviewed changes

julien-c requested a review from thomwolf March 5, 2020 05:41

sshleifer added 3 commits March 5, 2020 10:19

docstring link

57481a7

docstring link

a489359

remove trailing comma for 3.5 compat

8e0d6d3

thomwolf requested changes Mar 5, 2020

View reviewed changes

sshleifer added 4 commits March 14, 2020 18:27

Merge branch 'master' into ss-summarization-to-pipelines

40b8840

new name

f78d85f

delegate kwarging

2746370

Update kwargs

a123599

patrickvonplaten approved these changes Mar 16, 2020

View reviewed changes

LysandreJik approved these changes Mar 17, 2020

View reviewed changes

julien-c merged commit 38a555a into master Mar 17, 2020

julien-c deleted the ss-summarization-to-pipelines branch March 17, 2020 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Summarization to Pipelines #3128

Add Summarization to Pipelines #3128

sshleifer commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 5, 2020 •

edited

Loading

sshleifer Mar 5, 2020

julien-c Mar 5, 2020

sshleifer Mar 5, 2020

julien-c Mar 6, 2020

julien-c Mar 5, 2020

thomwolf Mar 5, 2020

sshleifer Mar 5, 2020

thomwolf Mar 5, 2020 •

edited

Loading

julien-c Mar 5, 2020

thomwolf left a comment

thomwolf Mar 5, 2020

LysandreJik Mar 5, 2020

thomwolf Mar 5, 2020 •

edited

Loading

thomwolf Mar 5, 2020

thomwolf Mar 5, 2020

thomwolf Mar 5, 2020

sshleifer Mar 15, 2020

thomwolf Mar 5, 2020

sshleifer Mar 14, 2020

mfuntowicz Mar 16, 2020

sshleifer commented Mar 15, 2020

Weilin37 commented Mar 23, 2020

sshleifer commented Mar 23, 2020

julien-c commented Mar 23, 2020

Weilin37 commented Mar 24, 2020

		modelcard (:obj:`str` or :class:`~transformers.ModelCard`, `optional`, defaults to :obj:`None`):
		Model card attributed to the model for this pipeline.

		def _forward(self, args, *kwargs):
		raise NotImplementedError("Should not be called")

Add Summarization to Pipelines #3128

Add Summarization to Pipelines #3128

Conversation

sshleifer commented Mar 4, 2020 • edited Loading

codecov-io commented Mar 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwolf Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomwolf Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshleifer commented Mar 15, 2020

Weilin37 commented Mar 23, 2020

sshleifer commented Mar 23, 2020

julien-c commented Mar 23, 2020

Weilin37 commented Mar 24, 2020

sshleifer commented Mar 4, 2020 •

edited

Loading

codecov-io commented Mar 5, 2020 •

edited

Loading

thomwolf Mar 5, 2020 •

edited

Loading

thomwolf Mar 5, 2020 •

edited

Loading