-
Notifications
You must be signed in to change notification settings - Fork 2
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
Structured Config Style #8
base: datasets
Are you sure you want to change the base?
Conversation
@@ -1,6 +1,7 @@ | |||
# @package dataset | |||
defaults: | |||
- dataset_s3dis |
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.
for ConfigStore
we have to add a line to all the defaults lines so that it can associate the config file with the dataclass.
|
||
# We seperate the dataset "cfg" from the actual dataset object | ||
# so that we can pass the "cfg" into the dataset constructors as a DictConfig | ||
# instead of as unwrapped parameters |
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.
Note this
|
||
@dataclass | ||
class Config: | ||
dataset: Any |
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 couldn't get the typing to work on this for some reason. You should be able to do Type[BaseDataset]
but it gave me an error...
|
||
cs = ConfigStore.instance() | ||
cs.store(name="base_config", node=Config) | ||
cs.store(group="dataset", name="dataset_s3dis", node=S3DISDataset) |
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.
where we define the dataset_s3dis
we referenced in the config
), | ||
} | ||
show(cfg) | ||
cfg.num_workers = "aj" |
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.
This will throw an error, because the cfg
is still ducttyped to S3DISDataConfig
and hydra is enforcing the runtime checks on it, which will be very helpful.
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.
Thanks for the proposition.
overall it looks good to me (except the the problem with dataset). I guess that we'll have the same problem with the model conf.
@@ -0,0 +1,74 @@ | |||
import hydra |
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 you move this file to the test directory ? and call this file test_config_store
or something 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.
Sure, if you wanted to keep it. I just wrote it to demonstrate the usecase. I think I'll just delete it and we can write a proper test file later.
@humanpose1 This is what I came up for a basic structured config solution.
All of the dataclasses will be moved to their appropriate locations, but I wanted to see your thoughts on how it worked before I applied it to the whole repo.