[Proposal] Adding new `encoder_no_repeat_ngram_size` to `generate`. by Narsil · Pull Request #9984 · huggingface/transformers

Narsil · 2021-02-03T18:13:41Z

What does this PR do?

Blenderbot results seemed off compared to original ParlAI script:
https://parl.ai/projects/recipes/. Notably the model seems
to repeat a lot what was said during the conversation.

The actual problem was that no_repeat_ngram_size actually applies
to the encoder_input_ids but HF's no_repeat_ngram_size applies
to the previously generated ids (within the decoder). The history
conversation of blenderbot is within the encoder part so that
explains why HF's implementation had the repetitions.

This fix was focused on blenderbot not small and added tests
for those because they are quite different in configuration.

This change includes:

Adding a new EncoderNoRepeatLogitProcessor.
Adding 1 new arg to generate (encoder_no_repeat_ngram_size)
Adding 1 new config parameter encoder_no_repeat_ngram_size.
Adding 2 tests, one for the pipeline (high level, inputs exhibited
repeat behavior, one low level for EncoderNoRepeatLogitProcessor)
Factored NoRepeatLogitProcessor so that logic could be reused.

Further work:

Blenderbot conversational pipeline still does not behave correctly
as they way input is prepared within the pipeline is still incorrect
(follow up PR)
Blenderbot allows the bot to have personas, which is done by
prepending "your personna: XXXX" to the input, this could be explored
too in a follow up PR.

Fixes # (issue)

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

@patrickvonplaten
@LysandreJik

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors which may be interested in your PR.

@patrickvonplaten

Blenderbot results seemed off compared to original ParlAI script: `https://parl.ai/projects/recipes/`. Notably the model seems to repeat a lot what was said during the conversation. The actual problem was that `no_repeat_ngram_size` actually applies to the `encoder_input_ids` but HF's `no_repeat_ngram_size` applies to the previously generated ids (within the decoder). The history conversation of blenderbot is within the `encoder` part so that explains why HF's implementation had the repetitions. This fix was focused on blenderbot *not* small and added tests for those because they are quite different in configuration. This change includes: - Adding a new EncoderNoRepeatLogitProcessor. - Adding 1 new arg to `generate` (`encoder_no_repeat_ngram_size`) - Adding 1 new config parameter `encoder_no_repeat_ngram_size`. - Adding 2 tests, one for the pipeline (high level, inputs exhibited repeat behavior, one low level for EncoderNoRepeatLogitProcessor) - Factored NoRepeatLogitProcessor so that logic could be reused. Further work: - Blenderbot conversational pipeline still does not behave correctly as they way input is prepared within the pipeline is still incorrect (follow up PR) - Blenderbot allows the bot to have personas, which is done by prepending "your personna: XXXX" to the input, this could be explored too in a follow up PR. @patrickvonplaten @LysandreJik

patrickvonplaten

Looks great to me! Thanks so much for diving into this and solving the blenderbot bug!
Like the design very much as discussed offline! Super nice to be able to solve the problem in such a clean way :-)

Left a couple of nits. But overall looks good to me!

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

LysandreJik

LGTM, this is indeed a clean fix. Do we know why our BlenderBot still behaves incorrectly compared to ParlAI?

Regarding personas, this could probably be handled directly in the ConversationalPipeline?

LysandreJik · 2021-02-04T09:19:41Z

Before merging, please take a look at the failing tests.

Narsil · 2021-02-04T09:57:01Z

LGTM, this is indeed a clean fix. Do we know why our BlenderBot still behaves incorrectly compared to ParlAI?

I need to look deeper, by default they use FP16 and final scores are still different in order of magnitude (I'm expecting they correspond to different things), but when looking at the full beam searches they still look similar.

I've done step by step debugging and scores withing the beam search are super close for a lot of steps.
This fix is the major drift that would occur pretty fast.

Regarding personas, this could probably be handled directly in the ConversationalPipeline?

Yes exactly my opinion.

patrickvonplaten · 2021-02-04T10:01:29Z

+    def __call__(self, input_ids: torch.LongTensor, scores: torch.FloatTensor) -> torch.FloatTensor:
+        # B x num_beams
+        num_hypos = scores.shape[0]
+        num_beams = num_hypos // self.batch_size


nice - yeah that's safe!

Narsil · 2021-02-04T10:10:38Z

@sgugger Can you take a look please?

Narsil · 2021-02-04T10:38:52Z

@LysandreJik figured it out. Its' because of some logic within ConversationPipeline which is invalid for blenderbot.

Coming up with a follow-up PR.

sgugger

LGTM, thanks for adding this!

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/generation_logits_process.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/generation_utils.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/models/blenderbot/configuration_blenderbot.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/generation_utils.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/generation_utils.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/generation_utils.py Outdated

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

Comment thread src/transformers/configuration_utils.py Outdated

patrickvonplaten approved these changes Feb 4, 2021

View reviewed changes

Narsil and others added 7 commits February 4, 2021 09:31

Update src/transformers/generation_logits_process.py

fb04f9c

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

Update src/transformers/generation_utils.py

fff75f6

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

Update src/transformers/generation_utils.py

6029561

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

Update src/transformers/configuration_utils.py

c5f0bcd

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>

Doc quality.

b2fc8d1

Fixing test.

baff66a

Last fixes.

48de309

patrickvonplaten requested review from LysandreJik and sgugger February 4, 2021 08:52

LysandreJik approved these changes Feb 4, 2021

View reviewed changes

Fixing to account for batch_size.

8a2920e

patrickvonplaten reviewed Feb 4, 2021

View reviewed changes

sgugger approved these changes Feb 4, 2021

View reviewed changes

Comment thread src/transformers/configuration_utils.py Outdated

Comment thread src/transformers/generation_utils.py Outdated

Narsil and others added 2 commits February 4, 2021 14:30

Update src/transformers/configuration_utils.py

65de602

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Update src/transformers/generation_utils.py

3c74a37

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Narsil merged commit aeb18b9 into huggingface:master Feb 4, 2021

Narsil deleted the encoder_no_repeat_ngram_size branch February 4, 2021 14:00

patil-suraj mentioned this pull request Feb 9, 2021

[RAG] fix generate #10094

Merged

Narsil mentioned this pull request Apr 6, 2021

[Blenderbot] Model yields weird results #9457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] Adding new `encoder_no_repeat_ngram_size` to `generate`.#9984

[Proposal] Adding new `encoder_no_repeat_ngram_size` to `generate`.#9984
Narsil merged 11 commits into
huggingface:masterfrom
Narsil:encoder_no_repeat_ngram_size

Narsil commented Feb 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickvonplaten left a comment •

edited

Loading

Uh oh!

LysandreJik left a comment

Uh oh!

LysandreJik commented Feb 4, 2021

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

patrickvonplaten Feb 4, 2021

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

sgugger left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Narsil commented Feb 3, 2021

What does this PR do?

Before submitting

Who can review?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickvonplaten left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LysandreJik left a comment

Choose a reason for hiding this comment

Uh oh!

LysandreJik commented Feb 4, 2021

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

patrickvonplaten Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

Narsil commented Feb 4, 2021

Uh oh!

sgugger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patrickvonplaten left a comment •

edited

Loading