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

Bug fix: Multi-task with datatype train:stream #2788

Merged
merged 10 commits into from
Jun 24, 2020
Merged

Conversation

emilydinan
Copy link
Contributor

@emilydinan emilydinan commented Jun 24, 2020

Patch description
@klshuster found a bug where if we multi-task with -dt train:stream, the shorter teacher will only complete 1 epoch. This is fixed by: (1) changing the condition we check for FixedDialogTeacher setting epoch_done and (2) changing self.random for multiworld. In the multiworld setting, we want to randomly choose between subworlds when training, even if the examples in the subworld may be ordered (as in train:stream).

I wrote a test that fails before these changes and now passes.

@github-actions
Copy link

Your PR contains a change to a task. Please paste the results of the following command into a comment:

python tests/datatests/test_new_tasks.py

@emilydinan emilydinan requested a review from stephenroller June 24, 2020 13:53
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.

this is_training is much better

@stephenroller
Copy link
Contributor

Why aren't tests running tho

@emilydinan
Copy link
Contributor Author

this is_training is much better

@klshuster 's idea

Copy link
Contributor

@klshuster klshuster left a comment

Choose a reason for hiding this comment

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

awesome, thank you!!

"""


def is_training(datatype: str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@stephenroller
Copy link
Contributor

tbh I'm surprised that chunky teacher doesn't hang.

@klshuster
Copy link
Contributor

also since tests aren't running here, maybe you can run tests locally? just to check

@stephenroller
Copy link
Contributor

Filed a bug with CircleCI. None of Emily's PRs will run tests until they fix something.

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.

god bless tests.

parlai datatype

:return should_shuffle:
given datatype, return whether we should cycle
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shuffle

@emilydinan emilydinan merged commit 05088f5 into master Jun 24, 2020
@emilydinan emilydinan deleted the infinteloop_fix branch June 24, 2020 21:14
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