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

Prefix match in first iteration of beam search OP #10231

Merged
merged 64 commits into from
Feb 2, 2022

Conversation

viboga
Copy link
Contributor

@viboga viboga commented Jan 10, 2022

Description: This PR targets required changes to allow prefix matching in beam search OP.

Motivation and Context

  • Why is this change required? What problem does it solve?
    In generative models, prefix of the last word needs to be matched with the first word being generated.

  • If it fixes an open issue, please link to the issue here.
    NA

** Tests :**
Attached test report
report.docx

@viboga
Copy link
Contributor Author

viboga commented Jan 12, 2022

Why not add an attribute to indicate that vocab_mask is used in first step only (default is false)? In this way, the change could be small.

@tianleiwu Are you saying use the vocab_mask already present to prefix match instead of using it on every iteration?

@viboga viboga marked this pull request as ready for review January 12, 2022 19:09
@tianleiwu
Copy link
Contributor

Why not add an attribute to indicate that vocab_mask is used in first step only (default is false)? In this way, the change could be small.

@tianleiwu Are you saying use the vocab_mask already present to prefix match instead of using it on every iteration?

Right. I think we only need add a new attribute (like is_prefix_vocab_mask with default value 0) so that user could choose whether using it on first iteration or every iteration.

@edgchen1
Copy link
Contributor

/azp run onnxruntime-binary-size-checks-ci-pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@viboga
Copy link
Contributor Author

viboga commented Jan 14, 2022

@tianleiwu
Wanted to make sure I understood your suggestion. The purpose of vocab_mask and prefix_mask is mutually exclusive.

The vocab_mask you have added is to limit the search space of the suggestions. This is a generic implementation which is an option at runtime. You can change the vocab_mask to change the search space/suggestions.

Prefix_mask on first iteration however is to match the incomplete last word of the input. Indeed, it is very specific to an input string and tokenizer. Both can be used on a model as needed.

@tianleiwu
Copy link
Contributor

tianleiwu commented Jan 18, 2022

@tianleiwu Wanted to make sure I understood your suggestion. The purpose of vocab_mask and prefix_mask is mutually exclusive.

The vocab_mask you have added is to limit the search space of the suggestions. This is a generic implementation which is an option at runtime. You can change the vocab_mask to change the search space/suggestions.

Prefix_mask on first iteration however is to match the incomplete last word of the input. Indeed, it is very specific to an input string and tokenizer. Both can be used on a model as needed.

It depends on the usage. When there is need to have both prefix matching and bad word list at the same time, we shall separate them. Otherwise, I think it is better to consolidate them since prefix mask is just like vocab_mask applied only to first iteration.

@viboga viboga requested a review from tianleiwu January 26, 2022 18:43
@@ -9,6 +9,8 @@ namespace onnxruntime {
namespace contrib {
namespace transformers {

static int beam_search_iteration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better not be stored as a global variable. (Consider that two nodes executed at the same time, that means this code is not thread safe).

Could we pass the parameter in Process function instead?

Copy link
Contributor Author

@viboga viboga Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viboga viboga requested a review from tianleiwu February 1, 2022 18:14
@viboga viboga merged commit ad9d2e2 into master Feb 2, 2022
@viboga viboga deleted the Vish/changes_for_prefix_matching branch February 2, 2022 18:44
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.

3 participants