-
Notifications
You must be signed in to change notification settings - Fork 0
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
Centralize config defaults, add safety for Config updates #68
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import logging | ||
from pathlib import Path | ||
from typing import Union | ||
|
||
|
@@ -6,6 +7,160 @@ | |
DEFAULT_CONFIG_FILEPATH = Path(__file__).parent.resolve() / "fibad_default_config.toml" | ||
DEFAULT_USER_CONFIG_FILEPATH = Path.cwd() / "fibad_config.toml" | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class ConfigDict(dict): | ||
"""The purpose of this class is to ensure key errors on config dictionaries return something helpful. | ||
and to discourage mutation actions on config dictionaries that should not happen at runtime. | ||
""" | ||
|
||
# TODO: Should there be some sort of "bake" method which occurs after config processing, and | ||
# percolates down to nested ConfigDicts and prevents __setitem__ and other mutations of dictionary | ||
# values? i.e. a method to make a config dictionary fully immutable (or very difficult/annoying to | ||
# mutuate) before we pass control to possibly external module code that is relying on the dictionary | ||
# to be static througout the run. | ||
|
||
__slots__ = () # we don't need __dict__ on this object at all. | ||
|
||
def __init__(self, map: dict, **kwargs): | ||
super().__init__(map, **kwargs) | ||
|
||
# Replace all dictionary keys with values recursively. | ||
for key in self: | ||
if isinstance(self[key], dict) and not isinstance(self[key], ConfigDict): | ||
self[key] = ConfigDict(map=self[key]) | ||
|
||
def __missing__(self, key): | ||
msg = f"Accessed configuration key/section {key} which has not been defined. " | ||
msg += "All configuration keys and sections must be defined in {DEFAULT_CONFIG_FILEPATH}" | ||
logger.fatal(msg) | ||
raise RuntimeError(msg) | ||
|
||
def get(self, key, default=None): | ||
"""Nonfunctional stub of dict.get() which errors always""" | ||
msg = f"ConfigDict.get({key},{default}) called. " | ||
msg += "Please index config dictionaries with [] or __getitem__() only. " | ||
msg += "Configuration keys and sections must be defined in {DEFAULT_CONFIG_FILEPATH}" | ||
logger.fatal(msg) | ||
raise RuntimeError(msg) | ||
|
||
def __delitem__(self, key): | ||
raise RuntimeError("Removing keys or sections from a ConfigDict using del is not supported") | ||
|
||
def pop(self, key, default): | ||
"""Nonfunctional stub of dict.pop() which errors always""" | ||
raise RuntimeError("Removing keys or sections from a ConfigDict using pop() is not supported") | ||
|
||
def popitem(self): | ||
"""Nonfunctional stub of dict.popitem() which errors always""" | ||
raise RuntimeError("Removing keys or sections from a ConfigDict using popitem() is not supported") | ||
|
||
def clear(self): | ||
"""Nonfunctional stub of dict.clear() which errors always""" | ||
raise RuntimeError("Removing keys or sections from a ConfigDict using clear() is not supported") | ||
|
||
|
||
def validate_runtime_config(runtime_config: ConfigDict): | ||
"""Validates that defaults exist for every config value before we begin to use a config. | ||
|
||
This should be called at the moment the runtime config is fully baked for science calculations. Meaning | ||
that all sources of config info have been combined in `runtime_config` and there are no further | ||
config altering operations that will be performed. | ||
|
||
Parameters | ||
---------- | ||
runtime_config : ConfigDict | ||
The current runtime config dictionary. | ||
|
||
Raises | ||
------ | ||
RuntimeError | ||
Raised if any config that exists in the runtime config does not have a default defined | ||
""" | ||
default_config = _read_runtime_config(DEFAULT_CONFIG_FILEPATH) | ||
_validate_runtime_config(runtime_config, default_config) | ||
|
||
|
||
def _validate_runtime_config(runtime_config: ConfigDict, default_config: ConfigDict): | ||
"""Recursive helper for validate_runtime_config. | ||
|
||
The two arguments passed in must represent the same nesting level of the runtime config and all | ||
default config parameters respectively. | ||
|
||
Parameters | ||
---------- | ||
runtime_config : ConfigDict | ||
Nested config dictionary representing the runtime config. | ||
default_config : ConfigDict | ||
Nested config dictionary representing the defaults | ||
|
||
Raises | ||
------ | ||
RuntimeError | ||
Raised if any config that exists in the runtime config does not have a default defined in | ||
default_config | ||
""" | ||
for key in runtime_config: | ||
if key not in default_config: | ||
msg = f"Runtime config contains key or section {key} which has no default defined." | ||
msg += f"All configuration keys and sections must be defined in {DEFAULT_CONFIG_FILEPATH}" | ||
raise RuntimeError(msg) | ||
|
||
if isinstance(runtime_config[key], dict): | ||
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. Do you think it would be worth validating that if runtime_config[key] is a dict, that default_config[key] is as well? 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. Yeah this is reasonable paranoia. |
||
_validate_runtime_config(runtime_config[key], default_config[key]) | ||
|
||
|
||
def _read_runtime_config(config_filepath: Union[Path, str] = DEFAULT_CONFIG_FILEPATH) -> ConfigDict: | ||
"""Read a single toml file and return a config dictionary | ||
|
||
Parameters | ||
---------- | ||
config_filepath : Union[Path, str], optional | ||
What file is to be read, by default DEFAULT_CONFIG_FILEPATH | ||
|
||
Returns | ||
------- | ||
ConfigDict | ||
The contents of that toml file as nested ConfigDicts | ||
""" | ||
with open(config_filepath, "r") as f: | ||
parsed_dict = toml.load(f) | ||
return ConfigDict(parsed_dict) | ||
|
||
|
||
def resolve_runtime_config(runtime_config_filepath: Union[Path, str, None] = None) -> Path: | ||
"""Resolve a user-supplied runtime config to where we will actually pull config from. | ||
|
||
1) If a runtime config file is specified, we will use that file | ||
2) If not file is specified and there is a file named "fibad_config.toml" in the cwd we will use that file | ||
3) If no file is specified and there is no file named "fibad_config.toml" in the current working directory | ||
we will exclusively work off the configuration defaults in the packaged "fibad_default_config.toml" | ||
file. | ||
|
||
Parameters | ||
---------- | ||
runtime_config_filepath : Union[Path, str, None], optional | ||
Location of the supplied config file, by default None | ||
|
||
Returns | ||
------- | ||
Path | ||
Path to the configuration file ultimately used for config resolution. When we fall back to the | ||
package supplied default config file, the Path to that file is returned. | ||
""" | ||
if isinstance(runtime_config_filepath, str): | ||
runtime_config_filepath = Path(runtime_config_filepath) | ||
|
||
# If a named config exists in cwd, and no config specified on cmdline, use cwd. | ||
if runtime_config_filepath is None and DEFAULT_USER_CONFIG_FILEPATH.exists(): | ||
runtime_config_filepath = DEFAULT_USER_CONFIG_FILEPATH | ||
|
||
if runtime_config_filepath is None: | ||
runtime_config_filepath = DEFAULT_CONFIG_FILEPATH | ||
|
||
return runtime_config_filepath | ||
|
||
|
||
def get_runtime_config( | ||
runtime_config_filepath: Union[Path, str, None] = None, | ||
|
@@ -33,24 +188,14 @@ | |
The parsed runtime configuration. | ||
""" | ||
|
||
if isinstance(runtime_config_filepath, str): | ||
runtime_config_filepath = Path(runtime_config_filepath) | ||
|
||
with open(default_config_filepath, "r") as f: | ||
default_runtime_config = toml.load(f) | ||
|
||
# If a named config exists in cwd, and no config specified on cmdline, use cwd. | ||
if runtime_config_filepath is None and DEFAULT_USER_CONFIG_FILEPATH.exists(): | ||
runtime_config_filepath = DEFAULT_USER_CONFIG_FILEPATH | ||
runtime_config_filepath = resolve_runtime_config(runtime_config_filepath) | ||
default_runtime_config = _read_runtime_config(default_config_filepath) | ||
|
||
if runtime_config_filepath is not None: | ||
if runtime_config_filepath is not DEFAULT_CONFIG_FILEPATH: | ||
if not runtime_config_filepath.exists(): | ||
raise FileNotFoundError(f"Runtime configuration file not found: {runtime_config_filepath}") | ||
|
||
with open(runtime_config_filepath, "r") as f: | ||
users_runtime_config = toml.load(f) | ||
|
||
final_runtime_config = merge_configs(default_runtime_config, users_runtime_config) | ||
users_runtime_config = _read_runtime_config(runtime_config_filepath) | ||
final_runtime_config = merge_configs(default_runtime_config, users_runtime_config) | ||
else: | ||
final_runtime_config = default_runtime_config | ||
|
||
|
@@ -80,7 +225,7 @@ | |
final_config = default_config.copy() | ||
for k, v in user_config.items(): | ||
if k in final_config and isinstance(final_config[k], dict) and isinstance(v, dict): | ||
final_config[k] = merge_configs(default_config.get(k, {}), v) | ||
final_config[k] = merge_configs(default_config[k], v) | ||
else: | ||
final_config[k] = v | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 comment seems like a really good idea. It will be interesting to see, when we have an actual user(s) interacting with Fibad if there is a desire to modify the merged config prior to running train or predict. But I agree, that during runtime, the config should be/becomes immutable.
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.
Agree, I do feel like its complicated enough that we should hold off for now.