Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

[RAG] Fix Extra Positions Bug #4389

Merged
merged 2 commits into from
Mar 3, 2022
Merged

[RAG] Fix Extra Positions Bug #4389

merged 2 commits into from
Mar 3, 2022

Conversation

klshuster
Copy link
Contributor

Patch description
--n-extra-positions can be used to expand the total number of positions used when encoding documents + input in pre-trained transformers. If we set this >0, then we are supposed to fill up all of end positions with the doc tokens, leaving the beginning positions for the input text.

In the current RAG implementation, if we specify --n-extra-positions M < --n-positions N, we encounter an issue where the documents were not properly truncated. This is found in the concat_docs_and_input function, where self.expanded_input_truncate is used to determine the maximum length of the documents. This is an overloaded attribute, hence why the bug was a bit subtle.

This bug was not previously caught, as in all of my experiments, --n-extra-positions > --n-positions.

Testing steps
Added a test to confirm that concat_docs_and_input respects the n_positions available in the model.

Copy link
Contributor

@mojtaba-komeili mojtaba-komeili left a comment

Choose a reason for hiding this comment

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

LGTM

@mojtaba-komeili
Copy link
Contributor

mojtaba-komeili commented Mar 2, 2022

nit: a general comment on self.n_extra_positions: In RagModel constructor we have assert opt['n_extra_positions'] >= 0. Yet there are line in code that we check for self.n_extra_positions <= 0. Can we remove the < check for clarity?

Copy link
Contributor

@jxmsML jxmsML left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for fixing this subtle bug

@klshuster
Copy link
Contributor Author

The failing test is fixed in #4395

@klshuster klshuster merged commit 2e3f5c7 into main Mar 3, 2022
@klshuster klshuster deleted the fix_n_extra_positions_bug branch March 3, 2022 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants