-
Notifications
You must be signed in to change notification settings - Fork 15
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
[WIP][example] Extending SC Schema for Experiments #71
base: main
Are you sure you want to change the base?
Conversation
I think there are at least 3 meaningful examples to work on: |
examples/extend_via_sc_schema/:
Outdated
@@ -0,0 +1,26 @@ | |||
# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved |
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.
something.py?
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 caught me vimmin'
|
||
cs = ConfigStore.instance() | ||
cs.store(name="base_config", node=Config) | ||
cs.store(group="optim", name="base_adam", node=AdamConf) |
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.
We don't have a way for the user to get the config registered? (something like hydra_configs.torch.register_configs()?`
Why are you registering it here?
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.
Are you implying we consolidate lines 10-18 into a single call:
hydra_configs.torch.register_configs()
?
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.
Not exactly.
Registering a schema for your own config is one thing (line 17), but registering AdamConf (and the rest of the configs hydra-torch is providing) is something we can provide a function (or functions as we have discussed) to do.
This app:
- defines its own Config class.
- registers it
- registers a config class defined by the library.
I am suggesting that registering config classes defined by the library will be done by the library and will be triggered with a function call.
This will enforce a consistent naming convention for the registered classes and will reduce the boilerplate required to use the configs package.
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.
Great, I agree. I'll make a quick PR for the registration. Then we can use that in this example.
@romesco Here's my approach for pure dataclasses. I'm not sure whether this would be considered best practice in Hydra - it sounds like typically from dataclasses import dataclass
from omegaconf import MISSING
@dataclass
class DatamoduleConf:
"""Schema definition to be extended by all concrete datamodules"""
_target_: str = MISSING
batch_size: int = MISSING
num_workers: int = -1
@dataclass
class MyDatamoduleConf(DatamoduleConf):
"""Projects should extend the datamodule schema above"""
_target_: str = 'project.data.MyDatamodule' # Required override
batch_size: int = 100 # Required override
crop_dir: str = MISSING # New requirement added by the extension
regions: bool = False # New default added by the extension |
This is very preliminary for the time being. I have a couple more ideas to simplify, but I want to make sure we think through this carefully (including naming conventions, folder structure, etc.).
@addisonklinke I would love to see your approach to doing this purely with dataclasses as well
First draft example can be run by:
Feel free to provide any and all feedback.