-
Notifications
You must be signed in to change notification settings - Fork 2k
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
refactor: TableReader #3456
refactor: TableReader #3456
Conversation
…r to Retriever Encoder design pattern
The auto-assignment of reviewers added me here but I agree with Sebastian that Bogdan is the best person to review this PR. Therefore, I unassigned myself. Just let me know if you need a second opinion on the PR. My highlight of this PR are the smaller tapas models in the tests. Thanks for speeding them up! 👍 |
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 for this PR, looks very good already :) I only left two minor suggestions
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.
LGTM, just one very small nitpick :)
a2a471b
to
8de16c6
Compare
Related Issues
TableReader
#1723Proposed Changes:
I refactored the
TableReader
class to make it easier to address issue #1723, which will be solved by either supporting a new table encoder or by implementing a sliding window approach. There will be a follow-up PR to finish solving issue #1723.TapasForScoredQA
architecture.How did you test it?
The API for loading a
TableReader
has not changed so existing unit and integration tests all pass.Notes for the reviewer
@bogdankostic I'd appreciate a review of this refactor since I'm changing your original implementation. Please let me know if you have any comments or concerns.
Checklist