-
Notifications
You must be signed in to change notification settings - Fork 38
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
Merge configuration object from multiple files (instead of one single file) #2448
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2448 +/- ##
==========================================
+ Coverage 94.70% 94.77% +0.06%
==========================================
Files 249 249
Lines 14114 14218 +104
==========================================
+ Hits 13367 13475 +108
+ Misses 747 743 -4 ☔ View full report in Codecov by Sentry. |
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.
Nice work here @schlunma!
Would it be possible to do this in a backward compatible way? The deprecations are fine, but it would be nice if existing configurations kept working (at least for a while). For example, if |
Yes, 100% agree. That's why I already implemented that in exactly this way. This will work for two more minor versions, and only then it will raise an error. EDIT: My solution is even stricter: if |
@bouweandela I think I addressed all your comments, could you have another look? Thanks! |
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 for the massive effort @schlunma! I have a few more suggestions for improving the documentation and then I think we can ask some people to test
@@ -13,14 +13,6 @@ | |||
# file. | |||
# | |||
############################################################################### | |||
# |
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.
What is the plan for the ESMValTool copy, remove?
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.
Yes, I think that's the easiest solution. We don't want users to simply copy-paste the file anymore but rather use esmvaltool config ...
, so it doesn't need be located in the repo anymore.
Co-authored-by: Bouwe Andela <[email protected]>
@ESMValGroup/userengagementteam Could you please try this out and provide us with feedback on the user-friendliness? |
Description
This PR is the first step towards our new configuration system (see #2371). It allows reading the configuration from multiple files (and directories), similar to Dask's configuration. The different files are merged using
dask.config.collect()
.The following directories are considered (descending priority):
--config_dir
command line argument~/.config/esmvaltool
, but this can be changed withthe
ESMVALTOOL_CONFIG_DIR
environment variable)esmvalcore/config/configurations/defaults
)Related to #2371
Closes #805
Closes #1433
Link to documentation:
Deprecations (since v2.12.0, will be removed in v2.14.0)
Usage of single configuration file
~/.esmvaltool/config-user.yml
As mentioned above, ESMValTool will now read configuration directories (instead of one single file). By default, the directory
~/.config/esmvaltool
will be considered (can be changed with command line argument--config_dir
; see below). To switch to the new format, runConfiguration option
config_file
/ command line argument--config_file
:As mentioned above, ESMValTool will now read configuration directories (instead of one single file). Instead of specifying one file, specify a configuration directory with
--config_dir
. To switch to the new format, runConfig.load_from_file()
:Please
Config.load_from_dirs()
instead, e.g.Session.config_dir
:There is no replacement for this attribute.
Specify
extra_facets_dir
astuple
:Please use a
list
instead. For example,Backwards-incompatible change
In some cases, simply copying the old configuration file (e.g., from
~/.esmvaltool/config-user-yml
) to the new location might not be enough. In some cases, it might be necessary to adaptdrs
androotpath
settings. For example, on Levante, it might be necessary to changeto
Due to the new way settings are merged across configuration files, nested dictionaries are properly updated now.
Example:
The files
and
were previously merged to
In the new syntax, they are merged to
Thus, in the example above, it is necessary to explicitly specify
obs4MIPS: default
.Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
Changes are backward compatible see aboveTo help with the number pull requests: