Merge bart generate into default generate #3140

patrickvonplaten · 2020-03-05T15:16:25Z

This PR is a first version of how the bart generation() code can be merged into the "default" generation function. I think it's actually much better feasible than we originally thought.
Please not that this change also includes all the changes from PR #3135 , so the code changes will be much less cluttered after #3135 merged.

This version was passes the general random language generation tests in found in test_modeling_common.py and the easy integration test with the original fairseq model (renamed to test_cnn_summarization_same_as_fairseq_easy in test_modeling_bart)

There are a couple of things we should discuss:

Both Bart generate() and default generate(), encoder-decoder models must have a BOS token and an EOS token.
Two new parameters were added: min_length and no_repeat_ngram_size . I think these parameters should be added generally as it is done now.
There was one hack which initializes the decoder_input_ids to the EOS token and then forces the model to generate the BOS token afterwards (see comment in code line). I changed it to simply start with the BOS token (which makes more sense) and it also passed the "easy integration tests". This hack might be needed to pass the hard integration test though.
Fairseq forces the last token of all beam hypotheses to be the EOS token (see comment in line). This is probably necessary to pass the integration tests. It's up for debate whether this the correct way. I would prefer not to do it this way because it will then be impossible to generate unfinished sentences (sentence that end because they hit max_length). If one really wants all beam hypotheses to be finished, one could set the max_length higher than usual and set the parameter:
self.early_stopping in the Beam Hypotheses class to True. Up for debate how to handle this.
In order to also pass the hard integration tests (which has a padded batch as an input), we will have to add attention_masks to the generate() function. Here I see three possibilities:
a) add the attention_mask as a parameter to the generation() function.
b) automatically calculate the attention_mask from the input_ids if the model has a pad_token_id and there is a pad_token_id in the input_ids.
c) Not allow padded batches for the moment.
I would prefer option b) because some models do have a set pad_token_id (such as Bart) so we should be able to allow padded generation.

src/transformers/modeling_utils.py

sshleifer · 2020-03-05T15:55:20Z

clarification:

bart.generate doesn't add EOS if max_length is hit, or require EOS to pass integration tests. It just "finalizes" a hypothesis when the model predicts EOS for a beam.

Example:

ARTICLE_TO_SUMMARIZE = "code doesnt generate EOS if max_length is hit"
inputs = tokenizer.batch_encode_plus([ARTICLE_TO_SUMMARIZE], return_tensors='pt')

generated_ids = model.generate(inputs['input_ids'], attention_mask=inputs['attention_mask'], num_beams=4, max_length=5)
summary_text = tokenizer.decode(generated_ids[0])
print(generated_ids[0], summary_text)
# (tensor([   0, 2387,  964,   32, 3035]), '<s>My friends are cool')

thomwolf

Ok good, just a fix on the config.

Regarding the question of the attention mask, here is my (more general) take on it:

In the library, we've initially decided not to mix models and tokenizer and as thus, ask the users to compute the attention mask outside of the forward pass (currently provided by the tokenizers).

Looking back on it, I think it was a good idea to split the two building blocks from the point of view of allowing the user to better understand what's happening and organize his workflow. On the other hand, the very specific case of designing an attention mask has become a typical silent error for many people using the models: they pad the input in batches and then forget to feed the attention mask to the model forward pass.

As we start having a common configuration file, the padding token index is now available in the model and I think we could propose a general (non-breaking) evolution of this behavior for the users, to reduce this source of silent mistake.

I would thus like to propose the following workflow for the forward pass of all models:

if an attention_mask is provided, we use it (like now)
if no attention_mask is provided but we have a padding_token_id in the configuration, we build a specific attention_mask for the input from the padding token (new behavior, could be breaking in some hacky usages of the lib where a padding token is used for something other than padding...)
if no attention_mask is provided and we don't have a padding_token_id in the configuration, we build a generic full attention attention_mask (like we did before).

What do you think especially @LysandreJik and @julien-c

thomwolf · 2020-03-06T09:55:39Z

src/transformers/modeling_utils.py

@@ -806,37 +816,66 @@ def generate(
                effective_batch_size * num_beams, input_ids_len
            )  # shape: (batch_size * num_return_sequences * num_beams, cur_len)

