-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add onboarding to crowdsourcing tests #4096
Conversation
@@ -215,7 +273,10 @@ def _get_agent_state(self, task_data: Dict[str, Any]): | |||
""" | |||
|
|||
# Set up the mock human agent | |||
agent_id = self._register_mock_agents(num_agents=1)[0] | |||
if self.config.mephisto.blueprint.get("onboarding_qualification", None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a better way to check if we should test onboarding? Would it be better to pass an onboarding bool variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment this is how we specify onboarding in Mephisto:
https://github.com/facebookresearch/Mephisto/blob/main/mephisto/abstractions/blueprints/mixins/onboarding_required.py#L103-L107
So I think this is really the only way to check that a config is trying to use onboarding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful to have this feature - thanks for building it in! I have a comment about structure
@@ -141,14 +141,13 @@ def _test_turn_annotations_static_task( | |||
overrides += [ | |||
'+mephisto.blueprint.annotation_last_only=False', | |||
'+mephisto.blueprint.conversation_count=null', | |||
'mephisto.blueprint.onboarding_qualification=null', | |||
'mephisto.blueprint.onboarding_qualification=test_turn_annoataions', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the changes :)
* move defaults for fast acute * add _self_ and move defaults for acute eval * udpated config overrides in test * move defaults for model chat and update test * move defaults for model image chat and update test * add hydra_configs to config path and update test * move defaults and add self for qa data collection task * move defaults to yaml, add self, and update tests * onboarding qualification arg * Fixed Some Typos (#4100) * Add onboarding to crowdsourcing tests (#4096) * add support for registering and onboarding agent * add a value for onboarding qualification instead of null to test * add parameter assume_onboarding and fix spelling * move defaults for fast acute * add _self_ and move defaults for acute eval * udpated config overrides in test * move defaults for model chat and update test * move defaults for model image chat and update test * add hydra_configs to config path and update test * move defaults and add self for qa data collection task * move defaults to yaml, add self, and update tests * onboarding qualification arg * rebase * rebase more edits and cleanup * linting * placeholder to remove unused import error * move defaults for tasks in crowdsourcing projects * point to new mephisto release for circle ci Co-authored-by: Atharv jairath <54663702+atharvjairath@users.noreply.github.com>
Added support for onboarding in crowdsourcing tests. Used this example in the Mephisto repo as a guide.
CI crowdsourcing tests.
(will rebase this PR after)