diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index e38a1d3..60ef358 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -8,6 +8,14 @@ validate, patch, and query merged config files. ## Upgrading +- The legacy config layouts are no longer migrated on load: top-level microgrid + IDs and `meta`-nested microgrid fields are gone. A file with no `assets` table + now loads as empty and contributes nothing to a merge, rather than having its + top-level keys read as microgrid IDs. This lets a mixed list of files (some + carrying an `[app]` table for their own config, some an `assets` table) be + passed to `AssetsConfig.load_from_files` / `load_configs`, which reads only the + `assets`-bearing ones. Rebuild any legacy-layout files with `generate-config`. + - `generate-config` now writes the `assets.microgrids` layout and stamps `assets.version`, instead of the legacy layout. Regenerate configs to get the current format; generated files then load back without a migration. @@ -40,3 +48,10 @@ validate, patch, and query merged config files. - `generate-config --inplace` refreshes managed values while preserving formatting; `--fill-missing` only adds absent values. Generated patches are validated before writing. + +## Bug Fixes + +- Config files with validity periods now load under marshmallow 3, not only + marshmallow 4. `tomllib` yields native `datetime` objects, which marshmallow + 3's `DateTime` field rejected. `marshmallow` is now a direct dependency so the + minimum-version test exercises this path. diff --git a/pyproject.toml b/pyproject.toml index 0ed785c..9244e65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -26,6 +26,10 @@ classifiers = [ ] requires-python = ">= 3.11, < 4" dependencies = [ + # Used transitively via marshmallow-dataclass (every config schema is a + # `marshmallow.Schema`); declared directly so `pytest_min` pins its lower + # bound and the minimum-version path is actually tested. + "marshmallow >= 3.18.0, < 5", "marshmallow-dataclass >= 8.7.1, < 9", "typing-extensions >= 4.14.1, < 5", "frequenz-microgrid-component-graph >= 0.5.0, < 0.6", diff --git a/src/frequenz/gridpool/cli/_dump_config.py b/src/frequenz/gridpool/cli/_dump_config.py index abbec28..68104c9 100644 --- a/src/frequenz/gridpool/cli/_dump_config.py +++ b/src/frequenz/gridpool/cli/_dump_config.py @@ -17,6 +17,7 @@ fields are done here, since TOML has no null and `tomlkit` rejects it. """ +from dataclasses import asdict from typing import Any import tomlkit @@ -86,13 +87,10 @@ def dump_map(configs: dict[int, MicrogridConfig]) -> str: The TOML representation as a string, with one blank line between microgrids and entries sorted by numeric microgrid ID. """ - schema = MicrogridConfig.Schema() doc = tomlkit.document() doc.append(tomlkit.key(["assets", "version"]), _CURRENT_VERSION) for mid in sorted(configs): - dumped = schema.dump(configs[mid]) - assert isinstance(dumped, dict) - leaves = _iter_leaves([*_MICROGRID_PREFIX, str(mid)], dumped) + leaves = _iter_leaves([*_MICROGRID_PREFIX, str(mid)], asdict(configs[mid])) if not leaves: continue doc.add(tomlkit.nl()) diff --git a/src/frequenz/gridpool/cli/_patch_config.py b/src/frequenz/gridpool/cli/_patch_config.py index d550691..cd996ef 100644 --- a/src/frequenz/gridpool/cli/_patch_config.py +++ b/src/frequenz/gridpool/cli/_patch_config.py @@ -10,6 +10,7 @@ """ from collections.abc import Mapping +from dataclasses import asdict from pathlib import Path from typing import Any @@ -54,7 +55,6 @@ def patch_text( """ doc = tomlkit.parse(original) _reject_legacy_layout(doc) - schema = MicrogridConfig.Schema() # Leaves needing a whole new sub-table can't be inserted via item assignment # without tomlkit falling back to bracket-header syntax, so they are spliced @@ -63,9 +63,7 @@ def patch_text( for microgrid_id, cfg in configs.items(): mid = str(microgrid_id) - dumped = schema.dump(cfg) - assert isinstance(dumped, dict) - leaves = _iter_leaves([], dumped) + leaves = _iter_leaves([], asdict(cfg)) if not leaves: continue diff --git a/src/frequenz/gridpool/config/_assets.py b/src/frequenz/gridpool/config/_assets.py index 3b8d179..d5b0b87 100644 --- a/src/frequenz/gridpool/config/_assets.py +++ b/src/frequenz/gridpool/config/_assets.py @@ -443,23 +443,28 @@ def _read_assets_table(cls, config_path: Path) -> dict[str, Any]: """Read the raw `assets` table from a TOML file. The document is migrated to the current format before its `assets` - table is returned for merging. + table is returned for merging. A file with no `assets` table contributes + nothing to the merge, so consumers can pass a mixed list of files and + gridpool reads only the `assets`-bearing ones. Args: config_path: The path to the TOML configuration file. Returns: - The raw `assets` table, unvalidated, for merging before it is loaded. + The raw `assets` table, unvalidated, for merging before it is loaded, + or an empty table if the file has none. Raises: - TypeError: If `assets` is not a table. + TypeError: If `assets` is present but not a table. """ with config_path.open("rb") as f: data: dict[str, Any] = tomllib.load(f) data = migrate(data, config_path) - assets = data["assets"] + assets = data.get("assets") + if assets is None: + return {} if not isinstance(assets, dict): raise TypeError( f"{config_path}: `assets` must be a table, got {type(assets)}" diff --git a/src/frequenz/gridpool/config/_fields.py b/src/frequenz/gridpool/config/_fields.py new file mode 100644 index 0000000..39b96b8 --- /dev/null +++ b/src/frequenz/gridpool/config/_fields.py @@ -0,0 +1,29 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Reusable marshmallow fields for config data models.""" + +from datetime import datetime +from typing import Any + +import marshmallow + + +class TomlAwareDateTime(marshmallow.fields.DateTime): + """A `DateTime` that also accepts an already-parsed `datetime`. + + marshmallow 3's `DateTime` deserialises only ISO strings, but `tomllib` + yields native `datetime` objects before the value reaches marshmallow, so + those inputs must pass through unchanged. marshmallow 4 accepts them itself; + this keeps both behaving the same. + """ + + def _deserialize(self, value: Any, *args: Any, **kwargs: Any) -> datetime: + if isinstance(value, datetime): + return value + return super()._deserialize(value, *args, **kwargs) + + +def toml_datetime_metadata() -> dict[str, Any]: + """Field metadata mapping a TOML-sourced `datetime` to `TomlAwareDateTime`.""" + return {"marshmallow_field": TomlAwareDateTime(allow_none=True)} diff --git a/src/frequenz/gridpool/config/_microgrid.py b/src/frequenz/gridpool/config/_microgrid.py index 33f8bac..82c3b6a 100644 --- a/src/frequenz/gridpool/config/_microgrid.py +++ b/src/frequenz/gridpool/config/_microgrid.py @@ -11,6 +11,8 @@ from marshmallow import Schema from marshmallow_dataclass import dataclass +from ._fields import toml_datetime_metadata + ComponentType = Literal["grid", "pv", "battery", "consumption", "chp", "ev"] """Valid component types.""" @@ -98,10 +100,10 @@ def is_valid_type(cls, ctype: str) -> bool: class PVConfig: """Configuration of a PV system in a microgrid.""" - start_time: datetime | None = None + start_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Start time of the PV system installation.""" - end_time: datetime | None = None + end_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """End time of the PV system installation.""" peak_power: float | None = None @@ -119,10 +121,10 @@ class WindConfig: # pylint: disable=too-many-instance-attributes """Configuration of a wind turbine in a microgrid.""" - start_time: datetime | None = None + start_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Start time of the wind turbine installation.""" - end_time: datetime | None = None + end_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """End time of the wind turbine installation.""" turbine_model: str | None = None @@ -151,10 +153,10 @@ class WindConfig: class BatteryConfig: """Configuration of a battery in a microgrid.""" - start_time: datetime | None = None + start_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Start time of the battery installation.""" - end_time: datetime | None = None + end_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """End time of the battery installation.""" capacity: float | None = None @@ -187,10 +189,10 @@ class MicrogridConfig: altitude: float | None = None """Geographic altitude of the microgrid.""" - start_time: datetime | None = None + start_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Start time of the microgrid operation.""" - end_time: datetime | None = None + end_time: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """End time of the microgrid operation.""" pv: dict[str, PVConfig] | None = None diff --git a/src/frequenz/gridpool/config/_migrations.py b/src/frequenz/gridpool/config/_migrations.py index bd9dfca..da44970 100644 --- a/src/frequenz/gridpool/config/_migrations.py +++ b/src/frequenz/gridpool/config/_migrations.py @@ -3,18 +3,16 @@ """Format migrations for raw config documents. -Each step rewrites the raw `dict` a file parses to, before it is loaded into the -typed model. Steps are self-detecting and idempotent, so the `version` field -does not gate whether a step runs; it only records how old a file may be, which -bounds when a step may be dropped. +Loading validates `assets.version` and stamps the current version, so a file +written for a newer reader is rejected rather than silently misread. Each +migration step rewrites the raw `dict` before it is loaded into the typed +model; steps are self-detecting and idempotent, so the `version` field does not +gate whether a step runs. No legacy layouts remain, so `_STEPS` is empty. """ -import logging from pathlib import Path from typing import Any, Callable -_logger = logging.getLogger(__name__) - _CURRENT_VERSION = 1 @@ -27,7 +25,7 @@ def migrate(raw: dict[str, Any], source: Path) -> dict[str, Any]: Args: raw: The raw document as parsed from a file. - source: The file it was parsed from, for warnings and errors. + source: The file it was parsed from, named in error messages. Returns: The document in the current format. @@ -56,84 +54,5 @@ def migrate(raw: dict[str, Any], source: Path) -> dict[str, Any]: return raw -def _nest_top_level_microgrids(raw: dict[str, Any], source: Path) -> dict[str, Any]: - """Wrap deprecated top-level microgrid entries under `assets.microgrids`. - - Args: - raw: The raw document. - source: The file it was parsed from. - - Returns: - The document with its entries under `assets`. - - Raises: - ValueError: If both layouts are present, a half-migrated file rather - than a merge. - """ - if "assets" in raw: - stray = sorted(k for k in raw if k != "assets") - if stray: - raise ValueError( - f"{source}: keys {stray} sit outside `assets` while the file " - "already has an `assets` table; move them under `assets.microgrids`." - ) - return raw - - _logger.warning( - "%s: top-level microgrid IDs are deprecated, " - "nest the entries under `assets.microgrids` instead.", - source, - ) - return {"assets": {"microgrids": dict(raw)}} - - -def _lift_microgrid_meta(raw: dict[str, Any], source: Path) -> dict[str, Any]: - """Merge a deprecated nested `meta` table onto its microgrid entry. - - Earlier files nested a microgrid's fields under `meta`; they now sit - directly on the entry. - - Args: - raw: The raw document. - source: The file it was parsed from. - - Returns: - The document with microgrid fields directly on each entry. - - Raises: - TypeError: If a `meta` value is not a table. - ValueError: If a field is set both under `meta` and on the entry. - """ - assets = raw.get("assets") - if not isinstance(assets, dict): - return raw - microgrids = assets.get("microgrids") - if not isinstance(microgrids, dict): - return raw - for mid, entry in microgrids.items(): - if not isinstance(entry, dict) or "meta" not in entry: - continue - meta = entry.pop("meta") - if not isinstance(meta, dict): - raise TypeError(f"{source}: microgrid {mid} `meta` must be a table") - _logger.warning( - "%s: microgrid %s nests fields under `meta`; that is deprecated, " - "set them directly on the entry instead.", - source, - mid, - ) - for key, value in meta.items(): - if key in entry: - raise ValueError( - f"{source}: microgrid {mid} sets `{key}` both under `meta` " - "and directly; keep only the direct one" - ) - entry[key] = value - return raw - - -_STEPS: list[tuple[int, Callable[[dict[str, Any], Path], dict[str, Any]]]] = [ - (0, _nest_top_level_microgrids), - (0, _lift_microgrid_meta), -] +_STEPS: list[tuple[int, Callable[[dict[str, Any], Path], dict[str, Any]]]] = [] """Migration steps as `(from_version, step)`, oldest first.""" diff --git a/src/frequenz/gridpool/config/_topology.py b/src/frequenz/gridpool/config/_topology.py index c17909a..99c7e08 100644 --- a/src/frequenz/gridpool/config/_topology.py +++ b/src/frequenz/gridpool/config/_topology.py @@ -26,6 +26,8 @@ from marshmallow import Schema from marshmallow_dataclass import dataclass +from ._fields import toml_datetime_metadata + _EIC_ALPHABET = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ-" _MARKET_AREA_EU_DE = 101 @@ -165,10 +167,10 @@ class ValidityConfig: participation: MarketParticipationType | None = None """The market use case, unset for a plain validity window.""" - start: datetime | None = None + start: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Inclusive start of the period.""" - end: datetime | None = None + end: datetime | None = field(default=None, metadata=toml_datetime_metadata()) """Exclusive end of the period.""" Schema: ClassVar[Type[Schema]] = Schema diff --git a/tests/test_config.py b/tests/test_config.py index ca8bd87..0e57b87 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -121,17 +121,17 @@ def test_microgrid_config_formula(valid_microgrid_config: MicrogridConfig) -> No def test_load_configs(mocker: MockerFixture) -> None: """Test loading configurations for multiple microgrids from mock TOML files.""" toml_data = """ - 1.microgrid_id = 1 - 1.name = "Test Grid" - 1.gid = 1 - 1.ctype.pv.meter = [101, 102] - 1.ctype.battery.inverter = [201, 202, 203] - 1.ctype.battery.component = [301, 302, 303, 304, 305, 306] - 1.pv.PV1.peak_power = 5000 - 1.pv.PV1.rated_power = 4500 - 1.pv.PV2.peak_power = 8000 - 1.pv.PV2.rated_power = 7000 - 1.battery.BAT1.capacity = 10000 + assets.microgrids.1.microgrid_id = 1 + assets.microgrids.1.name = "Test Grid" + assets.microgrids.1.gid = 1 + assets.microgrids.1.ctype.pv.meter = [101, 102] + assets.microgrids.1.ctype.battery.inverter = [201, 202, 203] + assets.microgrids.1.ctype.battery.component = [301, 302, 303, 304, 305, 306] + assets.microgrids.1.pv.PV1.peak_power = 5000 + assets.microgrids.1.pv.PV1.rated_power = 4500 + assets.microgrids.1.pv.PV2.peak_power = 8000 + assets.microgrids.1.pv.PV2.rated_power = 7000 + assets.microgrids.1.battery.BAT1.capacity = 10000 """ mock_file = mocker.mock_open(read_data=toml_data.encode("utf-8")) mocker.patch("pathlib.Path.open", mock_file) @@ -175,12 +175,6 @@ def _assert_optional_field(value: float | None, expected: float) -> None: assets.microgrids.1.ctype.pv.meter = [101, 102] """ -_LEGACY_TOML = """ -1.microgrid_id = 1 -1.name = "Test Grid" -1.ctype.pv.meter = [101, 102] -""" - def _write(tmp_path: Path, name: str, text: str) -> Path: """Write `text` to `name` under `tmp_path` and return the path.""" @@ -200,47 +194,18 @@ def test_load_prefixed(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None assert "deprecated" not in caplog.text -def test_load_legacy_warns(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: - """Top-level entries still load, but warn and name the file.""" - path = _write(tmp_path, "legacy.toml", _LEGACY_TOML) - - with caplog.at_level(logging.WARNING): - configs = AssetsConfig.load_from_files(path).microgrids - - assert configs[1].name == "Test Grid" - assert "deprecated" in caplog.text - assert str(path) in caplog.text - - -def test_load_meta_layout_warns( - tmp_path: Path, caplog: pytest.LogCaptureFixture -) -> None: - """A microgrid nesting its fields under `meta` still loads, but warns.""" - path = _write( - tmp_path, - "meta.toml", - "assets.microgrids.1.meta.microgrid_id = 1\n" - 'assets.microgrids.1.meta.name = "Test Grid"\n', - ) - - with caplog.at_level(logging.WARNING): - configs = AssetsConfig.load_from_files(path).microgrids - - assert configs[1].name == "Test Grid" - assert "meta" in caplog.text - assert "deprecated" in caplog.text - - -def test_load_mixed_layouts_rejected(tmp_path: Path) -> None: - """A half-migrated file with both layouts is an error, not a merge.""" +def test_load_ignores_keys_outside_assets(tmp_path: Path) -> None: + """Top-level keys next to `assets` are ignored; only `assets` is read.""" path = _write( tmp_path, "mixed.toml", _PREFIXED_TOML + '2.microgrid_id = 2\n2.name = "Other Grid"\n', ) - with pytest.raises(ValueError, match="outside `assets`"): - AssetsConfig.load_from_files(path) + configs = AssetsConfig.load_from_files(path).microgrids + + assert set(configs) == {1} + assert configs[1].name == "Test Grid" def test_load_assets_without_microgrids(tmp_path: Path) -> None: @@ -250,18 +215,16 @@ def test_load_assets_without_microgrids(tmp_path: Path) -> None: assert not AssetsConfig.load_from_files(path).microgrids -async def test_merge_prefixed_base_with_legacy_override(tmp_path: Path) -> None: - """Layers of either layout merge as usual, since layout is resolved on load.""" +async def test_merge_ignores_a_non_assets_override(tmp_path: Path) -> None: + """An override file with no `assets` table contributes nothing to the merge.""" base = _write(tmp_path, "base.toml", _PREFIXED_TOML) - override = _write( - tmp_path, "override.toml", '1.microgrid_id = 1\n1.name = "Renamed"\n' - ) + override = _write(tmp_path, "override.toml", '[app]\nname = "consumer"\n') configs = ( await load_configs(default_files=base, override_files=override) ).microgrids - assert configs[1].name == "Renamed" + assert configs[1].name == "Test Grid" assert configs[1].component_type_ids("pv") == [101, 102] diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 8e1f213..8a8ed59 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -11,31 +11,6 @@ from frequenz.gridpool.config._migrations import _CURRENT_VERSION, migrate -def test_migrate_nests_top_level_microgrids() -> None: - """A v0 top-level document is nested under `assets.microgrids` and stamped.""" - raw = {"1": {"microgrid_id": 1, "name": "Grid"}} - - migrated = migrate(raw, Path("v0.toml")) - - assert migrated == { - "assets": { - "microgrids": {"1": {"microgrid_id": 1, "name": "Grid"}}, - "version": _CURRENT_VERSION, - } - } - - -def test_migrate_lifts_nested_meta() -> None: - """A microgrid nesting fields under `meta` has them lifted onto the entry.""" - raw = { - "assets": {"microgrids": {"1": {"meta": {"microgrid_id": 1, "name": "Grid"}}}} - } - - migrated = migrate(raw, Path("meta.toml")) - - assert migrated["assets"]["microgrids"]["1"] == {"microgrid_id": 1, "name": "Grid"} - - def test_migrate_is_a_noop_on_a_current_document() -> None: """A current document passes through unchanged.""" raw = { @@ -48,20 +23,6 @@ def test_migrate_is_a_noop_on_a_current_document() -> None: assert migrate(raw, Path("current.toml")) == raw -def test_migrate_rejects_mixed_layout_at_current_version() -> None: - """Layout validation still runs when a document declares the current version.""" - raw = { - "assets": { - "version": _CURRENT_VERSION, - "microgrids": {"1": {"microgrid_id": 1}}, - }, - "2": {"microgrid_id": 2}, - } - - with pytest.raises(ValueError, match="outside `assets`"): - migrate(raw, Path("mixed.toml")) - - def test_migrate_rejects_a_newer_version() -> None: """A future document is not silently downgraded.""" future_version = _CURRENT_VERSION + 1 @@ -80,12 +41,30 @@ def test_migrate_rejects_a_newer_version() -> None: assert raw["assets"]["version"] == future_version -def test_v0_top_level_file_loads(tmp_path: Path) -> None: - """A v0 top-level file still loads through the public loader.""" - path = tmp_path / "v0.toml" - path.write_text('1.microgrid_id = 1\n1.name = "Grid"\n') +def test_file_without_assets_loads_empty(tmp_path: Path) -> None: + """A file with no `assets` table loads empty and contributes nothing to a merge.""" + other = tmp_path / "app.toml" + other.write_text('[app]\nname = "consumer"\n') + real = tmp_path / "assets.toml" + real.write_text( + 'assets.microgrids.1.microgrid_id = 1\nassets.microgrids.1.name = "Grid"\n' + ) + + assert AssetsConfig.load_from_files(other).microgrids == {} + + merged = AssetsConfig.load_from_files([other, real]) + assert merged.microgrids[1].name == "Grid" + + +def test_file_with_app_and_assets_reads_only_assets(tmp_path: Path) -> None: + """A sibling `[app]` table next to `assets` is ignored; only `assets` is read.""" + path = tmp_path / "mixed.toml" + path.write_text( + "assets.microgrids.1.microgrid_id = 1\n" + 'assets.microgrids.1.name = "Grid"\n\n' + '[app]\nname = "consumer"\n' + ) config = AssetsConfig.load_from_files(path) assert config.microgrids[1].name == "Grid" - assert config.version == _CURRENT_VERSION diff --git a/tests/test_topology.py b/tests/test_topology.py index 3c1a28c..d71c402 100644 --- a/tests/test_topology.py +++ b/tests/test_topology.py @@ -575,6 +575,27 @@ def test_period_and_query_require_a_utc_offset() -> None: relation.covers(naive) +def test_validity_accepts_datetime_objects_and_iso_strings() -> None: + """A period loads from native datetimes, as `tomllib` yields, or ISO strings.""" + aware = datetime(2026, 1, 1, tzinfo=timezone.utc) + + from_objects = ValidityConfig.Schema().load( + {"start": aware, "end": aware.replace(month=6)} + ) + assert isinstance(from_objects, ValidityConfig) + assert from_objects.start == aware + + from_strings = ValidityConfig.Schema().load( + {"start": "2026-01-01T00:00:00+00:00", "end": "2026-06-01T00:00:00+00:00"} + ) + assert isinstance(from_strings, ValidityConfig) + assert from_strings.start == aware + + naive = datetime(2026, 1, 1) # noqa: DTZ001 + with pytest.raises(ValueError, match="UTC offset"): + ValidityConfig.Schema().load({"start": naive}) + + def test_market_location_properties() -> None: """A location identifies its market and how to read its identifier.""" config = _load(