Config with pydantic #334
Config with pydantic #334
Conversation
Was getting
```
RuntimeError: The Poetry configuration is invalid:
- [extras.pipfile_deprecated_finder.2] 'pip-shims<=0.3.4' does not match '^[a-zA-Z-_.0-9]+$'
```
Also * made . the default value for directories * renamed Config class to Configuration, as mypy was confused by nested Config.Config class * added filename to validation error * create dir before assigning to CFG field * docstrings for Configuration fields * dropped test in tests/parameter_sets/__init__.py, as it rewrote my config file
|
Kudos, SonarCloud Quality Gate passed! |
| """Load user configuration from the given file. | ||
|
|
||
| The config is cleared and updated in-place. | ||
| """ |
There was a problem hiding this comment.
Is this complex mechanism still necessary? Now that we have all default values, a simple Configuration.from_file would suffice, wouldn't it? No need to clear the config and overwrite stuff. Or do I miss something?
There was a problem hiding this comment.
The ewatercycle.CFG is a single instance and can not be re-assigned aka CFG = Configuration() as on import time the pointer to CFG was set, any reassignments would not be visible to the rest of the code. We need all this overwriting stuff to make sure we change the single instance.
We could switch to a singleton patternwhere we can reassign it, but this changes the public API.
| """Set of model versions that are | ||
| supported by this parameter set. If not set then parameter set will be | ||
| supported by all versions of model""" |
There was a problem hiding this comment.
| """Set of model versions that are | |
| supported by this parameter set. If not set then parameter set will be | |
| supported by all versions of model""" | |
| """Set of model versions that are compatible with this parameterset.""" |
I'd love to also rename the setting to compatible_model_versions, but that might break stuff.
There was a problem hiding this comment.
I am not sure about this suggestion.
With your suggestion how would a model developer know what the behavior is of an empty set. An empty set could also mean zero versions are supported/compatible.
There was a problem hiding this comment.
We could make it optional so
- None means compatible with all versions
- set() means no versions are compatible
There was a problem hiding this comment.
My concern was more about the word "supported" than the behavior for not setting it. Sorry if that was unclear. With respect to making it optional, perhaps we should reconsider this behaviour altogether. Imagine I make a new (backward compatible) release of the model. Then I'd like for my existing parameter sets to work out of the box.
| def __str__(self): | ||
| """Nice formatting of parameter set.""" | ||
| return "\n".join( | ||
| [ | ||
| "Parameter set", | ||
| "-------------", | ||
| ] | ||
| + [f"{k}={v!s}" for k, v in self.__dict__.items()] | ||
| ) | ||
|
|
There was a problem hiding this comment.
Can we please remove this now in favour of the default pydantic printing? Especially their integration with Rich makes things look much better(https://docs.pydantic.dev/usage/rich/):
In [10]: from rich import print
In [11]: print(CFG)
Configuration(
grdc_location=PosixPath('.'),
container_engine='docker',
apptainer_dir=PosixPath('.'),
singularity_dir=None,
output_dir=PosixPath('.'),
parameterset_dir=PosixPath('.'),
parameter_sets={},
ewatercycle_config=None
)There was a problem hiding this comment.
For an end user I like that they now see /bla instead of PosixPath('/bla').
If we do that then the Forcing and ParameterSet and models have different str outputs. I think they should look similar. We should do them all on none of them?
There was a problem hiding this comment.
Rather do all then. But I do agree that PosixPaths are not so nice.
| from ._config_object import CFG, SYSTEM_CONFIG, USER_HOME_CONFIG, Configuration | ||
|
|
||
| __all__ = ["CFG", "Config", "DEFAULT_CONFIG", "SYSTEM_CONFIG", "USER_HOME_CONFIG"] | ||
| __all__ = ["CFG", "Configuration", "SYSTEM_CONFIG", "USER_HOME_CONFIG"] |
There was a problem hiding this comment.
Would it make sense to move documentation to the user guide, and reduce the config to a single-file module called config.py?
There was a problem hiding this comment.
I moved the content of src/ewatercycle/config/_config_object.py to src/ewatercycle/config/init.py in c86f3b7.
I also added a link from docs/system_setup.rst to api docs.
To not increase the public API I prefixed private things with underscore.
Moving it to src/ewatercycle/config.py could be done once src/ewatercycle/config/_lisflood_versions.py has been moved to its own plugin/repo.
|
Just out of curiosity. Have you also considered using pydantic for the forcing classes? |
I tried at #345 but got stuck on save/load. |
|
Merged as part of #340 |
Fixes #332
Also
TODO:
make CFG.save_to_file() less uglydo later, has TODO in code