+        # TODO (PVP): probably not the best way to check whether model is encoder decoder
+        is_encoder_decoder = (


Good point, this something to clean, let's add a is_encoder_decoder attribute in the configuration of Bart and T5, ok?
cc @sshleifer

We should clean up the T5 implementation also, this is_decoder flag should not be in the config but should be an argument to the __init__ method:

transformers/src/transformers/modeling_t5.py

Line 193 in 7336896

self.is_decoder = config.is_decoder

src/transformers/configuration_utils.py

julien-c · 2020-03-06T20:57:19Z

I would thus like to propose the following workflow for the forward pass of all models:
[...]
What do you think especially @LysandreJik and @julien-c

Sounds good to me

thomwolf · 2020-03-06T20:59:10Z

By the way, my workflow proposition actually implied that we should use the same workflow and inputs for the generate() method as well (I could have been more explicit)

src/transformers/modeling_utils.py

sshleifer

Would love to give it a deeper read before you merge, Ping me!

sshleifer · 2020-03-06T23:12:18Z

src/transformers/modeling_utils.py

-                eos_token_id,
-#                bos_token_id,
-                #                eos_token_id, # Why eos_token_id here? bos_token_id seems to work as well ... to see if it works as well with hard summarization case
+                eos_token_id,  # TODO (PVP): to check if this is the only solution -> quite hacky to do this


fairseq way is even crazier! look for prefix_tokens in sequence_generator.py

Ok, not sure whether all encoder-decoder models should start with eos_token_id though. Happy to discuss! @thomwolf @sshleifer

Why should the encoder-decoder output sequence start with eos_token_id.
This seems both weird and probably not generalizable to models behind Bart to me.
Can you explain this better for me?

(read the prefix tokens comment before this one)
My understanding is that since we know we are going to generate BOS first, we want the score associated with BOS to be P(BOS | EOS), because that seems reasonable. So the first step, even though we will force BOS, we send EOS as input_ids to get the appropriate score.

tests/test_modeling_bart.py

codecov-io · 2020-03-07T10:53:47Z

Codecov Report

Merging #3140 into master will decrease coverage by 0.15%.
The diff coverage is 74.71%.

@@            Coverage Diff             @@
##           master    #3140      +/-   ##
==========================================
- Coverage   78.14%   77.99%   -0.16%     
==========================================
  Files          98       98              
  Lines       16668    16665       -3     
==========================================
- Hits        13026    12998      -28     
- Misses       3642     3667      +25

Impacted Files	Coverage Δ
src/transformers/configuration_bart.py	`100% <ø> (ø)`	⬆️
src/transformers/configuration_utils.py	`96.82% <100%> (+0.07%)`	⬆️
src/transformers/modeling_bart.py	`96.27% <100%> (+2.69%)`	⬆️
src/transformers/configuration_t5.py	`96.55% <100%> (ø)`	⬆️
src/transformers/modeling_tf_utils.py	`87.47% <46.96%> (-6.27%)`	⬇️
src/transformers/modeling_utils.py	`93.84% <90.32%> (-0.57%)`	⬇️

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 d6de642...bc9d5d9. Read the comment docs.

patrickvonplaten · 2020-03-07T11:04:49Z

src/transformers/modeling_utils.py

+            scores = F.log_softmax(next_token_logits, dim=-1)  # (batch_size * num_beams, vocab_size)
+            if (
+                self.config.is_encoder_decoder and do_sample is False
+            ):  # TODO(PVP) to be refactored later - do we need this boolean flag here? Also Only add for beam_search or also for no_beam_search? The prepare scores fn is ugly here


Very hacky here - don't like the if statement and the prepare_scores_for_generation fn too much. Better ideas? @sshleifer @thomwolf

I agree and I don't understand why we need the prepare_scores_for_generation method (cf. my comment on it).
Let's talk about it @sshleifer @patrickvonplaten

patrickvonplaten · 2020-03-07T11:06:21Z

UPDATE: This version passes all integration tests now. There are two things which I quite hacky we probably should implement in a cleaner way:

the function prepare_scores_for_generation is a hacky function to make Bart pass the integration tests. See in code-line comment
initializing all encoder-decoder models with the EOS token is probably not the right thing to do. See in code-line comment.

@sshleifer @thomwolf @LysandreJik

src/transformers/modeling_utils.py

thomwolf

Ok it's great to see a convergence here.

As mentioned in the comment there are a couple of method that I don't understand and which seems to be potentially very Bart's specific so I'm not sure we want them in the base class (I don't really understance their use).

Let's discuss them before merging.

