-
Notifications
You must be signed in to change notification settings - Fork 75
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
[BREAKING CHANGES] Argument Configuration with Hydra and Dataclasses #246
Conversation
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.
Can't wait until this gets shipped!
Most of the comments here are mostly be just being annoying with some nits.
'Operator.parse_and_launch_run_wrapper has been deprecated in favor ' | ||
'of using Hydra for argument configuration. See the docs TODO in order ' | ||
'to upgrade.' | ||
) |
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.
yay for developer friendly UX
Awesome! |
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.
Awesome.
access-key-id=$ACCESS_KEY\ | ||
secret-access-key=$SECRET_KEY |
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.
You can access environment variables from the config:
key: ${env:ACCESS_KEY}
See OmegaConf docs.
- `mephisto.task.task_title` - The title of the task showed to workers in their queue | ||
- `mephisto.task.task_description` - The short description for the task that may be showed inline while they're searching for tasks | ||
- `mephisto.task.task_tags` - Any searchable tags that would make it easy to find your task | ||
- `mephisto.task.task_reward` - The reward paid out in dollars for completing your task | ||
- `mephisto.task.task_name` - This task name will be used to consolidate your data across runs. Generally we suggest using `descriptive-string-pilot-#` when piloting and `descriptive-task-string` for your main collection. More details on this are in the (TODO) Mephisto argument instructions. |
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.
You can generate the config node with --cfg job --package mephisto
.
I often use those as documentation snippets, try it out to see the output.
Example here.
Unfortunately there is no support for help strings yet.
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.
I've managed pulling out help strings from the dataclass
metadata in #249. Might be worth considering pulling help from metadata in Hydra? (of course this relies on being able to get the dataclass itself rather than the config).
@@ -0,0 +1,13 @@ | |||
#@package _global_ |
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.
You can also use #@ package mephisto
here to drop one nesting level.
used to demonstrate the functionalities around using Mephisto | ||
for ParlAI tasks." | ||
task_reward: 0.3 | ||
task_tags: "dynamic,chat,testing" |
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.
You can use yaml lists:
task_tags: [dynamic,chat,testing]
or:
task_tags:
- dynamic
- chat
- testing
defaults = [ | ||
{"mephisto.blueprint": BLUEPRINT_TYPE}, | ||
{"mephisto.architect": 'local'}, | ||
{"mephisto.provider": 'mock'}, | ||
"conf/base", | ||
{"conf": "example"}, | ||
] | ||
|
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.
If you have a scriptconfig.yaml
file in your search path you can put the defaults in it in yaml format (which is cleaner). it will get used even if your TestScriptConfig does not have the defaults declared.
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.
The only difficulty here is that BLUEPRINT_TYPE
is only loaded up at runtime, as we import it from another class.
@dataclass | ||
class TestScriptConfig(RunScriptConfig): | ||
defaults: List[Any] = field(default_factory=lambda: defaults) | ||
task_dir: str = TASK_DIRECTORY |
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.
consider using OmegaConf resolvers for things like that.
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.
Considered, but what is the use case for using a resolve being more advantageous? Sure, in both cases I can use ${task_dir:}
but if I actually want to access cfg.task_dir
I'll still need task_dir: ${task_dir:}
somewhere right?
Overview
This change is moving Mephisto over from using argparser (and related helpers) to configure its core operations to using Hydra. Hydra will allow for configurable, committable files that allow sharing exact experimental runs with ease. It'll also allow easier access to deep Mephisto configuration options directly from the command line, or through those configuration scripts.
There's no clean way to introduce these changes into Mephisto other than one breaking branch, and as such this PR contains all of the components for migrating Mephisto over. The commits in order roughly follow the Implementation checklist. Most are direct refactors that choose to retain existing behavior where possible, but introduce Hydra to facilitate a complete and total removal of
ARG_STRING
,Argparse
, and all related helpers.Implementation
argparse
based scripts.ARG_STRING
, argparse, and related code (add_args_to_group
, much ofargparse_parser
, etc).Testing
Tests:
mephisto register <>
test