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

skipping the padding #3704

Merged
merged 2 commits into from
Jun 16, 2021
Merged

skipping the padding #3704

merged 2 commits into from
Jun 16, 2021

Conversation

mojtaba-komeili
Copy link
Contributor

Patch description
After looking into parlai data_stats crashes that I was running into, I noticed that it was happening when the script was trying to tokenize the non-existing text from a padding message. Added this to skip those messages.
Note: this was happening with chunk teachers, even with batch-size of 1. So the main problem might be from the teacher itself.

Testing steps
My parlai data_stats runs after this.

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.

sgtm

@@ -108,6 +108,8 @@ def keep_token(t):
while not world.epoch_done() and world.total_exs < max_cnt:
world.parley()
act = world.get_acts()[opt.get('agent')]
if act.is_padding():
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this is_padding() field -- do we always have that?
why not just check if the text we get is an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are using a Message object, instead of dict, that method must be there (link).
But maybe skipping the ones that do not have text field in them covers a wider number of things that could go wrong as well. I change it to that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Image Teachers often give empty text with an image field. We recently changed messages to explicitly mark when they are padding. It's much cleaner.

@mojtaba-komeili
Copy link
Contributor Author

Regarding the above comments, I keep the skipping condition as before (is_padding) but also set the default value for text to be an empty string, instead of None, in case there are any other messages without text field.

@mojtaba-komeili mojtaba-komeili merged commit ba97523 into master Jun 16, 2021
@mojtaba-komeili mojtaba-komeili deleted the skip-padding branch June 16, 2021 20:16
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