src/transformers/modeling_utils.py

thomwolf · 2020-03-08T16:42:39Z

src/transformers/modeling_utils.py

+                device=next(self.parameters()).device,
+            )
+            cur_len = 1
+            self.model.decoder.generation_mode = True


I don't like this generation_mode in Bart which this rather specific to the fairseq way to do it and probably won't be used in other encoder-decoder models.

I'll remove it in my review of Bart if I can. In the meantime for these model specific stuff we can either (1) activate it in the model specific prepare_inputs_for_generation or (2) run behind a if hasattr(self.model, 'generation_mode') test.

thomwolf · 2020-03-08T16:44:20Z

src/transformers/modeling_utils.py


        if num_beams > 1:
            output = self._generate_beam_search(
                input_ids,
                cur_len,
                max_length,
+                min_length,


For these long list of arguments, I think it's safer to have them all called by keyword names:

max_length=max_length, min_length=min_length,

I know it's longer to write but it's more robust to changes in these methods API later on.

Though if you do it, be careful not to introduce bugs in LHS, RHS equality, better to do it will multi-cursors if you can.

sounds good

thomwolf · 2020-03-08T16:48:39Z

src/transformers/modeling_utils.py

-                eos_token_id,
-#                bos_token_id,
-                #                eos_token_id, # Why eos_token_id here? bos_token_id seems to work as well ... to see if it works as well with hard summarization case
+                eos_token_id,  # TODO (PVP): to check if this is the only solution -> quite hacky to do this


Why should the encoder-decoder output sequence start with eos_token_id.
This seems both weird and probably not generalizable to models behind Bart to me.
Can you explain this better for me?

src/transformers/modeling_utils.py

thomwolf · 2020-03-08T16:56:13Z

src/transformers/modeling_bart.py

        }

+    def prepare_scores_for_generation(self, scores, cur_len, max_length):


This method is really weird. Do we need that?
Can one of you explain this to me? @sshleifer @patrickvonplaten

fairseq generation (for all models) has the following rules:

Never generate pad or unk

if we have hit the max_length constraint, force the model to predict EOS.

when I say force, I mean set all other words' scores to -inf, but keep the true probability of the forced token so that the hypothesis is scored accurately.

Optional tensor argument prefix_tokens: for each step i, force the model to predict prefix_tokens[:, step]

for bart the prefix tokens are a (batch_size,1) tensor filled with BOS

for language_modeling, I believe the prefix tokens are the whole input_ids sequence.

(what they call src_tokens, we call input_ids)

That's what this function is doing, deciding what to force based on the step.

thomwolf · 2020-03-08T16:57:31Z

src/transformers/modeling_utils.py

+            scores = F.log_softmax(next_token_logits, dim=-1)  # (batch_size * num_beams, vocab_size)
+            if (
+                self.config.is_encoder_decoder and do_sample is False
+            ):  # TODO(PVP) to be refactored later - do we need this boolean flag here? Also Only add for beam_search or also for no_beam_search? The prepare scores fn is ugly here


I agree and I don't understand why we need the prepare_scores_for_generation method (cf. my comment on it).
Let's talk about it @sshleifer @patrickvonplaten

src/transformers/modeling_utils.py

sshleifer

Super interesting/difficult PR. I left comments explaining some of the tricks.

High Level Points:

Nearly all of the tricks are from fairseq's sequence_generator.py which is used for nearly all conditional generation as far as I can tell.
I still think it is somewhat better to wait for this refactor until we have more seq2seq callers, but I don't feel strongly and am open to being outvoted.
Happy to discuss on any forum!

sshleifer · 2020-03-08T18:45:01Z

src/transformers/modeling_bart.py

        }

+    def prepare_scores_for_generation(self, scores, cur_len, max_length):


fairseq generation (for all models) has the following rules:

Never generate pad or unk

if we have hit the max_length constraint, force the model to predict EOS.

when I say force, I mean set all other words' scores to -inf, but keep the true probability of the forced token so that the hypothesis is scored accurately.

Optional tensor argument prefix_tokens: for each step i, force the model to predict prefix_tokens[:, step]

for bart the prefix tokens are a (batch_size,1) tensor filled with BOS

for language_modeling, I believe the prefix tokens are the whole input_ids sequence.

(what they call src_tokens, we call input_ids)

That's what this function is doing, deciding what to force based on the step.

src/transformers/modeling_utils.py

