-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
fixing prefix_allowed_tokens_fn #3276
fixing prefix_allowed_tokens_fn #3276
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I'm interested in seeing this land, but I was curious if it would be possible to also include a bit of documentation about this |
@erip you are right. Where do you think is the best place to write the signature of the function? here? https://github.com/pytorch/fairseq/blob/e5e8b3fee1e57a7abf35ad1a3ff223a2b7190c65/fairseq/search.py#L148 |
@nicola-decao I think that makes good sense. There doesn't seem to be any documentation on the other search strategies, but this one is somewhat less straightforward since it's got the callback. Unless @myleott has other thoughts, I think throwing a docstring beneath the ctor would be great. |
@nicola-decao if you can share a docstring here, I can update the imported version before merging |
@myleott Here you go: prefix_allowed_tokens_fn: |
@myleott Any news on this? Is there something I should do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@myleott has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@myleott Any news on this? I have a facebook AI project https://github.com/facebookresearch/GENRE that depends on this bug fix (for now I link people to my fork with the fix that is not ideal). |
@myleott @sshleifer can we please proceed on the merge here? It is really a minor change.
|
also cc @alexeib |
Summary: # Before submitting - [ ] Was this discussed/approved via a Github issue? (no need for typos, doc improvements) - [x] Did you read the [contributor guideline](https://github.com/pytorch/fairseq/blob/master/CONTRIBUTING.md)? - [x] Did you make sure to update the docs? - [x] Did you write any new necessary tests? ## What does this PR do? Fixes the use of `prefix_allowed_tokens_fn` in generation. It was working for `fairseq==0.9.0` (see https://github.com/facebookresearch/GENRE) but with the current version is broken. ## PR review Anyone in the community is free to review the PR once the tests have passed. ## Did you have fun? Make sure you had fun coding � Pull Request resolved: #3276 Reviewed By: alexeib Differential Revision: D26725494 Pulled By: myleott fbshipit-source-id: ce3da725f36352687e5cb5d62a59b4c89ce0b0bc
Before submitting
What does this PR do?
Fixes the use of
prefix_allowed_tokens_fn
in generation. It was working forfairseq==0.9.0
(see https://github.com/facebookresearch/GENRE) but with the current version is broken.PR review
Anyone in the community is free to review the PR once the tests have passed.
Did you have fun?
Make sure you had fun coding 🙃