-
Notifications
You must be signed in to change notification settings - Fork 903
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
OmegaConfigLoader returns Dict
instead of DictConfig
, resolves runtime_params properly
#2467
Changes from 1 commit
4565684
67cff2d
055124e
842e83c
d6d224a
78c1ae0
5b48003
5b0bff8
c864783
2c9da40
f578c39
de98e15
4c0571d
f1c1ec2
acbde3e
87a51b6
a1b5f0c
29f62ff
f610010
5799792
78c2371
e24fbfe
0f7911f
e7d6f62
a4d9893
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,6 +129,8 @@ def __init__( | |
env=env, | ||
runtime_params=runtime_params, | ||
) | ||
if not self.runtime_params: | ||
self.runtime_params = {} | ||
|
||
def __getitem__(self, key) -> Dict[str, Any]: | ||
"""Get configuration files by key, load and merge them, and | ||
|
@@ -275,7 +277,9 @@ def load_and_merge_dir_config( | |
return {} | ||
if len(aggregate_config) == 1: | ||
return list(aggregate_config)[0] | ||
return dict(OmegaConf.merge(*aggregate_config)) | ||
return OmegaConf.to_container( | ||
OmegaConf.merge(*aggregate_config, self.runtime_params), resolve=True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this definitely work when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked with the source code, def merge(
*configs: Union[
DictConfig,
ListConfig,
Dict[DictKeyType, Any],
List[Any],
Tuple[Any, ...],
Any,
] |
||
) | ||
|
||
def _is_valid_config_path(self, path): | ||
"""Check if given path is a file path and file type is yaml or json.""" | ||
|
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.
Probably nicer to do
runtime_params=runtime_params or {}
above instead?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.
Or even better
AbstractConfigLoader
should actually be the class responsible for this defaulting behaviour, i.e. it should doself.runtime_params = runtime_params or {}
.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.
100% agree with this.