-
Notifications
You must be signed in to change notification settings - Fork 37
Fairseq v0.10.2 compatible #104
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
Conversation
|
Avoid checking in those txt files and old backup file - "generate_old.py". |
feihugis
left a comment
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.
Thanks @JulianneKnott for this PR! I did a pass and have one question for the files under results: are they used for comparing the outputs from fastseq and fairseq? It will be very helpful if we can add a unit test to automatically check it. We can also integrate it with our CI in the next step.
Yes. Didn't mean to commit them and will remove them on the next commit. Will look into adding a unit test also. |
|
Benchmark Info:
|
feihugis
left a comment
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.
The current code looks good to me. As this change will break the other parts of the main branch, could we create another branch (e.g., fairseq-0.10.2) for this work? After finishing all the tasks, it can be merged to the main branch.
|
Updated beam search optimizer.
|
|
Updated el attention optimizer
|
feihugis
left a comment
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.
Thanks @JulianneKnott ! LGTM. Just find some very minor issues. After fixing, the PR will be good to merge.
|
|
||
| def step(self, step, lprobs, scores): | ||
| super()._init_buffers(lprobs) | ||
| class BeamSearch(BeamSearch): |
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.
I'm not sure if people will get confused when the child class has the same name with the parent class? What do @JulianneKnott think of it?
yuyan2do
left a comment
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.
Looks good. Just added a few suggestions inline.
Another question: Is there any test cover converting a model to its TorchScript version? If not, we need do it manually to verify our change do not break this functionality.
| metavar='N', | ||
| help='number of worker for post process') | ||
| parser.add_argument( | ||
| '--decode_hypothesis', |
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.
Where is this param used?
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.
fastseq/fastseq/optimizer/fairseq/generate.py
Line 152 in 4a3f17e
| if args.decode_hypothesis: |
Updated generate.py to be compatible with Fairseq v0.10.2.
Speed tbd.