sshleifer · 2020-03-08T19:04:11Z

src/transformers/modeling_utils.py

-                eos_token_id,
-#                bos_token_id,
-                #                eos_token_id, # Why eos_token_id here? bos_token_id seems to work as well ... to see if it works as well with hard summarization case
+                eos_token_id,  # TODO (PVP): to check if this is the only solution -> quite hacky to do this


(read the prefix tokens comment before this one)
My understanding is that since we know we are going to generate BOS first, we want the score associated with BOS to be P(BOS | EOS), because that seems reasonable. So the first step, even though we will force BOS, we send EOS as input_ids to get the appropriate score.

patrickvonplaten · 2020-03-08T21:25:50Z

@thomwolf, @sshleifer Thanks for your reviews guys. I think we are on the same page except for two things:

Force tokens to be generated. The BOS token is generated at step=0 and the EOS token is generated at step=max_length. This is necessary to reproduce the original fairseq results. Should we keep that as a model specific "prepare" function?
Use the EOS token for the Bart models. This is also necessary to reproduce the original fairseq results. Should we keep that?

My opinion is:

I would remove both statements that force a certain token to be predicted. Even without doing it, Bart produces (in my opinion) good summarization results.
Same goes for this point. Would always use BOS token as the starting decoder_input_ids for encoder-decoder models and force encoder-decoder models to have a BOS token to be able to do language generation.

Doing this would means, we would have to change the integration tests and we won't produce 1:1 the same results as fairseq anymore.

@sshleifer you can probably estimate the consequences for the Bart summarization quality much better than me! From the examples in the test_modeling_bart.py, I ran the summarization and the output looked good to me, but didn't measure ROUGE scores or anything...

What do you guys think?

sshleifer · 2020-03-09T20:56:01Z

I ran eval on the full CNN test set with these simplifications, and Rouge decreases from .21072 to .20285.

For context, here are the published Rouge-2 scores of a bunch of different models:

Note: the published bart score is a bit higher (.2128) because there are even more tricks I didn't implement.

thomwolf · 2020-03-11T12:21:45Z

Ok let's merge this to be able to move forward with T5 cc @craffel
@julien-c @LysandreJik we think the self-hosted failing tests are not related to this.
Can you have a look later maybe?

thesamuel · 2020-03-11T19:24:57Z

@patrickvonplaten You mentioned a way to perform batch inference with GPT2LMHeadModel using an attention mask here: #3021 (comment).

Does this PR make this possible by calling model.generate(input_ids=..., attention_mask=...)?

patrickvonplaten · 2020-03-11T20:35:57Z

@patrickvonplaten You mentioned a way to perform batch inference with GPT2LMHeadModel using an attention mask here: #3021 (comment).

Does this PR make this possible by calling model.generate(input_ids=..., attention_mask=...)?

Hi @thesamuel,

Not yet completely. It's one step to make the generation possible as shown in #3021, but there are still two things that are not considered yet in the generate fn:

The position embeddings have to be updated - which generate() does not do yet
And this is the hard one: If a padded batch is given as an input, it should not be sampled from the last token, but from the last non-padded token and this can be quite hacky.

We are currently thinking about how to implement this!

thesamuel · 2020-03-11T23:27:50Z

@patrickvonplaten Got it, thanks!

sshleifer · 2020-03-12T14:32:01Z

(Investigating) This PR may introduce a BOS bug that reduces rouge to 15.068 from 21.28

* Update bart example docs

sshleifer · 2020-03-12T14:56:01Z

Simple bug caused by do_sample (which for some reason defaults to True). Anyway, I'm rerunning rouge but it will likely be at a reasonable level.

patrickvonplaten · 2020-03-12T19:29:15Z

Possible follow-up states are explained in PR: #3225

…3233) * Update bart example docs

patrickvonplaten commented Mar 5, 2020

View reviewed changes

src/transformers/modeling_utils.py Show resolved Hide resolved

patrickvonplaten commented Mar 5, 2020

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

patrickvonplaten commented Mar 5, 2020

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

patrickvonplaten commented Mar 5, 2020

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

patrickvonplaten requested review from sshleifer, LysandreJik and thomwolf March 5, 2020 16:05

thomwolf mentioned this pull request Mar 5, 2020

Add Summarization to Pipelines #3128

Merged

patrickvonplaten force-pushed the merge_bart_generate_into_default_generate branch from 1963a94 to 87a78f6 Compare March 5, 2020 22:20

