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

ConversationTeacher parent class changed #4256

Merged
merged 9 commits into from
Dec 16, 2021
Merged

ConversationTeacher parent class changed #4256

merged 9 commits into from
Dec 16, 2021

Conversation

mojtaba-komeili
Copy link
Contributor

Patch description
ChangedConversationTeacher's parent class to ParlAIDialogTeacher.

Testing steps
1- Standard teacher tests in ParlAI that rely on ConversationTeacher.
2- parlai dd some internal classes that use this teacher

@@ -1611,12 +1611,12 @@ def setup_data(self, datafile):
yield act, next_episode_new


class ConversationTeacher(FixedDialogTeacher):
class ConversationTeacher(ParlAIDialogTeacher):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might need to be DialogTeacher, not ParlAIDialogTeacher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What makes DialogTeacher preferable to ParlAIDialogTeacher here?

Copy link
Contributor

Choose a reason for hiding this comment

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

ParlAI DT should be assuming a particular format. It might be that we successfully override everything but really we shouldn't.

@mojtaba-komeili
Copy link
Contributor Author

@stephenroller I changed the base class to DialogTeacher. There were more changes and I ended up directly changing JsonTeacher as well. Teacher tests pass and I tried to run tasks that were using these teachers.
There is going to be a separate PR on adapting one of the heavily customized teachers to the new format. But the rest should be fine.

@@ -1719,27 +1704,24 @@ def _setup_data(self, path):
if self.label_turns in ['firstspeaker', 'both']:
eps = self._get_ep_from_turns(turns[::2], turns[1::2])
if eps:
self.episodes.append(eps)
self.num_exs += len(eps)
for example, example_begins in self._return_episode_examples(eps):
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good


# train on even turns as labels (turns w/ second speaker)
if self.label_turns in ['secondspeaker', 'both']:
eps = self._get_ep_from_turns(turns[1::2], turns[2::2])
if eps:
self.episodes.append(eps)
self.num_exs += len(eps)
for example, example_begins in self._return_episode_examples(eps):
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good!

@mojtaba-komeili mojtaba-komeili merged commit 2c1bcb0 into main Dec 16, 2021
@mojtaba-komeili mojtaba-komeili deleted the conv-teacher branch December 16, 2021 23:04
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