-
Notifications
You must be signed in to change notification settings - Fork 910
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 all commits
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 |
---|---|---|
|
@@ -168,7 +168,7 @@ def __getitem__(self, key) -> dict[str, Any]: | |
else: | ||
base_path = str(Path(self._fs.ls("", detail=False)[-1]) / self.base_env) | ||
base_config = self.load_and_merge_dir_config( | ||
base_path, patterns, read_environment_variables | ||
base_path, patterns, key, read_environment_variables | ||
) | ||
config = base_config | ||
|
||
|
@@ -179,7 +179,7 @@ def __getitem__(self, key) -> dict[str, Any]: | |
else: | ||
env_path = str(Path(self._fs.ls("", detail=False)[-1]) / run_env) | ||
env_config = self.load_and_merge_dir_config( | ||
env_path, patterns, read_environment_variables | ||
env_path, patterns, key, read_environment_variables | ||
) | ||
|
||
# Destructively merge the two env dirs. The chosen env will override base. | ||
|
@@ -211,6 +211,7 @@ def load_and_merge_dir_config( | |
self, | ||
conf_path: str, | ||
patterns: Iterable[str], | ||
key: str, | ||
read_environment_variables: bool | None = False, | ||
) -> dict[str, Any]: | ||
"""Recursively load and merge all configuration files in a directory using OmegaConf, | ||
|
@@ -219,6 +220,7 @@ def load_and_merge_dir_config( | |
Args: | ||
conf_path: Path to configuration directory. | ||
patterns: List of glob patterns to match the filenames against. | ||
key: Key of the configuration type to fetch. | ||
read_environment_variables: Whether to resolve environment variables. | ||
|
||
Raises: | ||
|
@@ -275,9 +277,13 @@ def load_and_merge_dir_config( | |
|
||
if not aggregate_config: | ||
return {} | ||
if len(aggregate_config) == 1: | ||
return list(aggregate_config)[0] | ||
return dict(OmegaConf.merge(*aggregate_config)) | ||
|
||
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 think the flow here is a little unclear (too many different This would be solved if we removed lines 276-279. I know @merelcht said there was some reason to pick out the 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 would be keen on removing it, unless we have a specific failing case. I try remove line 278-279 and all test case passed. Should we separate this into another issue? Removing Line 276-277 will cause a few tests fail, I suspect it's also link to #2556 and some refactoring may be needed. 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'm happy for those lines to be removed if everything is still working fine. I don't remember why I added it, so really should have left a comment 😅 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 think we should remove line 278-279 here since as I understand it at the moment they are not just unnecessary but following this change will actually be causing a bug (would be good if you could check this): if you just have a single parameters.yml file, the Lines 276-277 don't actually cause a bug I think, so if removing them makes some tests fail and it seems related to #2556 then let's leave those here for now and try to remove them in that PR. |
||
if key == "parameters": | ||
# Merge with runtime parameters only for "parameters" | ||
return OmegaConf.to_container( | ||
OmegaConf.merge(*aggregate_config, self.runtime_params), resolve=True | ||
) | ||
return OmegaConf.to_container(OmegaConf.merge(*aggregate_config), resolve=True) | ||
|
||
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.
Personally I'd put all this as bug fix rather than major improvement.
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.
It's unclear to me if this is part of the official support feature. My reasoning go as below:
We never fixed the
TemplatedConfigLoader
as we promised we need some more thinking for 0.19. If this is considered a bug fix, I don't see why we can't fix this forTemplatedConfigLoader
in 0.18.x and requires users to sub-class it instead.In fact,
TemplatedConfigLoader
doesn't suffer from the residual problem so it can be safely used for all configurations. There is nothing wrong with merging theruntime_params
as this is exactly what we are doing now.#1782 (comment)
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 is a fair comment I think for the "interpolated parameters" point, but I still think the
dict
vs.DictConfig
point should be under bug fix.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.
@antonymilne I agree with this and I will move it to bug fix.