thomwolf reviewed Mar 6, 2020

View reviewed changes

patrickvonplaten force-pushed the merge_bart_generate_into_default_generate branch from 87a78f6 to c4da5cc Compare March 6, 2020 13:39

sshleifer reviewed Mar 6, 2020

View reviewed changes

src/transformers/modeling_utils.py Show resolved Hide resolved

sshleifer reviewed Mar 6, 2020

View reviewed changes

patrickvonplaten requested review from thomwolf and sshleifer March 7, 2020 11:00

patrickvonplaten commented Mar 7, 2020

View reviewed changes

patrickvonplaten changed the title ~~[Discussion] Merge bart generate into default generate~~ Merge bart generate into default generate Mar 7, 2020

patrickvonplaten commented Mar 7, 2020

View reviewed changes

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved

patrickvonplaten force-pushed the merge_bart_generate_into_default_generate branch from a4ba33c to eb3ba73 Compare March 8, 2020 13:26

thomwolf reviewed Mar 8, 2020

View reviewed changes

sshleifer reviewed Mar 8, 2020

View reviewed changes

patrickvonplaten force-pushed the merge_bart_generate_into_default_generate branch 2 times, most recently from f173dd5 to 4f9d1f2 Compare March 9, 2020 16:00

patrickvonplaten added 9 commits March 11, 2020 11:06

fix torch to tf translation

a2c8e51

fixed typo

374deef

remove ipdb

cf06290

rename variable

1098971

do not mess with the negative sign

ca1330f

delete print and make style

9b8ee8c

re-add scoring filtering

7351a8d

fix typo

d997ac7

fix bug in tf no_repeat_ngram_size

1ba21f9

patrickvonplaten force-pushed the merge_bart_generate_into_default_generate branch from 9b12642 to 1ba21f9 Compare March 11, 2020 10:08

patrickvonplaten added 3 commits March 11, 2020 11:53

finalize generation merge

a332cc9

make all tensors half precision

bc9d5d9

fix problem with half

ac303ea

thomwolf merged commit db29ffc into huggingface:master Mar 11, 2020

patrickvonplaten deleted the merge_bart_generate_into_default_generate branch March 11, 2020 13:15

patrickvonplaten mentioned this pull request Mar 11, 2020

Complete merge Seq-2-Seq generation into default generation #3225

Merged

sshleifer added a commit that referenced this pull request Mar 12, 2020

Bart: update example for #3140 compatibility (#3233)

2e81b9d

* Update bart example docs

This was referenced Mar 13, 2020

[BART] test_generate_fp16 fails after PR#3140 #3265

Closed

[BART] FP16 testing fixes #3266

Merged

jplu pushed a commit to jplu/transformers that referenced this pull request Mar 25, 2020

Bart: update example for huggingface#3140 compatibility (huggingface#…

6e500bc

…3233) * Update bart example docs

This was referenced Apr 9, 2020

❓ In BART, why forcing the first token to BOS ? #3668

Closed

Seq2seq generation with prefix #3739

Closed

abisee mentioned this pull request Oct 27, 2020

Commit 121dd43 changes DialoGPT generation behavior #8032

Closed

4 tasks

		}

		def prepare_scores_for_generation(self, scores, cur_len, max_length):

Merge bart generate into default generate #3140

Merge bart generate into default generate #3140

Conversation

patrickvonplaten commented Mar 5, 2020 • edited Loading

sshleifer commented Mar 5, 2020

thomwolf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-c commented Mar 6, 2020

thomwolf commented Mar 6, 2020

sshleifer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten commented Mar 7, 2020

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten commented Mar 8, 2020

sshleifer commented Mar 9, 2020

thomwolf commented Mar 11, 2020

thesamuel commented Mar 11, 2020

patrickvonplaten commented Mar 11, 2020

thesamuel commented Mar 11, 2020

sshleifer commented Mar 12, 2020 • edited Loading

sshleifer commented Mar 12, 2020

patrickvonplaten commented Mar 12, 2020

patrickvonplaten commented Mar 5, 2020 •

edited

Loading

thomwolf left a comment •

edited

Loading

sshleifer left a comment •

edited

Loading

patrickvonplaten Mar 7, 2020 •

edited

Loading

codecov-io commented Mar 7, 2020 •

edited

Loading

sshleifer commented Mar 12, 2020 •

edited

Loading