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

Refactor Blueprint Mixins, create ScreenTaskRequired mixin #566

Merged
merged 8 commits into from
Sep 24, 2021

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Sep 18, 2021

Overview

Creates the first full-fledged Blueprint mixin ScreenTaskRequired, in a similar vein to OnboardingRequired, but with clearer delineation for workflow-specific functionality. This PR also refactors both of these to follow the new BlueprintMixin structure, which lets us standardize how workflows for blueprints can be mixed in. The goal here is to create a task that, for workers that haven't completed any of the task yet, you can put out a test task for which you can more easily validate the results to than a full task. This launches new tasks on the fly, such that you can have validation tasks be separate from the rest of the pool.

Implementation

  • Creates the ValidationRequired mixin, which defines behaviors to properly validate and initialize blueprints with this functionality. Defines some logic hooks that Mephisto's ops layer can use to interact with this workflow. Also defines utility methods that are useful in user scripts for those utilizing this workflow.
  • Updates TaskRun to ignore special units (for gold, validators, etc) when determining eligible units for workers
  • Passes the TaskLauncher along to the Supervisor to be able to handle launching validator units. This smells but I aim to address this kind of over-coupling in the coming refactor of the Supervisor.
  • Updates the Supervisor to run validation tasks for new workers on blueprints that have ValidationRequired. Ensures that these are expired when abandoned, to not pass to others.
  • Creates a function in the TaskLauncher that creates and launches the special validator units.

Considerations

  • This implementation is just getting things working, but really displays the need for a general supervisor refactor. Moving some of the business logic into blueprints and feature mixins will likely help encapsulate things.
  • This introduces a specific way to determine if launched tasks are used for special purposes, keyed on the unit_index. I think this is a decent way to go about this, but I wonder if mixins should also have considerations for review scripts? Likely need to update the DataBrowser to handle these better in the future.
  • Right now I've dumped the mixins with the Blueprint file, but I think these should go somewhere better. Will break out before merging

Testing

In internal

  • Fix automated testing broken by passing task_launcher to supervisor
  • write automatic testing for this new blueprint mixin

@JackUrb JackUrb requested a review from pringshia September 18, 2021 00:02
@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 Sep 18, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #566 (e4515f5) into main (ef8a831) will increase coverage by 0.87%.
The diff coverage is 80.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
+ Coverage   65.45%   66.32%   +0.87%     
==========================================
  Files          79       81       +2     
  Lines        7297     7459     +162     
==========================================
+ Hits         4776     4947     +171     
+ Misses       2521     2512       -9     
Impacted Files Coverage Δ
...ueprints/static_html_task/static_html_blueprint.py 43.54% <ø> (-0.21%) ⬇️
...ractions/blueprints/mixins/screen_task_required.py 71.59% <71.59%> (ø)
...tractions/blueprints/mixins/onboarding_required.py 74.00% <74.00%> (ø)
mephisto/abstractions/blueprint.py 73.75% <76.92%> (+0.80%) ⬆️
...lueprints/abstract/static_task/static_blueprint.py 41.83% <100.00%> (+0.83%) ⬆️
...sto/abstractions/blueprints/mock/mock_blueprint.py 87.75% <100.00%> (+2.04%) ⬆️
...ns/blueprints/parlai_chat/parlai_chat_blueprint.py 36.98% <100.00%> (+0.43%) ⬆️
mephisto/abstractions/test/architect_tester.py 95.55% <100.00%> (ø)
mephisto/data_model/task_run.py 83.41% <100.00%> (ø)
mephisto/operations/operator.py 57.44% <100.00%> (+0.36%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef8a831...e4515f5. Read the comment docs.

@pringshia
Copy link
Contributor

Overall looks good! Will re-review pending some of the changes we discussed, e.g. adding max_validations @ the blueprint level, as well as perhaps asserting that this value is necessary to re-emphasize to the user that there is a cost for these tasks

@JackUrb JackUrb changed the title Creating the validator workflow mixin Refactor Blueprint Mixins, create ScreenTaskRequired mixin Sep 22, 2021
Copy link
Contributor

@pringshia pringshia left a comment

Choose a reason for hiding this comment

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

I have a few copy and terminology related notes, but otherwise this looks good to me.

### `OnboardingRequired`
This mixin allows for blueprints that require people to complete an onboarding task _before_ they're even able to start on their first task. Usually this is useful for providing task context, and then quizzing workers to see if they understand what's provided. Tasks using this mixin will activate onboarding mode for new `Worker`s whenever the `mephisto.blueprint.onboarding_qualification` hydra argument is provided.
### `ScreenTaskRequired`
This mixin allows for blueprints that require people to complete a _test_ version of the real task the first time a worker does the task. This allows you to validate workers on a run of the real task, either on your actual data (when providing `SharedTaskState.generate_screening_unit_data=False`) or on test data that you may more easily validate using (when providing a generator to `SharedTaskState.generate_screening_unit_data`). The tasks should be the same as your standard task, just able to be easily validated. You **do pay** for screening tasks, and as such we ask you set `mephisto.blueprint.max_screening_units` to put a cap on how many screening units you want to launch.
Copy link
Contributor

@pringshia pringshia Sep 23, 2021

Choose a reason for hiding this comment

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

Really like this API! ❤️

screening_data_factory could be another possible name here... just throwing out ideas

@JackUrb JackUrb merged commit dfdfc75 into main Sep 24, 2021
@JackUrb JackUrb deleted the validator-blueprint-mixin branch September 24, 2021 17:25
@JackUrb JackUrb added this to the 🚀 Mephisto 1.0 milestone Jan 5, 2022
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.

4 participants