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

skipping the padding message #3634

Merged
merged 4 commits into from
Apr 29, 2021
Merged

skipping the padding message #3634

merged 4 commits into from
Apr 29, 2021

Conversation

mojtaba-komeili
Copy link
Contributor

@mojtaba-komeili mojtaba-komeili commented Apr 29, 2021

Patch description
The rag agent may crash if there is batch padding message. These message do not contain the text data that it want to encode.

Test
It used to crash if it was receiving a batch padding message. It successfully finishes with this.

@stephenroller
Copy link
Contributor

Use obs.is_padding_example()

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

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

Is observation not a message already?! This is doing a shallow copy which is fine, but we should have gotten those dicts out of there by now :(

@mojtaba-komeili
Copy link
Contributor Author

Is observation not a message already?! This is doing a shallow copy which is fine, but we should have gotten those dicts out of there by now :(

It might be in the code that uses it. But since the params were allowing dict type as well I thought of doing that for safety.

@mojtaba-komeili mojtaba-komeili merged commit b5c2eb0 into master Apr 29, 2021
@mojtaba-komeili mojtaba-komeili deleted the rag-batch-padding branch April 29, 2021 19:41
@klshuster
Copy link
Contributor

observation can be a dict but TorchAgent.observe takes care of that so this copy is probably not necessary but alas (self._generation_agent.observe() is guaranteed to return a Message)

@stephenroller
Copy link
Contributor

I tried to make the teachers also forcefully output Messages by now

@klshuster
Copy link
Contributor

klshuster commented Apr 30, 2021

interestingly the WoW teachers do not, which is probably where this error surfaced

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