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

Ease-of-use improvements to wrapper teacher #3247

Merged
merged 4 commits into from
Nov 5, 2020

Conversation

EricMichaelSmith
Copy link
Contributor

@EricMichaelSmith EricMichaelSmith commented Nov 3, 2020

Patch description
In most cases, AbstractWrapperTeacher is used to edit the fields of the action returned by the teacher that it wraps around. Assuming that this inner teacher is a FixedDialogTeacher, however, editing the fields of this action is tricky because it must be done before the FixedDialogTeacher performs further processing on the action and registers it for metrics and evaluations, as part of FixedDialogTeacher.process_action(). This, this PR gives AbstractWrapperTeacher a concrete implementation of .act() and an abstract method ._edit_action(), in order to give subclasses of AbstractWrapperTeacher an easy way to edit the action at the appropriate point in the pipeline without worrying about side effects.

This change is back-compatible with all existing subclasses of AbstractWrapperTeacher, which are not required to use the new .act() method or to provide an implementation of ._edit_action(). However, two particular subclasses of AbstractWrapperTeacher have been upgraded to use ._edit_action() to demonstrate how this new method is used.

Testing steps
This change has test coverage through tests/nightly/gpu/test_style_gen.py and tests/test_wrapper.py.

test_new_tasks.py
As expected, an error is produced when running python tests/datatests/test_new_tasks.py on AbstractWrapperTeacher, for the reason given in #2642

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

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

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.

lgtm!

@EricMichaelSmith EricMichaelSmith merged commit bec4df6 into master Nov 5, 2020
@EricMichaelSmith EricMichaelSmith deleted the wrapper-teacher-improvements branch November 5, 2020 13:34
@dianaglzrico
Copy link
Contributor

@EricMichaelSmith sorry, just saw this but this looks really nice! it will help to understand better for any future users, thanks for doing this!

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