-
Notifications
You must be signed in to change notification settings - Fork 7
Configuration #936
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
base: main
Are you sure you want to change the base?
Configuration #936
Conversation
…FrozenNamespace by Enum
# Conflicts: # model/common/pyproject.toml # uv.lock
… _nudge_max_coeff from config yml
fix Mixin import
# Conflicts: # model/atmosphere/diffusion/tests/diffusion/integration_tests/test_benchmark_diffusion.py # model/testing/src/icon4py/model/testing/definitions.py
# Conflicts: # model/atmosphere/diffusion/tests/diffusion/integration_tests/test_benchmark_diffusion.py # model/atmosphere/dycore/tests/dycore/integration_tests/test_benchmark_solve_nonhydro.py # model/driver/src/icon4py/model/driver/icon4py_configuration.py # model/testing/src/icon4py/model/testing/fixtures/datatest.py
# Conflicts: # model/atmosphere/diffusion/src/icon4py/model/atmosphere/diffusion/diffusion.py # uv.lock
|
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
To run tests and benchmarks with the DaCe backend you can use:
To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
| self._initialize_components(user_config) | ||
|
|
||
| # TODO (halungge): have this return a std dict to hide away omegaconf | ||
| def get( |
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.
Make this return a TypeDict..
OngChia
left a 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.
Thanks a lot for the configuration infrastructure!
It looks awesome. Currently, I have a few questions on the configuration manager.
| """ | ||
|
|
||
| # number of dynamics substeps | ||
| ndyn_substeps: int = dataclasses.field( |
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.
In contrast to DiffusionConfig, we do not need ndyn_substeps for NonHydrostaticConfig because stencils in SolveNonhydro need the runtime changeable ndyn_substeps_var.
|
|
||
| # from mo_nonhydrostatic_nml.f90 | ||
| ndyn_substeps: int = dataclasses.field( | ||
| default=common_config.resolve_or_else("model.ndyn_substeps", 5) |
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.
Does ndyn_substeps always have to be inherited from a outer config? Or I can do, let's say,
ndyn_substeps: int = dataclasses.field(
default=common_config.resolve_or_else("model.run.ndyn_substeps", 5)
)
I feel that it is more suitable for ndyn_substeps to sit in the run config, because this variable controls how many substeps the driver runs the dycore.
| default_reader = module.init_config() | ||
| default_reader.update(update) | ||
| else: | ||
| log.warning(f" module {module.__name__} has no `init_config` function") |
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.
Should we force config.py in all components has this init_config function?
Otherwise, it seems that users can have some wrong parameters in the full_model_config.yaml file and probably an error will be thrown when the granule is initialized.
| def get( | ||
| self, | ||
| is_default: bool = False, | ||
| ) -> oc.DictConfig: |
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.
I am thinking that whether to give users an option of returning a dictionary of config classes (run, dycore, diffusion, ...) for convenience?
| f"No default configuration and no user-configuration for component {name} defined in file {self._config_file}" | ||
| ) | ||
| else: | ||
| reader.update(module_config) |
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 may be true in the if statement above that users have developed a component without any config parameters.
However, I am wondering that should we just throw an error here in this case where users put a config for a particular component whereas there is in config.py.
Adds Configuration for ICON4Py in two parts:
commonprovides aConfigurationHandlerfor initialization of configuration for a@dataclassor a yaml file. This one can be used in each model component. There are two examples for this inmodel.atmosphere.diffusionandmodel.atmosphere.dycoreConfigurationMangerinmodel.driverwhich is supposed to be used in a model run. It reads a yaml file and initializes aConfigurationHandlerfor each model registered in the model config and makes the configuration available (as dictionary) to the model.