Skip to content
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

Add public Config.update_from_dirs() method #2538

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/api/esmvalcore.config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,13 @@ To load the configuration object from custom directories, use:
>>> dirs = ['my/default/config', 'my/custom/config']
>>> CFG.load_from_dirs(dirs)

To update the existing configuration object from custom directories, use:

.. code-block:: python

>>> dirs = ['my/default/config', 'my/custom/config']
>>> CFG.update_from_dirs(dirs)


Session
*******
Expand Down
10 changes: 4 additions & 6 deletions esmvalcore/_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,6 @@ def run(self, recipe, **kwargs):

"""
from .config import CFG
from .config._config_object import _get_all_config_dirs
from .exceptions import InvalidConfigParameter

cli_config_dir = kwargs.pop("config_dir", None)
Expand All @@ -427,10 +426,9 @@ def run(self, recipe, **kwargs):

# New in v2.12.0: read additional configuration directory given by CLI
# argument
if CFG.get("config_file") is None: # remove in v2.14.0
config_dirs = _get_all_config_dirs(cli_config_dir)
if CFG.get("config_file") is None and cli_config_dir is not None:
try:
CFG.load_from_dirs(config_dirs)
CFG.update_from_dirs([cli_config_dir])

# Potential errors must come from --config_dir (i.e.,
# cli_config_dir) since other sources have already been read (and
Expand Down Expand Up @@ -458,8 +456,8 @@ def run(self, recipe, **kwargs):

# New in v2.12.0
else:
config_dirs = _get_all_config_dirs(cli_config_dir) # remove v2.14
CFG.load_from_dirs(config_dirs)
if cli_config_dir is not None:
CFG.update_from_dirs([cli_config_dir])

@staticmethod
def _create_session_dir(session):
Expand Down
56 changes: 46 additions & 10 deletions esmvalcore/config/_config_object.py
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,17 @@ def load_from_file(
self.clear()
self.update(Config._load_user_config(filename))

@staticmethod
def _get_config_dict_from_dirs(dirs: Iterable[str | Path]) -> dict:
"""Get configuration :obj:`dict` from directories."""
dirs_str: list[str] = []
for config_dir in dirs:
config_dir = Path(config_dir).expanduser().absolute()
dirs_str.append(str(config_dir))
return dask.config.collect(paths=dirs_str, env={})

def load_from_dirs(self, dirs: Iterable[str | Path]) -> None:
"""Load configuration object from directories.
"""Clear and load configuration object from directories.

This searches for all YAML files within the given directories and
merges them together using :func:`dask.config.collect`. Nested objects
Expand All @@ -344,23 +353,17 @@ def load_from_dirs(self, dirs: Iterable[str | Path]) -> None:
Invalid configuration option given.

"""
dirs_str: list[str] = []

# Always consider default options; these have the lowest priority
dirs_str.append(str(DEFAULT_CONFIG_DIR))
dirs = [DEFAULT_CONFIG_DIR] + list(dirs)

for config_dir in dirs:
config_dir = Path(config_dir).expanduser().absolute()
dirs_str.append(str(config_dir))

new_config_dict = dask.config.collect(paths=dirs_str, env={})
new_config_dict = self._get_config_dict_from_dirs(dirs)
self.clear()
self.update(new_config_dict)

self.check_missing()

def reload(self) -> None:
"""Reload the configuration object.
"""Clear and reload the configuration object.

This will read all YAML files in the user configuration directory (by
default ``~/.config/esmvaltool``, but this can be changed with the
Expand Down Expand Up @@ -431,6 +434,39 @@ def start_session(self, name: str) -> Session:
session = Session(config=self.copy(), name=name)
return session

def update_from_dirs(self, dirs: Iterable[str | Path]) -> None:
"""Update configuration object from directories.

This will first search for all YAML files within the given directories
and merge them together using :func:`dask.config.collect` (values in
the latter directories are preferred to those in the former). Then, the
schlunma marked this conversation as resolved.
Show resolved Hide resolved
current configuration is merged with these new configuration options
using :func:`dask.config.merge` (new values are preferred over old
values). Nested objects are properly considered; see
:func:`dask.config.update` for details.

Note
----
Just like :func:`dask.config.collect`, this silently ignores
non-existing directories.

Parameters
----------
dirs:
A list of directories to search for YAML configuration files.

Raises
------
esmvalcore.exceptions.InvalidConfigParameter
Invalid configuration option given.

"""
new_config_dict = self._get_config_dict_from_dirs(dirs)
merged_config_dict = dask.config.merge(self, new_config_dict)
Copy link
Contributor

@valeriupredoi valeriupredoi Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this throws a wobbly, do you want to catch and release that or filter it through our own custom exception? Just looked at both merge and update from Dask, and they're really basic, and do not account for any possible exception, so if there are exceptions, then those will be cryptic AF

self.update(merged_config_dict)

self.check_missing()


class Session(ValidatedConfig):
"""Container class for session configuration and directory information.
Expand Down
111 changes: 84 additions & 27 deletions tests/unit/config/test_config_object.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,33 +393,8 @@ def test_get_global_config_deprecated(mocker, tmp_path):
assert cfg["output_dir"] == Path("/new/output/dir")


@pytest.mark.parametrize(
"dirs,output_file_type,rootpath",
[
([], "png", {"default": "~/climate_data"}),
(["/this/path/does/not/exist"], "png", {"default": "~/climate_data"}),
(["{tmp_path}/config1"], "1", {"default": "1", "1": "1"}),
(
["{tmp_path}/config1", "/this/path/does/not/exist"],
"1",
{"default": "1", "1": "1"},
),
(
["{tmp_path}/config1", "{tmp_path}/config2"],
"2b",
{"default": "2b", "1": "1", "2": "2b"},
),
(
["{tmp_path}/config2", "{tmp_path}/config1"],
"1",
{"default": "1", "1": "1", "2": "2b"},
),
],
)
def test_load_from_dirs_always_default(
dirs, output_file_type, rootpath, tmp_path
):
"""Test `Config.load_from_dirs`."""
def _setup_config_dirs(tmp_path):
"""Setup test configuration directories."""
config1 = tmp_path / "config1" / "1.yml"
config2a = tmp_path / "config2" / "2a.yml"
config2b = tmp_path / "config2" / "2b.yml"
Expand Down Expand Up @@ -456,6 +431,36 @@ def test_load_from_dirs_always_default(
)
)


@pytest.mark.parametrize(
"dirs,output_file_type,rootpath",
[
([], "png", {"default": "~/climate_data"}),
(["/this/path/does/not/exist"], "png", {"default": "~/climate_data"}),
(["{tmp_path}/config1"], "1", {"default": "1", "1": "1"}),
(
["{tmp_path}/config1", "/this/path/does/not/exist"],
"1",
{"default": "1", "1": "1"},
),
(
["{tmp_path}/config1", "{tmp_path}/config2"],
"2b",
{"default": "2b", "1": "1", "2": "2b"},
),
(
["{tmp_path}/config2", "{tmp_path}/config1"],
"1",
{"default": "1", "1": "1", "2": "2b"},
),
],
)
def test_load_from_dirs_always_default(
dirs, output_file_type, rootpath, tmp_path
):
"""Test `Config.load_from_dirs`."""
_setup_config_dirs(tmp_path)

config_dirs = []
for dir_ in dirs:
config_dirs.append(dir_.format(tmp_path=str(tmp_path)))
Expand All @@ -465,11 +470,14 @@ def test_load_from_dirs_always_default(

cfg = Config()
assert not cfg
cfg["rootpath"] = {"X": "x"}
cfg["search_esgf"] = "when_missing"

cfg.load_from_dirs(config_dirs)

assert cfg["output_file_type"] == output_file_type
assert cfg["rootpath"] == rootpath
assert cfg["search_esgf"] == "never"


@pytest.mark.parametrize(
Expand Down Expand Up @@ -514,3 +522,52 @@ def test_get_all_config_sources(cli_config_dir, output, monkeypatch):
cli_config_dir
)
assert config_srcs == output


@pytest.mark.parametrize(
"dirs,output_file_type,rootpath",
[
([], None, {"X": "x"}),
(["/this/path/does/not/exist"], None, {"X": "x"}),
(["{tmp_path}/config1"], "1", {"default": "1", "1": "1", "X": "x"}),
(
["{tmp_path}/config1", "/this/path/does/not/exist"],
"1",
{"default": "1", "1": "1", "X": "x"},
),
(
["{tmp_path}/config1", "{tmp_path}/config2"],
"2b",
{"default": "2b", "1": "1", "2": "2b", "X": "x"},
),
(
["{tmp_path}/config2", "{tmp_path}/config1"],
"1",
{"default": "1", "1": "1", "2": "2b", "X": "x"},
),
],
)
def test_update_from_dirs(dirs, output_file_type, rootpath, tmp_path):
"""Test `Config.update_from_dirs`."""
_setup_config_dirs(tmp_path)

config_dirs = []
for dir_ in dirs:
config_dirs.append(dir_.format(tmp_path=str(tmp_path)))
for name, path in rootpath.items():
path = Path(path).expanduser().absolute()
rootpath[name] = [path]

cfg = Config()
assert not cfg
cfg["rootpath"] = {"X": "x"}
cfg["search_esgf"] = "when_missing"

cfg.update_from_dirs(config_dirs)

if output_file_type is None:
assert "output_file_type" not in cfg
else:
assert cfg["output_file_type"] == output_file_type
assert cfg["rootpath"] == rootpath
assert cfg["search_esgf"] == "when_missing"