Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Constrained beam search duplication and weird output issue #17814

Conversation

boy2000-007man
Copy link
Contributor

What does this PR do?

  • prevent duplicates between (topk) generic beam search best model next tokens and (advance) constraints forcing the next token
  • ensure unfulfilled hypothesis will advance with correct beam score instead of wrong token score

Fixes # (issue)

#17812

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

@boy2000-007man boy2000-007man changed the title Constrained beam search duplication weird fix Fix Constrained beam search duplication and weird output issue Jun 22, 2022
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 22, 2022

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

@patrickvonplaten
Copy link
Contributor

@cwkeam - do you have an idea here?

@cwkeam
Copy link
Contributor

cwkeam commented Jun 23, 2022

Good catch @boy2000-007man !! @patrickvonplaten I just read through the issue and saw the code changes and they're all right-on. Thanks for finding these problems!

@patrickvonplaten patrickvonplaten merged commit bc7a6fd into huggingface:main Jun 24, 2022
@boy2000-007man boy2000-007man deleted the constrained-beam-search-duplication-weird-fix branch June 24, 2022 20:51
@AncalagonX
Copy link

Thank you for this fix! I found this pull request while I was searching to figure out why constrained beam search was churning out such repetitive results on 4.20.1. Installing the current repo of transformers fixed this immediately!

younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 25, 2022
…ngface#17814)

* fix(ConstrainedBeamSearchScorer.step_sentence_constraint): avoid hypothesis duplication between topk and advance

* fix(GenerationMixin.constrained_beam_search): appropriately assign beam scores instead of token scores
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 29, 2022
…ngface#17814)

* fix(ConstrainedBeamSearchScorer.step_sentence_constraint): avoid hypothesis duplication between topk and advance

* fix(GenerationMixin.constrained_beam_search): appropriately assign beam scores instead of token scores
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants