feat: revamp config loading - #156
Conversation
for more information, see https://pre-commit.ci
| ] | ||
|
|
||
| requires-python = ">= 3.10" | ||
| requires-python = ">= 3.12" |
There was a problem hiding this comment.
Python 3.12 is needed for built-in toml loader and using the type alias keyword. There are other workarounds, but the Python upgrade was the easiest. The change is aligned with https://scientific-python.org/specs/spec-0000/, but not sure what Python version convention you guys want to follow.
|
This is a draft for now as I still want to double-check that the config is effectively the same and the model runs result in the same output. Early feedback, especially about the new |
|
Looks good to me, thanks Tobias! |
JakobBD
left a comment
There was a problem hiding this comment.
This is great stuff, so helpful!!!
Thanks, it's greatly appreciated.
I have commented the point on yaml (Leonie and Bennet, please comment!), and a few nit-picky points of personal preference, and a few suggestions for further improvements.
| CEMENT = "cement" | ||
|
|
||
|
|
||
| def get_model_classes(): |
There was a problem hiding this comment.
For lazy loading, we could have this function take the model name as an argument, and only import the needed model? Maybe in a match/case?
| @@ -1,7 +1,7 @@ | |||
| # %% | |||
| from remind_mfa.common.helpers import ModelNames | |||
There was a problem hiding this comment.
While we're at it, this file could be deleted, along with those for the other materials.
| selected_models = list(ModelNames) if selection == "all" else [selection] | ||
|
|
||
| for model in selected_models: | ||
| model_config = load_config(config_names, model) |
There was a problem hiding this comment.
There's currently no option to specify a config path other than cwd/config, and no option to give paths to individual config files.
I find that behaviour fine when prompting for config files, but at least I would find it more intuituve if
uv run run_remind_mfa.py --config config/default.yaml would not return an error.
Personally, I would change the behaviour such that --config always expects the full (relative) path, while prompting looks in the config folder (could even be relative to the repo instead of cwd).
I see the downside of having to give the full path several times for several config files, but the only use case for that right now is running on the cluster for input data generation.
| import logging | ||
| import yaml | ||
| import sys | ||
| from typing import Annotated, Literal |
There was a problem hiding this comment.
While we're at it, rename file to remind_mfa.py? wdyt?
| def init_model(cfg: dict) -> CommonModel: | ||
| """Choose MFA subclass and return an initialized instance.""" | ||
| @app.command() | ||
| def main( |
There was a problem hiding this comment.
To make that more convenient, you could add a (boolean) --remind-input option, which sets all the others (config files, materials, later also scenarios).
| configure_logger() | ||
| selected_models = list(ModelNames) if selection == "all" else [selection] |
There was a problem hiding this comment.
I would move these two lines to main(), and pass selected_models to run_remind_mfa, only calling it models there.
| @@ -0,0 +1,116 @@ | |||
| [base.input] | |||
There was a problem hiding this comment.
As discussed, I would personally prefer switching back to yaml - my apologies for the miscommunication and the resulting back and forth, that was my fault!
For our deep hierarchy and nesting (I count up to 6 levels), I find the yaml indentation much easier to read than the toml dot notation. For me that outweighs toml files being somewhat more pythonic.
I don't want to decide this alone. @leonieschweiger and @bennet21 - What do you think?
There was a problem hiding this comment.
I also find the yaml indentation more clear and intuitive. If the advantages of toml are not big (which I cannot judge myself), I would also be in favour of keeping yaml, but I think I could adjust to both :)
There was a problem hiding this comment.
@JakobBD We discussed this a bit. There is no strong preference from our side to either option. Yaml has the advantage of being familiar, toml that it optionally allows to extract groups of keys to a table. We discussed that eg sankey under visualization might be better as a table/indented subgroup, while the single-value options like extrapolation.do_visualize feel cleaner in the toml dot notation than with forced indention in yaml.
toml does allow indentation, and that would be our proposal (see last commit) but going back to yaml is definitely also a fine option for us.
| ] | ||
|
|
||
| requires-python = ">= 3.10" | ||
| requires-python = ">= 3.12" |
I agree! @leonieschweiger could you make a suggestion?
I like the idea! We could add the folder to .gitignore, and except |
| consumption.per_capita = true | ||
| trade.do_visualize = true | ||
| sankey.do_visualize = true | ||
| sankey.plotter_args.slice_dict.e = "C" |
There was a problem hiding this comment.
| sankey.plotter_args.slice_dict.e = "C" | |
| sankey.plotter_args.slice_dict = {"t": 2050, "e": "C"} |
| consumption.do_visualize = false | ||
| consumption.per_capita = false | ||
| sankey.do_visualize = false | ||
| sankey.plotter_args.slice_dict.t = 2050 |
There was a problem hiding this comment.
| sankey.plotter_args.slice_dict.t = 2050 | |
| sankey.plotter_args.slice_dict = {"t": 2050} |
Migrate from seperate per-model config files to more comprehensive config files that can be layered.
In particular:
basesettings plus per-material overrides. (No strong opinion about using toml, but its the 'pythonic' config format and Python has bultin support for reading such files)--configoptions for layered overrides and--modelto specify what model to run.Using typer it was then also easly to add an interactive prompt when no arguments are provided to make Bennet happy.
Fixes https://github.com/pik-piam/industry_issues/issues/11.
Possible follow-ups:
visualizationandexportssection are material-specific but actually follow the same pattern. We could remove those seperate config options by either using placeholders (egfigures_path = "data/%model%/output/figures) or use a non-configurable convention about the folder structure.