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

Allow more subclassing of self-chat world #3955

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

EricMichaelSmith
Copy link
Contributor

Allow for more subclassing of SelfChatWorld to modify/extend its logic:

  • Abstract out the logic applied at the end of every episode, in order to allow subclasses to supply custom logic (in my case, resetting a random turn-index variable per episode)
  • Allow for specifying custom logic for when to use seed utterances (in my case, only when the current turn index equals the value of the random turn-index variable specified above)

Comment on lines 193 to 194
for a in agents:
a.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should wrap this into self.reset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a World.reset_agents() method - is this what you were thinking? We can't call World.reset() directly here, because there's additional logic there - i.e. World.reset() also resets self.total_parleys, self.total_exs, self.time, etc.

@@ -160,7 +154,7 @@ def parley(self):
self.agents[i].observe(validate(context))
# clear contexts so they are only added once per episode
self.contexts = None
elif self.seed_utterances:
elif self.seed_utterances and self._use_seed_utterances():
Copy link
Contributor

@spencerp spencerp Aug 18, 2021

Choose a reason for hiding this comment

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

I have a slight preference toward integrating this logic closer with the above:

self.seed_utterances = self._get_seed_utt_acts(
    self.episode_cnt, self.agents
)

Maybe by putting the self._use_seed_utterances check inside of self._get_seed_utt_acts so there's a single source of truth (if self.seed_utterances)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point - changed

@EricMichaelSmith
Copy link
Contributor Author

Thanks @stephenroller @spencerp for useful comments! I've tried to address your suggestions as best as possible, and since you've both approved the PR, I'm merging it in now - but if you feel like I didn't address your suggestions appropriately, lemme know and I'd be happy to make changes in another PR

@EricMichaelSmith EricMichaelSmith merged commit 376888e into main Sep 8, 2021
@EricMichaelSmith EricMichaelSmith deleted the generalize-self-chat-seed-utterances branch September 8, 2021 18:05
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