Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blueprint Mixin cleanup and test #582

Merged
merged 1 commit into from
Oct 11, 2021
Merged

Blueprint Mixin cleanup and test #582

merged 1 commit into from
Oct 11, 2021

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Oct 6, 2021

Overview

Finishes formalizing the content for BlueprintMixins, as well as the methodology for using them. With this change, we address two types of mixin users:

  1. People who want to simply include a mixin in a simple blueprint, but don't need to be creating their own BlueprintArgs, SharedTaskState, etc.
  2. People who are building a complicated blueprint, and likely making their own of the previous listed classes.

It then includes machinery to discover Mixins for a blueprint, and automatically call their initializers and asserts. It also includes fixes for the ParlAI blueprint, and testing for the mixin architecture.

Implementation

Rather than using metaclass magic, in order to address both of the above classes, simple case users can use the @BlueprintMixin. mixin_args_and_state decorator on their Blueprint class, for whatever BlueprintMixins they want. Our tasks (ParlAIChatBlueprint) use the extended form, in which we ask each configuration class to also mixin the mixin's defined configuration classes.

This method wraps the created Blueprint with a MixedInBlueprint which automatically incorporates and mixes in the config mixins that are required. Generally it's more opaque than doing it yourself, but it's certainly simpler for people who don't need to get into the nitty-gritty of defining the arguments.

Discussion

The extract_unique_mixins method is likely the most "magic" method in this, going through the class structure of a given Blueprint and discovering all BlueprintMixins that can be considered "unique" (not directly subclassed by another mixin that is also included). I think this covers most cases to avoid redundancy, but in any case the init_mixin_config method of any BlueprintMixin should be idempotent to deal with cases where there are strange inheritance structures.

@kushalchawla this should resolve your issue, and you can return to using main once it is merged. Sorry for the inconvenience!

Testing

Added new testing, existing tests pass after the changes that move mixin config inits into the Blueprint base class.

@JackUrb JackUrb requested a review from pringshia October 6, 2021 18:07
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 6, 2021
@pringshia
Copy link
Contributor

Thanks for the tests, they make it easier to follow along what's happening here. I think the decorator is a nice, clean touch. On my end it looks good. Perhaps someone with more Python expertise can chime in as well, but on my end I'm ok to approve!

@JackUrb JackUrb merged commit db2ca5f into main Oct 11, 2021
@JackUrb JackUrb deleted the mixin-refactor-metaclass branch October 11, 2021 19:46
@JackUrb JackUrb mentioned this pull request Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants