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

Move defaults to config yaml files for crowdsourcing tasks #4095

Merged
merged 27 commits into from Oct 25, 2021

Conversation

meganung
Copy link
Contributor

@meganung meganung commented Oct 15, 2021

Moved configs from run scripts to yaml config files for the crowdsourcing tasks.

Checked that the configs set are respected by running the run scripts with the flag --cfg job and inspecting the defaults list by running the run scripts with the flag --info defaults

crowdsourcing tests pass except for:

  • test_chat_demo which will pass when the corresponding PR in Mephisto is merged done- pointed to new mephisto release tag
  • test_turn_annotations_static tests - failing because need to add onboarding testing (in a separate PR and then rebase this branch ontop of it). Config changes make the onboarding_qualification parameter to be non optional so cannot be null. this is done.

@meganung
Copy link
Contributor Author

Everything should be in order now, I think it's failing the one crowdsourcing TestChatDemo on CI because the CI config for ParlAI uses Mephisto v0.4.0. I merged the corresponding mephisto PR so locally when I'm on the latest Mephisto main commit and on this branch the crowdsourcing tests pass (including TestChatDemo).

What's the proper or preferred procedure so we can merge this PR and also have the test pass?

@meganung meganung changed the title [WIP] Move defaults to config yaml files for crowdsourcing tasks Move defaults to config yaml files for crowdsourcing tasks Oct 20, 2021
@JackUrb
Copy link
Contributor

JackUrb commented Oct 20, 2021

I just released a 0.4.1 Mephisto tag you can point to.

Copy link
Contributor

@EricMichaelSmith EricMichaelSmith left a comment

Choose a reason for hiding this comment

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

Oh yeah this is great, thanks for doing this :) Would you be able to add in similar changes in parlai/crowdsourcing/projects/wizard_of_internet/run.py and parlai/crowdsourcing/projects/multisession_chat/human_eval/run.py as well, so that the regression in Hydra 1.1 defaulting doesn't show up there either? Also, I have a few minor comments

parlai/crowdsourcing/tasks/acute_eval/run.py Show resolved Hide resolved
parlai/crowdsourcing/utils/tests.py Show resolved Hide resolved
projects/blenderbot2/agents/modules.py Show resolved Hide resolved
Copy link
Contributor

@EricMichaelSmith EricMichaelSmith left a comment

Choose a reason for hiding this comment

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

Great, thanks for the revisions :)

@meganung meganung merged commit 018a407 into main Oct 25, 2021
@meganung meganung deleted the move-defaults-acute-evals branch October 25, 2021 20:38
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.

None yet

5 participants