From 858edabe82d8d3eb8028b4fee889af64e0246aca Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:39:27 +0000 Subject: [PATCH 01/25] Add `__contains__` to `Bounds` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Give `Bounds` membership-testing capability, so callers can write `value in bounds` to check whether a value falls within the range. The semantics are: * both bounds are inclusive (`lower <= value <= upper`), * a `None` bound means unbounded in that direction, and * `None` is a bound marker only, never a value, so `None in bounds` is always `False`. The method lives on `Bounds` alone, not on `BaseBounds` or `InvalidBounds`: malformed bounds must not be range-checked — preserving them as `InvalidBounds` is precisely so callers inspect the raw values instead of testing membership against a broken range. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 21 +++++++++++++ tests/metrics/_bounds/test_bounds.py | 31 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 30896b9a..d865a095 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -69,6 +69,27 @@ def __str__(self) -> str: """Return a string representation of these bounds.""" return f"[{self.lower},{self.upper}]" + def __contains__(self, item: float | None) -> bool: + """Check whether a value is within these bounds. + + The bounds are inclusive on both ends, and a `None` bound means these + bounds are unbounded in that direction. `None` is a bound marker only + and is never itself a value, so `None` is never contained. + + Args: + item: The value to check. + + Returns: + Whether `item` is within these bounds. + """ + if item is None: + return False + if self.lower is not None and item < self.lower: + return False + if self.upper is not None and item > self.upper: + return False + return True + @dataclasses.dataclass(frozen=True, kw_only=True) class InvalidBounds(BaseBounds): diff --git a/tests/metrics/_bounds/test_bounds.py b/tests/metrics/_bounds/test_bounds.py index ee3d5dd7..5d5ce5b4 100644 --- a/tests/metrics/_bounds/test_bounds.py +++ b/tests/metrics/_bounds/test_bounds.py @@ -75,3 +75,34 @@ def test_hash() -> None: bounds_dict = {bounds1: "test1", bounds3: "test2"} assert len(bounds_dict) == 2 + + +@pytest.mark.parametrize( + "lower, upper, item, expected", + [ + (None, None, 0.0, True), + (None, None, 1e9, True), + (-10.0, 10.0, 0.0, True), + (-10.0, 10.0, -10.0, True), # lower bound is inclusive + (-10.0, 10.0, 10.0, True), # upper bound is inclusive + (-10.0, 10.0, -10.1, False), + (-10.0, 10.0, 10.1, False), + (None, 10.0, -1e9, True), # unbounded below + (None, 10.0, 10.0, True), + (None, 10.0, 10.1, False), + (-10.0, None, 1e9, True), # unbounded above + (-10.0, None, -10.0, True), + (-10.0, None, -10.1, False), + ], +) +def test_contains( + lower: float | None, upper: float | None, item: float, expected: bool +) -> None: + """Test membership with `in`, inclusive on both ends.""" + assert (item in Bounds(lower=lower, upper=upper)) is expected + + +def test_contains_none() -> None: + """`None` is never contained, even by unbounded bounds.""" + assert None not in Bounds() + assert None not in Bounds(lower=-10.0, upper=10.0) From 2fa80f84919de7a1b64f5ed8a56703a3c23ed7aa Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:41:04 +0000 Subject: [PATCH 02/25] Add `__bool__` and `is_bounded()` to `Bounds` Let `Bounds` answer "do these bounds restrict anything?" so that a `Bounds | None` field can be tested uniformly: both `None` and a fully unbounded `Bounds()` become falsy, so `if not bounds:` reads as "unbounded". Any set `lower` or `upper` makes the bounds truthy. `is_bounded()` is the explicit spelling of that truthiness, following the method style used elsewhere (e.g. `Microgrid.is_active()`), for callers who prefer a named predicate over relying on `bool()`. Emptiness is decided by `is not None`, not by the value's own truthiness, so `Bounds(lower=0.0, upper=0.0)` is correctly bounded rather than being mistaken for unbounded because `0.0` is falsy. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 24 +++++++++++++++++++ tests/metrics/_bounds/test_bounds.py | 19 +++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index d865a095..234657bb 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -90,6 +90,30 @@ def __contains__(self, item: float | None) -> bool: return False return True + def __bool__(self) -> bool: + """Return whether these bounds restrict the range in any direction. + + Fully unbounded bounds (`Bounds()`, where both `lower` and `upper` + are `None`) accept every value and are therefore falsy; any set bound + makes them truthy. + + Returns: + Whether at least one of `lower` or `upper` is set. + """ + return self.lower is not None or self.upper is not None + + def is_bounded(self) -> bool: + """Return whether these bounds restrict the range in any direction. + + This is the explicit spelling of these bounds' truthiness: fully + unbounded bounds (`Bounds()`) are not bounded, while any set `lower` + or `upper` makes them bounded. + + Returns: + Whether at least one of `lower` or `upper` is set. + """ + return bool(self) + @dataclasses.dataclass(frozen=True, kw_only=True) class InvalidBounds(BaseBounds): diff --git a/tests/metrics/_bounds/test_bounds.py b/tests/metrics/_bounds/test_bounds.py index 5d5ce5b4..2284a9cc 100644 --- a/tests/metrics/_bounds/test_bounds.py +++ b/tests/metrics/_bounds/test_bounds.py @@ -106,3 +106,22 @@ def test_contains_none() -> None: """`None` is never contained, even by unbounded bounds.""" assert None not in Bounds() assert None not in Bounds(lower=-10.0, upper=10.0) + + +@pytest.mark.parametrize( + "lower, upper, expected", + [ + (None, None, False), # fully unbounded accepts everything -> falsy + (-10.0, None, True), + (None, 10.0, True), + (-10.0, 10.0, True), + (0.0, 0.0, True), # a zero bound still counts as bounded + ], +) +def test_bool_and_is_bounded( + lower: float | None, upper: float | None, expected: bool +) -> None: + """Unbounded bounds are falsy; any set bound makes them bounded/truthy.""" + bounds = Bounds(lower=lower, upper=upper) + assert bool(bounds) is expected + assert bounds.is_bounded() is expected From b91f8dcc0ade9041172398c6c02a896b79d063ad Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:53:53 +0000 Subject: [PATCH 03/25] Add `BoundsSet` Introduce a normalized union of `Bounds` for efficient membership testing. On construction the bounds are sorted by lower value and overlapping or touching bounds are merged (inclusive, so `[1, 5]` and `[5, 10]` become `[1, 10]`), so the stored `bounds` are canonical: sorted and pairwise non-overlapping. Membership checks perform a binary search over the normalized bounds. `BoundsSet` is a domain-specialized set, not a mathematical one: the empty set is the *unbounded* set. It contains every value and is falsy, so `not bounds_set` reliably means "unbounded". To keep that invariant honest even when unboundedness arrives split across bounds (e.g. the wire sends `[None, 5]` and `[3, None]`), a union that covers the whole space collapses to the empty set, giving unboundedness a single canonical representation. `__contains__` special-cases the empty set to contain everything, and `__bool__` / `is_bounded()` report emptiness. The type deliberately omits `__iter__` and `__len__`: the normalized sequence is public as `BoundsSet.bounds`, and querying membership by iterating it would disagree with `value in bounds_set` for the unbounded set, so `in` is kept the single authoritative operation. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 3 +- src/frequenz/client/common/metrics/_bounds.py | 187 ++++++++++++++++++ tests/metrics/_bounds/test_bounds_set.py | 164 +++++++++++++++ 3 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 tests/metrics/_bounds/test_bounds_set.py diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index 987f203c..cad245d2 100644 --- a/src/frequenz/client/common/metrics/__init__.py +++ b/src/frequenz/client/common/metrics/__init__.py @@ -3,7 +3,7 @@ """Metrics definitions.""" -from ._bounds import BaseBounds, Bounds, InvalidBounds, InvalidBoundsError +from ._bounds import BaseBounds, Bounds, BoundsSet, InvalidBounds, InvalidBoundsError from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -18,6 +18,7 @@ "AggregationMethod", "BaseBounds", "Bounds", + "BoundsSet", "InvalidBounds", "InvalidBoundsError", "Metric", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 234657bb..0ae0a8a4 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -4,7 +4,10 @@ """Definitions for bounds.""" +import bisect import dataclasses +import math +from collections.abc import Iterable from typing import Any, Self from .._exception import InvalidAttributeError @@ -168,3 +171,187 @@ def __init__( else f"invalid bounds {bounds!r} for attribute {attr_name!r} in {instance}" ), ) + + +def _end_covers_start(upper: float | None, lower: float | None) -> bool: + """Return whether an upper bound reaches a lower bound, treating `None` as ±∞. + + Args: + upper: An upper bound, where `None` means +∞. + lower: A lower bound, where `None` means -∞. + + Returns: + Whether `upper >= lower` under the ±∞ convention. + """ + if upper is None: + return True + if lower is None: + return True + return not upper < lower + + +def _max_upper(first: float | None, second: float | None) -> float | None: + """Return the larger of two upper bounds, where `None` means +∞. + + Args: + first: An upper bound. + second: Another upper bound. + + Returns: + The larger of `first` and `second` under the +∞ convention. + """ + if first is None or second is None: + return None + return second if first < second else first + + +def _sort_and_merge_bounds(bounds: Iterable[Bounds]) -> tuple[Bounds, ...]: + """Sort bounds by lower value and merge overlapping or touching ones. + + A `None` lower bound is treated as -∞ and a `None` upper bound as +∞. + Bounds are inclusive on both ends, so `[1, 5]` and `[5, 10]` touch and + merge into `[1, 10]`. If the merged result covers the whole space (a single + unbounded `[None, None]`), the empty tuple is returned instead, so the + unbounded set has a single canonical (empty) representation. + + Args: + bounds: The bounds to normalize. + + Returns: + A tuple of sorted, pairwise non-overlapping bounds covering the same + values as the input, or the empty tuple when the union is unbounded. + """ + all_bounds = list(bounds) + if not all_bounds: + return () + + with_none_lower: list[Bounds] = [] + with_real_lower: list[tuple[float, Bounds]] = [] + for bound in all_bounds: + if bound.lower is None: + with_none_lower.append(bound) + else: + with_real_lower.append((bound.lower, bound)) + with_real_lower.sort(key=lambda pair: pair[0]) + ordered = [pair[1] for pair in with_real_lower] + + if with_none_lower: + if any(bound.upper is None for bound in with_none_lower): + ordered.insert(0, Bounds(lower=None, upper=None)) + else: + uppers = [b.upper for b in with_none_lower if b.upper is not None] + ordered.insert(0, Bounds(lower=None, upper=max(uppers))) + + result: list[Bounds] = [ordered[0]] + for current in ordered[1:]: + last = result[-1] + if _end_covers_start(last.upper, current.lower): + result[-1] = Bounds( + lower=last.lower, upper=_max_upper(last.upper, current.upper) + ) + else: + result.append(current) + + if len(result) == 1 and result[0].lower is None and result[0].upper is None: + return () + return tuple(result) + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class BoundsSet: + """A normalized set of metric bounds for efficient membership testing. + + A `BoundsSet` represents the union of a collection of + [`Bounds`][..Bounds]: a value is contained when it falls within *any* of + them. This matches the way multiple metric-sample bounds work — the value + must be within at least one of the bounds. On construction the bounds are + sorted by their lower bound and any overlapping or touching bounds are + merged, so the stored `bounds` are canonical: sorted and pairwise + non-overlapping. + + Note: + This is a domain-specialized set, not a mathematical one: **the empty + set is the unbounded set**. It contains every value and is falsy, so + `not bounds_set` reliably means "unbounded" (bounds that together cover + the whole space also normalize to the empty set). Because of this, + membership must be tested with `value in bounds_set`, which is + authoritative — do not reconstruct it by iterating `bounds`, since the + two disagree for the unbounded set. + + Example: + ```python + from frequenz.client.common.metrics import Bounds, BoundsSet + + allowed = BoundsSet( + bounds=( + Bounds(lower=1.0, upper=5.0), + Bounds(lower=3.0, upper=10.0), + Bounds(lower=15.0, upper=20.0), + ) + ) + # Overlapping bounds are merged on construction. + assert allowed.bounds == ( + Bounds(lower=1.0, upper=10.0), + Bounds(lower=15.0, upper=20.0), + ) + assert 7.0 in allowed + assert 12.0 not in allowed + ``` + """ + + bounds: tuple[Bounds, ...] = () + """The normalized bounds: sorted by lower bound and pairwise non-overlapping.""" + + def __post_init__(self) -> None: + """Normalize the bounds by sorting and merging overlapping ones.""" + object.__setattr__(self, "bounds", _sort_and_merge_bounds(self.bounds)) + + def __contains__(self, item: float | None) -> bool: + """Check whether a value is within any bounds of this set. + + Args: + item: The value to check. + + Returns: + Whether `item` is within any bounds of this set. `None` is never + contained, and the empty (unbounded) set contains every value. + """ + if item is None: + return False + if not self.bounds: + return True + position = bisect.bisect_right( + self.bounds, + item, + key=lambda bound: -math.inf if bound.lower is None else bound.lower, + ) + index = position - 1 + return index >= 0 and item in self.bounds[index] + + def __bool__(self) -> bool: + """Return whether this set restricts the accepted values. + + The empty set is the unbounded set: it accepts every value and is + therefore falsy. A set with any bounds is truthy. + + Returns: + Whether this set contains any bounds. + """ + return bool(self.bounds) + + def is_bounded(self) -> bool: + """Return whether this set restricts the accepted values. + + This is the explicit spelling of this set's truthiness: the empty + (unbounded) set is not bounded, while a set with any bounds is. + + Returns: + Whether this set contains any bounds. + """ + return bool(self) + + def __str__(self) -> str: + """Return a string representation of this set.""" + if not self.bounds: + return "[None,None]" + return "∪".join(str(bound) for bound in self.bounds) diff --git a/tests/metrics/_bounds/test_bounds_set.py b/tests/metrics/_bounds/test_bounds_set.py new file mode 100644 index 00000000..1c63a78e --- /dev/null +++ b/tests/metrics/_bounds/test_bounds_set.py @@ -0,0 +1,164 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `BoundsSet`.""" + +import pytest + +from frequenz.client.common.metrics import Bounds, BoundsSet + + +def test_empty() -> None: + """The empty set is the unbounded set: falsy and contains everything.""" + empty = BoundsSet() + assert not empty.bounds + assert not empty + assert not empty.is_bounded() + assert 0.0 in empty + assert 1e9 in empty + assert -1e9 in empty + assert None not in empty + assert str(empty) == "[None,None]" + + +def test_default_is_empty() -> None: + """`BoundsSet()` and `BoundsSet(bounds=())` are equal empty sets.""" + assert BoundsSet() == BoundsSet(bounds=()) + + +def test_single() -> None: + """A single bound is kept as-is and is bounded.""" + single = BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) + assert single.bounds == (Bounds(lower=1.0, upper=5.0),) + assert single + assert single.is_bounded() + + +def test_disjoint_kept_sorted() -> None: + """Non-overlapping bounds are kept as separate, sorted members.""" + result = BoundsSet( + bounds=(Bounds(lower=15.0, upper=20.0), Bounds(lower=1.0, upper=5.0)) + ) + assert result.bounds == ( + Bounds(lower=1.0, upper=5.0), + Bounds(lower=15.0, upper=20.0), + ) + + +def test_overlapping_merged() -> None: + """Overlapping bounds are merged into one.""" + result = BoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=3.0, upper=10.0)) + ) + assert result.bounds == (Bounds(lower=1.0, upper=10.0),) + + +def test_touching_merged() -> None: + """Bounds sharing an endpoint touch (inclusive) and merge.""" + result = BoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=5.0, upper=10.0)) + ) + assert result.bounds == (Bounds(lower=1.0, upper=10.0),) + + +def test_gap_not_merged() -> None: + """Bounds separated by a gap are not merged.""" + result = BoundsSet( + bounds=(Bounds(lower=1.0, upper=4.0), Bounds(lower=5.0, upper=10.0)) + ) + assert result.bounds == ( + Bounds(lower=1.0, upper=4.0), + Bounds(lower=5.0, upper=10.0), + ) + + +def test_containment_merged() -> None: + """A bound contained in another is absorbed.""" + result = BoundsSet( + bounds=(Bounds(lower=1.0, upper=10.0), Bounds(lower=3.0, upper=5.0)) + ) + assert result.bounds == (Bounds(lower=1.0, upper=10.0),) + + +def test_duplicate_merged() -> None: + """Duplicate bounds collapse to a single member.""" + result = BoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=1.0, upper=5.0)) + ) + assert result.bounds == (Bounds(lower=1.0, upper=5.0),) + + +def test_unbounded_below_merge() -> None: + """A `None` lower bound is treated as -inf when merging.""" + result = BoundsSet( + bounds=(Bounds(lower=None, upper=5.0), Bounds(lower=3.0, upper=8.0)) + ) + assert result.bounds == (Bounds(lower=None, upper=8.0),) + + +def test_all_covering_single_collapses_to_empty() -> None: + """An explicit fully-unbounded bound collapses to the empty set.""" + result = BoundsSet(bounds=(Bounds(lower=None, upper=None),)) + assert not result.bounds + assert not result + + +def test_all_covering_halves_collapse_to_empty() -> None: + """Two half-bounds that together cover the space collapse to the empty set.""" + result = BoundsSet( + bounds=(Bounds(lower=None, upper=5.0), Bounds(lower=3.0, upper=None)) + ) + assert not result.bounds + assert not result + assert 42.0 in result # unbounded -> contains everything + + +@pytest.mark.parametrize( + "item, expected", + [ + (0.0, False), + (1.0, True), # lower bound is inclusive + (5.0, True), # upper bound is inclusive + (3.0, True), + (6.0, False), # in the gap between the two bounds + (15.0, True), + (20.0, True), + (21.0, False), + ], +) +def test_contains(item: float, expected: bool) -> None: + """Membership tests the union of all bounds, inclusive on both ends.""" + bounds_set = BoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=15.0, upper=20.0)) + ) + assert (item in bounds_set) is expected + + +def test_contains_none() -> None: + """`None` is never contained, not even by the unbounded set.""" + assert None not in BoundsSet() + assert None not in BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) + + +def test_str() -> None: + """The string form joins members with a union symbol.""" + bounds_set = BoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=15.0, upper=20.0)) + ) + assert str(bounds_set) == "[1.0,5.0]∪[15.0,20.0]" + + +def test_equality_on_normalized_form() -> None: + """Sets built from different inputs that normalize equal are equal and hash equal.""" + a = BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=3.0, upper=10.0))) + b = BoundsSet(bounds=(Bounds(lower=1.0, upper=10.0),)) + assert a == b + assert hash(a) == hash(b) + + +def test_hashable() -> None: + """`BoundsSet` can be used in sets and as dict keys.""" + a = BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) + b = BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) + c = BoundsSet(bounds=(Bounds(lower=6.0, upper=8.0),)) + assert len({a, b, c}) == 2 From afe3cdb885b9c969458d7a628537cfc046d55b32 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:56:42 +0000 Subject: [PATCH 04/25] Add `InvalidBoundsSet` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the set-level counterpart to `InvalidBounds`, mirroring the `Bounds` / `InvalidBounds` split one level up. A collection of bounds that contains any `InvalidBounds` cannot be normalized into a well-formed `BoundsSet` — malformed ranges have no meaningful sort or merge order — so it is preserved as an `InvalidBoundsSet` instead of silently dropping the bad entries. The type keeps all of the raw bounds (valid and invalid alike) in their original wire order, with no normalization, so callers can inspect exactly what was received. It deliberately provides no membership test: range-checking malformed data is exactly the mistake this type exists to prevent. It is truthy by default, since malformed data is not the same as "unbounded" — only an empty `BoundsSet` means unbounded. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 10 +++- src/frequenz/client/common/metrics/_bounds.py | 26 +++++++++ .../_bounds/test_invalid_bounds_set.py | 55 +++++++++++++++++++ 3 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 tests/metrics/_bounds/test_invalid_bounds_set.py diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index cad245d2..1fd8b786 100644 --- a/src/frequenz/client/common/metrics/__init__.py +++ b/src/frequenz/client/common/metrics/__init__.py @@ -3,7 +3,14 @@ """Metrics definitions.""" -from ._bounds import BaseBounds, Bounds, BoundsSet, InvalidBounds, InvalidBoundsError +from ._bounds import ( + BaseBounds, + Bounds, + BoundsSet, + InvalidBounds, + InvalidBoundsError, + InvalidBoundsSet, +) from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -21,6 +28,7 @@ "BoundsSet", "InvalidBounds", "InvalidBoundsError", + "InvalidBoundsSet", "Metric", "MetricConnection", "MetricConnectionCategory", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 0ae0a8a4..292abc1b 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -355,3 +355,29 @@ def __str__(self) -> str: if not self.bounds: return "[None,None]" return "∪".join(str(bound) for bound in self.bounds) + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class InvalidBoundsSet: + """A set of metric bounds built from at least one malformed bound. + + When a collection of bounds contains any [`InvalidBounds`][..InvalidBounds] + it cannot be normalized into a well-formed [`BoundsSet`][..BoundsSet]: + malformed ranges cannot be meaningfully sorted or merged. This type + preserves all of the raw bounds — valid and invalid alike — in their + original order, so callers can inspect exactly what was received without + accidentally range-checking against broken data. + + Unlike [`BoundsSet`][..BoundsSet], this type intentionally provides no + membership test: malformed bounds must not be used for range checks. Use a + semantic accessor, such as `MetricSample.get_bounds_set()`, to receive a + clear error on invalid data. + """ + + bounds: tuple[Bounds | InvalidBounds, ...] = () + """The raw bounds, preserved in their original order without merging.""" + + def __str__(self) -> str: + """Return a compact string representation of this invalid set.""" + inner = "∪".join(str(bound) for bound in self.bounds) + return f"" diff --git a/tests/metrics/_bounds/test_invalid_bounds_set.py b/tests/metrics/_bounds/test_invalid_bounds_set.py new file mode 100644 index 00000000..f5a9b197 --- /dev/null +++ b/tests/metrics/_bounds/test_invalid_bounds_set.py @@ -0,0 +1,55 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `InvalidBoundsSet`.""" + +from frequenz.client.common.metrics import ( + Bounds, + BoundsSet, + InvalidBounds, + InvalidBoundsSet, +) + + +def test_preserves_raw_bounds_unmerged() -> None: + """All bounds are preserved in order, without sorting or merging.""" + raw = ( + Bounds(lower=5.0, upper=10.0), + InvalidBounds(lower=10.0, upper=-10.0), + Bounds(lower=1.0, upper=3.0), + ) + invalid = InvalidBoundsSet(bounds=raw) + assert invalid.bounds == raw + + +def test_is_not_bounds_set_subclass() -> None: + """`InvalidBoundsSet` is a sibling of `BoundsSet`, not a subclass.""" + assert not issubclass(InvalidBoundsSet, BoundsSet) + + +def test_no_membership_test() -> None: + """`InvalidBoundsSet` provides no membership test for malformed data.""" + invalid = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + assert not hasattr(invalid, "__contains__") + + +def test_truthy() -> None: + """An invalid set is truthy: it is malformed, not "unbounded".""" + invalid = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + assert invalid + + +def test_str() -> None: + """`__str__` wraps the members in the `` marker.""" + invalid = InvalidBoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), InvalidBounds(lower=10.0, upper=-10.0)) + ) + assert str(invalid) == ">" + + +def test_equality() -> None: + """Two invalid sets with the same raw bounds are equal and hash equal.""" + a = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + b = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + assert a == b + assert hash(a) == hash(b) From 813eb9b05c0f42f281a3e842b561e48f30aa466e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:09:48 +0000 Subject: [PATCH 05/25] Migrate `MetricSample.bounds` to `bounds_set` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace `MetricSample.bounds: list[Bounds]` with `bounds_set: BoundsSet | InvalidBoundsSet`. The metric bounds are a union of ranges, so a normalized `BoundsSet` models them better than a raw list, and — mirroring `bounds_from_proto2` returning `Bounds | InvalidBounds` — malformed wire data is now preserved as an `InvalidBoundsSet` instead of being silently dropped. `bounds` is kept as deprecated for backwards-compatibility. A hand-written `__init__` still accepts the deprecated `bounds=` keyword (emitting a `DeprecationWarning` and building a `BoundsSet` from it), and a deprecated read-only `bounds` property returns the valid `Bounds` from `bounds_set`, so both existing readers and constructors keep working (with warnings). `init=False` plus a manual `__init__` is required because an `InitVar` named `bounds` would collide with the `bounds` property. We could have named the new field `bounds2`, but we decided for `bounds_set` instead: it is a permanent, self-documenting name, so the next minor just drops the deprecated `bounds` property and parameter and leaves `bounds_set` in place — no rename needed. Since the new field is not a 1-1 translation of the protocol message (it goes through normalization) it makes sense to have a different name than the protobuf field. The compatibility property returns only the valid bounds (dropping malformed ones, as the old field effectively did) but the merged, normalized bounds rather than the raw wire list; that small divergence is acceptable for a deprecated shim. On the proto side `_metric_bounds_from_proto` becomes `_bounds_set_from_proto`, returning `BoundsSet | InvalidBoundsSet`. It no longer needs the `metric` argument or the `major_issues` / `minor_issues` side channels: validity is encoded in the returned type (as done for `metric_config_bounds`), so the "bounds ... is invalid, ignoring these bounds" major issue is gone and the invalid bounds are preserved instead of dropped. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 95 +++++++++++++++---- .../common/metrics/proto/v1alpha8/_sample.py | 43 ++++----- .../v1alpha8/test_sample_metric_sample.py | 32 ++++--- tests/metrics/test_sample_metric_sample.py | 87 ++++++++++++++--- 4 files changed, 185 insertions(+), 72 deletions(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index 71dfb515..13e8a55c 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -10,9 +10,10 @@ from typing import assert_never from frequenz.core.enum import Enum, deprecated_member, unique +from typing_extensions import deprecated from .._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError -from ._bounds import Bounds +from ._bounds import Bounds, BoundsSet, InvalidBoundsSet from ._metric import Metric @@ -166,7 +167,7 @@ def get_category(self) -> MetricConnectionCategory: assert_never(unexpected) -@dataclass(frozen=True, kw_only=True) +@dataclass(frozen=True, init=False) class MetricSample: """A sampled metric. @@ -195,33 +196,22 @@ class MetricSample: value: float | AggregatedMetricValue | None """The value of the sampled metric.""" - bounds: list[Bounds] + bounds_set: BoundsSet | InvalidBoundsSet """The bounds that apply to the metric sample. These bounds adapt in real-time to reflect the operating conditions at the time of - aggregation or derivation. + aggregation or derivation. They form a union: the value of the metric must be within + at least one of them, and an empty [`BoundsSet`][...BoundsSet] means the metric is + unbounded. - In the case of certain components like batteries, multiple bounds might exist. These - multiple bounds collectively extend the range of allowable values, effectively - forming a union of all given bounds. In such cases, the value of the metric must be - within at least one of the bounds. + This is a [`BoundsSet`][...BoundsSet] for well-formed data, or an + [`InvalidBoundsSet`][...InvalidBoundsSet] preserving the raw bounds when the wire + carried any malformed entry, so callers must handle both. In accordance with the passive sign convention, bounds that limit discharge would have negative numbers, while those limiting charge, such as for the State of Power (SoP) metric, would be positive. Hence bounds can have positive and negative values depending on the metric they represent. - - Example: - The diagram below illustrates the relationship between the bounds. - - ``` - bound[0].lower bound[1].upper - <-------|============|------------------|============|---------> - bound[0].upper bound[1].lower - - ---- values here are disallowed and will be rejected - ==== values here are allowed and will be accepted - ``` """ connection: MetricConnection | None = None @@ -243,6 +233,71 @@ class MetricSample: sampled from is important. """ + # This custom `__init__` should be removed once the deprecated `bounds` field is removed. + # pylint: disable-next=too-many-arguments + def __init__( + self, + *, + sample_time: datetime, + metric: Metric | int, + value: float | AggregatedMetricValue | None, + bounds_set: BoundsSet | InvalidBoundsSet | None = None, + bounds: list[Bounds] | None = None, + connection: MetricConnection | None = None, + ) -> None: + """Initialize this metric sample. + + Args: + sample_time: The moment when the metric was sampled. + metric: The metric that was sampled. + value: The value of the sampled metric. + bounds_set: The bounds that apply to the metric sample. + bounds: Deprecated alias that accepts a list of valid + [`Bounds`][...Bounds] and stores them as a + [`BoundsSet`][...BoundsSet]. Use `bounds_set` instead. + connection: The source or connection the metric was sampled from. + + Raises: + TypeError: If both `bounds_set` and the deprecated `bounds` are + given, or if neither is given. + """ + if bounds is not None and bounds_set is not None: + raise TypeError( + "`MetricSample` accepts either `bounds_set` or the deprecated " + "`bounds`, not both." + ) + if bounds is not None: + warnings.warn( + "The `bounds` argument is deprecated; use `bounds_set` instead.", + DeprecationWarning, + stacklevel=2, + ) + bounds_set = BoundsSet(bounds=tuple(bounds)) + if bounds_set is None: + raise TypeError("`MetricSample` requires the `bounds_set` argument.") + object.__setattr__(self, "sample_time", sample_time) + object.__setattr__(self, "metric", metric) + object.__setattr__(self, "value", value) + object.__setattr__(self, "bounds_set", bounds_set) + object.__setattr__(self, "connection", connection) + + @property + @deprecated("`MetricSample.bounds` is deprecated; use `bounds_set` instead.") + def bounds(self) -> list[Bounds]: + """The valid bounds that apply to the metric sample. + + Deprecated: + Use `bounds_set` instead. For backward compatibility this returns + only the valid [`Bounds`][...Bounds] from `bounds_set` (dropping any + malformed entries, as the old field did), but it returns the + normalized, merged bounds rather than the raw list received on the + wire. + + Returns: + The valid bounds in `bounds_set`. + """ + return [bound for bound in self.bounds_set.bounds if isinstance(bound, Bounds)] + def as_single_value( self, *, aggregation_method: AggregationMethod = AggregationMethod.AVG ) -> float | None: diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py index 5eaef157..6b8a28cc 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py @@ -9,7 +9,7 @@ from frequenz.api.common.v1alpha8.metrics import bounds_pb2, metrics_pb2 from ....proto import datetime_from_proto -from ..._bounds import Bounds, InvalidBounds +from ..._bounds import Bounds, BoundsSet, InvalidBounds, InvalidBoundsSet from ..._metric import Metric from ..._sample import ( AggregatedMetricValue, @@ -106,9 +106,7 @@ def metric_sample_from_proto_with_issues( message.value.aggregated_metric ) - bounds = _metric_bounds_from_proto( - metric, message.bounds, major_issues=major_issues, minor_issues=minor_issues - ) + bounds_set = _bounds_set_from_proto(message.bounds) connection = None if message.HasField("connection"): @@ -120,41 +118,38 @@ def metric_sample_from_proto_with_issues( sample_time=sample_time, metric=metric, value=value, - bounds=bounds, + bounds_set=bounds_set, connection=connection, ) -def _metric_bounds_from_proto( - metric: Metric | int, +def _bounds_set_from_proto( messages: Sequence[bounds_pb2.Bounds], - *, - major_issues: list[str], - minor_issues: list[str], # pylint:disable=unused-argument -) -> list[Bounds]: - """Convert a sequence of bounds messages to a list of [`Bounds`][....Bounds]. +) -> BoundsSet | InvalidBoundsSet: + """Convert a sequence of bounds messages to a bounds set. Args: - metric: The metric for which the bounds are defined, used for logging issues. messages: The sequence of bounds messages. - major_issues: A list to append major issues to. - minor_issues: A list to append minor issues to. Returns: - The resulting list of [`Bounds`][....Bounds]. + A [`BoundsSet`][....BoundsSet] when every bound is well-formed, or an + [`InvalidBoundsSet`][....InvalidBoundsSet] preserving all the raw + bounds when any bound is malformed. """ - bounds: list[Bounds] = [] + valid: list[Bounds] = [] + raw: list[Bounds | InvalidBounds] = [] + has_invalid = False for pb_bound in messages: match bounds_from_proto2(pb_bound): case Bounds() as bound: - bounds.append(bound) + valid.append(bound) + raw.append(bound) case InvalidBounds() as bound: - metric_name = metric if isinstance(metric, int) else metric.name - major_issues.append( - f"bounds for {metric_name} is invalid ({bound}), " - "ignoring these bounds" - ) + has_invalid = True + raw.append(bound) case unknown: assert_never(unknown) - return bounds + if has_invalid: + return InvalidBoundsSet(bounds=tuple(raw)) + return BoundsSet(bounds=tuple(valid)) diff --git a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py index b17a1aa2..3415bf0a 100644 --- a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py +++ b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py @@ -15,6 +15,9 @@ from frequenz.client.common.metrics import ( AggregatedMetricValue, Bounds, + BoundsSet, + InvalidBounds, + InvalidBoundsSet, Metric, MetricConnection, MetricConnectionCategory, @@ -66,7 +69,7 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=5.0, - bounds=[], + bounds_set=BoundsSet(), connection=None, ), ), @@ -85,7 +88,7 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=AggregatedMetricValue(avg=5.0, min=1.0, max=10.0, raw=[]), - bounds=[], + bounds_set=BoundsSet(), connection=None, ), ), @@ -99,7 +102,7 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=None, - bounds=[], + bounds_set=BoundsSet(), connection=None, ), ), @@ -113,7 +116,11 @@ class _TestCase: ), ), expected_sample=MetricSample( - sample_time=DATETIME, metric=999, value=5.0, bounds=[], connection=None + sample_time=DATETIME, + metric=999, + value=5.0, + bounds_set=BoundsSet(), + connection=None, ), ), _TestCase( @@ -130,7 +137,7 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=5.0, - bounds=[Bounds(lower=-10.0, upper=10.0)], + bounds_set=BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)), connection=None, ), ), @@ -151,15 +158,14 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=5.0, - bounds=[Bounds(lower=-10.0, upper=10.0)], # Invalid bounds are ignored + bounds_set=InvalidBoundsSet( + bounds=( + Bounds(lower=-10.0, upper=10.0), + InvalidBounds(lower=10.0, upper=-10.0), + ) + ), connection=None, ), - expected_major_issues=[ - ( - "bounds for AC_POWER_ACTIVE is invalid (), " - "ignoring these bounds" - ) - ], ), _TestCase( name="with_connection", @@ -180,7 +186,7 @@ class _TestCase: sample_time=DATETIME, metric=Metric.AC_POWER_ACTIVE, value=5.0, - bounds=[], + bounds_set=BoundsSet(), connection=MetricConnection( category=MetricConnectionCategory.BATTERY, name="dc_battery_0" ), diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index 918c1527..86293598 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -15,6 +15,7 @@ AggregatedMetricValue, AggregationMethod, Bounds, + BoundsSet, Metric, MetricConnection, MetricSample, @@ -58,18 +59,18 @@ def test_creation( connection: MetricConnection | None, ) -> None: """Test MetricSample creation with different value types.""" - bounds = [Bounds(lower=-10.0, upper=10.0)] + bounds_set = BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)) sample = MetricSample( sample_time=now, metric=Metric.AC_POWER_ACTIVE, value=value, - bounds=bounds, + bounds_set=bounds_set, connection=connection, ) assert sample.sample_time == now assert sample.metric == Metric.AC_POWER_ACTIVE assert sample.value == value - assert sample.bounds == bounds + assert sample.bounds_set == bounds_set assert sample.connection == connection @@ -116,13 +117,13 @@ def test_as_single_value( method_results: dict[AggregationMethod, float | None], ) -> None: """Test MetricSample.as_single_value with different value types and methods.""" - bounds = [Bounds(lower=-10.0, upper=10.0)] + bounds_set = BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)) sample = MetricSample( sample_time=now, metric=Metric.AC_POWER_ACTIVE, value=value, - bounds=bounds, + bounds_set=bounds_set, ) for method, expected in method_results.items(): @@ -131,30 +132,81 @@ def test_as_single_value( def test_multiple_bounds(now: datetime) -> None: """Test MetricSample creation with multiple bounds.""" - bounds = [ - Bounds(lower=-10.0, upper=-5.0), - Bounds(lower=5.0, upper=10.0), - ] + bounds_set = BoundsSet( + bounds=( + Bounds(lower=-10.0, upper=-5.0), + Bounds(lower=5.0, upper=10.0), + ) + ) sample = MetricSample( sample_time=now, metric=Metric.AC_POWER_ACTIVE, value=7.0, - bounds=bounds, + bounds_set=bounds_set, + ) + assert sample.bounds_set == bounds_set + + +def test_deprecated_bounds_kwarg(now: datetime) -> None: + """The deprecated `bounds` argument builds a `BoundsSet` and warns.""" + with pytest.deprecated_call(): + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds=[Bounds(lower=-10.0, upper=10.0)], + ) + assert sample.bounds_set == BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)) + + +def test_deprecated_bounds_property(now: datetime) -> None: + """The deprecated `bounds` property returns the valid bounds and warns.""" + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)), ) - assert sample.bounds == bounds + with pytest.deprecated_call(): + assert sample.bounds == [Bounds(lower=-10.0, upper=10.0)] + + +def test_bounds_and_bounds_set_raises(now: datetime) -> None: + """Passing both `bounds` and `bounds_set` raises `TypeError`.""" + with pytest.raises(TypeError, match="not both"): + MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds=[Bounds(lower=-10.0, upper=10.0)], + bounds_set=BoundsSet(), + ) + + +def test_missing_bounds_set_raises(now: datetime) -> None: + """Passing neither `bounds` nor `bounds_set` raises `TypeError`.""" + with pytest.raises(TypeError, match="requires the"): + MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + ) def test_get_metric_returns_known_member(now: datetime) -> None: """get_metric returns the metric when it is a known member.""" sample = MetricSample( - sample_time=now, metric=Metric.AC_POWER_ACTIVE, value=None, bounds=[] + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=None, + bounds_set=BoundsSet(), ) assert sample.get_metric() is Metric.AC_POWER_ACTIVE def test_get_metric_unspecified_int_raises(now: datetime) -> None: """get_metric raises UnspecifiedEnumValueError for the raw int 0.""" - sample = MetricSample(sample_time=now, metric=0, value=None, bounds=[]) + sample = MetricSample(sample_time=now, metric=0, value=None, bounds_set=BoundsSet()) with pytest.raises(UnspecifiedEnumValueError): sample.get_metric() @@ -163,7 +215,10 @@ def test_get_metric_unspecified_member_raises(now: datetime) -> None: """get_metric raises UnspecifiedEnumValueError for the value-0 member.""" with pytest.deprecated_call(): sample = MetricSample( - sample_time=now, metric=Metric.UNSPECIFIED, value=None, bounds=[] + sample_time=now, + metric=Metric.UNSPECIFIED, + value=None, + bounds_set=BoundsSet(), ) with pytest.raises(UnspecifiedEnumValueError): sample.get_metric() @@ -171,7 +226,9 @@ def test_get_metric_unspecified_member_raises(now: datetime) -> None: def test_get_metric_unrecognized_int_raises(now: datetime) -> None: """get_metric raises UnrecognizedEnumValueError carrying the raw int value.""" - sample = MetricSample(sample_time=now, metric=99999, value=None, bounds=[]) + sample = MetricSample( + sample_time=now, metric=99999, value=None, bounds_set=BoundsSet() + ) with pytest.raises(UnrecognizedEnumValueError) as exc_info: sample.get_metric() assert exc_info.value.value == 99999 From ab984c751f74cf485bdd9e909a688d09f2c78dbb Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:12:02 +0000 Subject: [PATCH 06/25] Add `InvalidBoundsSetError` Add the set-level counterpart to `InvalidBoundsError`, for a semantic accessor that resolves a `BoundsSet | InvalidBoundsSet` field to a valid `BoundsSet` and instead sees an `InvalidBoundsSet`. It carries the offending set on its `bounds_set` attribute so callers can inspect the raw wire data, and is an `InvalidAttributeError` (hence also a `ValueError`) like the rest of the accessor errors. This is the error `MetricSample.get_bounds_set()` will raise; it is added on its own first. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 2 + src/frequenz/client/common/metrics/_bounds.py | 40 +++++++++++++++ .../_bounds/test_invalid_bounds_set_error.py | 50 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 tests/metrics/_bounds/test_invalid_bounds_set_error.py diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index 1fd8b786..66a437f6 100644 --- a/src/frequenz/client/common/metrics/__init__.py +++ b/src/frequenz/client/common/metrics/__init__.py @@ -10,6 +10,7 @@ InvalidBounds, InvalidBoundsError, InvalidBoundsSet, + InvalidBoundsSetError, ) from ._metric import Metric from ._sample import ( @@ -29,6 +30,7 @@ "InvalidBounds", "InvalidBoundsError", "InvalidBoundsSet", + "InvalidBoundsSetError", "Metric", "MetricConnection", "MetricConnectionCategory", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 292abc1b..c21c0e2a 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -381,3 +381,43 @@ def __str__(self) -> str: """Return a compact string representation of this invalid set.""" inner = "∪".join(str(bound) for bound in self.bounds) return f"" + + +class InvalidBoundsSetError(InvalidAttributeError): + """Raised when a semantic accessor sees an invalid bounds set. + + The offending [`InvalidBoundsSet`][..InvalidBoundsSet] is available as the + [`bounds_set`][.bounds_set] attribute so callers can inspect the raw wire + data. + + This is also a [`ValueError`][] for convenience. + """ + + def __init__( + self, + instance: object, + attr_name: str, + bounds_set: InvalidBoundsSet, + message: str | None = None, + ) -> None: + """Initialize this error. + + Args: + instance: The instance that was being accessed when this error was raised. + attr_name: The name of the attribute that was being accessed. + bounds_set: The invalid bounds set instance. + message: A custom error message. If `None`, a default message mentioning + the invalid bounds set is used. + """ + self.bounds_set: InvalidBoundsSet = bounds_set + """The invalid bounds set that caused this error.""" + + super().__init__( + instance, + attr_name, + ( + message + if message is not None + else f"invalid bounds set {bounds_set!r} for attribute {attr_name!r} in {instance}" + ), + ) diff --git a/tests/metrics/_bounds/test_invalid_bounds_set_error.py b/tests/metrics/_bounds/test_invalid_bounds_set_error.py new file mode 100644 index 00000000..6f44eac4 --- /dev/null +++ b/tests/metrics/_bounds/test_invalid_bounds_set_error.py @@ -0,0 +1,50 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `InvalidBoundsSetError`.""" + +from frequenz.client.common import InvalidAttributeError +from frequenz.client.common.metrics import ( + Bounds, + InvalidBounds, + InvalidBoundsSet, + InvalidBoundsSetError, +) + + +def test_default_message() -> None: + """`InvalidBoundsSetError` builds a default message from the invalid set.""" + invalid = InvalidBoundsSet( + bounds=(Bounds(lower=1.0, upper=5.0), InvalidBounds(lower=10.0, upper=-10.0)) + ) + error = InvalidBoundsSetError("some-instance", "bounds_set", invalid) + + assert error.bounds_set is invalid + assert ( + str(error) == f"invalid bounds set {invalid!r} for attribute 'bounds_set' " + "in some-instance" + ) + + +def test_custom_message() -> None: + """`InvalidBoundsSetError` accepts a custom message.""" + invalid = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + error = InvalidBoundsSetError( + "some-instance", + "bounds_set", + invalid, + message="bad bounds set from server", + ) + + assert error.bounds_set is invalid + assert str(error) == "bad bounds set from server" + + +def test_is_invalid_attribute_error() -> None: + """`InvalidBoundsSetError` is an `InvalidAttributeError`.""" + assert issubclass(InvalidBoundsSetError, InvalidAttributeError) + + +def test_is_value_error() -> None: + """`InvalidBoundsSetError` is a `ValueError`.""" + assert issubclass(InvalidBoundsSetError, ValueError) From 6bdba6eadedc90aadf827296d3b42e3cdfc9f844 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:15:35 +0000 Subject: [PATCH 07/25] Add safe accessor `MetricSample.get_bounds_set()` `MetricSample.bounds_set` is the lower-level, forward-compatible field: callers reading it must narrow the `BoundsSet | InvalidBoundsSet` union themselves on every access, which is easy to get wrong and easy to skip. Add a higher-level accessor that does the narrowing once: `get_bounds_set()` returns the `BoundsSet` unchanged for well-formed data and turns an `InvalidBoundsSet` into an `InvalidBoundsSetError`, whose `bounds_set` attribute preserves the raw set for inspection. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 30 ++++++++++++++++++- tests/metrics/test_sample_metric_sample.py | 29 ++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index 13e8a55c..ef519516 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -13,7 +13,7 @@ from typing_extensions import deprecated from .._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError -from ._bounds import Bounds, BoundsSet, InvalidBoundsSet +from ._bounds import Bounds, BoundsSet, InvalidBoundsSet, InvalidBoundsSetError from ._metric import Metric @@ -208,6 +208,10 @@ class MetricSample: [`InvalidBoundsSet`][...InvalidBoundsSet] preserving the raw bounds when the wire carried any malformed entry, so callers must handle both. + Tip: + Prefer `MetricSample.get_bounds_set()` to obtain a valid `BoundsSet` or a + clear error. + In accordance with the passive sign convention, bounds that limit discharge would have negative numbers, while those limiting charge, such as for the State of Power (SoP) metric, would be positive. Hence bounds can have positive and negative values @@ -361,3 +365,27 @@ def get_metric(self) -> Metric: raise UnrecognizedEnumValueError(self, "metric", self.metric) case unexpected: assert_never(unexpected) + + def get_bounds_set(self) -> BoundsSet: + """Return the bounds as a valid `BoundsSet`. + + This is the higher-level accessor for the lower-level + [`bounds_set`][frequenz.client.common.metrics.MetricSample.bounds_set] + field: it returns a valid `BoundsSet` or raises instead of exposing an + `InvalidBoundsSet`. + + Returns: + The bounds set when it is a valid `BoundsSet`. + + Raises: + InvalidBoundsSetError: If the bounds set is an `InvalidBoundsSet`. + The offending set is available on the error's `bounds_set` + attribute. + """ + match self.bounds_set: + case BoundsSet() as bounds_set: + return bounds_set + case InvalidBoundsSet() as invalid: + raise InvalidBoundsSetError(self, "bounds_set", invalid) + case unexpected: + assert_never(unexpected) diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index 86293598..4a5a93a1 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -16,6 +16,9 @@ AggregationMethod, Bounds, BoundsSet, + InvalidBounds, + InvalidBoundsSet, + InvalidBoundsSetError, Metric, MetricConnection, MetricSample, @@ -232,3 +235,29 @@ def test_get_metric_unrecognized_int_raises(now: datetime) -> None: with pytest.raises(UnrecognizedEnumValueError) as exc_info: sample.get_metric() assert exc_info.value.value == 99999 + + +def test_get_bounds_set_returns_valid(now: datetime) -> None: + """get_bounds_set returns the set when it is a valid BoundsSet.""" + bounds_set = BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)) + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=bounds_set, + ) + assert sample.get_bounds_set() is bounds_set + + +def test_get_bounds_set_invalid_raises(now: datetime) -> None: + """get_bounds_set raises InvalidBoundsSetError for an InvalidBoundsSet.""" + invalid = InvalidBoundsSet(bounds=(InvalidBounds(lower=10.0, upper=-10.0),)) + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=invalid, + ) + with pytest.raises(InvalidBoundsSetError) as exc_info: + sample.get_bounds_set() + assert exc_info.value.bounds_set is invalid From 1b2d85613c01d10dd8e783654a62df5c45c65aef Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:29:18 +0000 Subject: [PATCH 08/25] Reject `NaN` in bounds membership tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `float("nan")` compares `False` against everything, so `nan < lower` and `nan > upper` are both `False` and the membership checks fell through to `True`: `nan in Bounds(1, 5)` and `nan in a_bounds_set` both reported the value as contained. For metric bounds that is a real hazard — a `NaN` sample would be silently treated as within range. Guard both `Bounds.__contains__` and `BoundsSet.__contains__` with `math.isnan()` so `NaN` is never contained, matching how `None` is already rejected. `math` is already imported for the bisect key. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 4 ++-- tests/metrics/_bounds/test_bounds.py | 7 +++++++ tests/metrics/_bounds/test_bounds_set.py | 8 ++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index c21c0e2a..3cd812e9 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -85,7 +85,7 @@ def __contains__(self, item: float | None) -> bool: Returns: Whether `item` is within these bounds. """ - if item is None: + if item is None or math.isnan(item): return False if self.lower is not None and item < self.lower: return False @@ -316,7 +316,7 @@ def __contains__(self, item: float | None) -> bool: Whether `item` is within any bounds of this set. `None` is never contained, and the empty (unbounded) set contains every value. """ - if item is None: + if item is None or math.isnan(item): return False if not self.bounds: return True diff --git a/tests/metrics/_bounds/test_bounds.py b/tests/metrics/_bounds/test_bounds.py index 2284a9cc..6d38cdb0 100644 --- a/tests/metrics/_bounds/test_bounds.py +++ b/tests/metrics/_bounds/test_bounds.py @@ -3,6 +3,7 @@ """Tests for `Bounds`.""" +import math import re import pytest @@ -108,6 +109,12 @@ def test_contains_none() -> None: assert None not in Bounds(lower=-10.0, upper=10.0) +def test_contains_nan() -> None: + """`NaN` is never contained, even by unbounded bounds.""" + assert math.nan not in Bounds() + assert math.nan not in Bounds(lower=-10.0, upper=10.0) + + @pytest.mark.parametrize( "lower, upper, expected", [ diff --git a/tests/metrics/_bounds/test_bounds_set.py b/tests/metrics/_bounds/test_bounds_set.py index 1c63a78e..c44c6670 100644 --- a/tests/metrics/_bounds/test_bounds_set.py +++ b/tests/metrics/_bounds/test_bounds_set.py @@ -3,6 +3,8 @@ """Tests for `BoundsSet`.""" +import math + import pytest from frequenz.client.common.metrics import Bounds, BoundsSet @@ -140,6 +142,12 @@ def test_contains_none() -> None: assert None not in BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) +def test_contains_nan() -> None: + """`NaN` is never contained, not even by the unbounded set.""" + assert math.nan not in BoundsSet() + assert math.nan not in BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) + + def test_str() -> None: """The string form joins members with a union symbol.""" bounds_set = BoundsSet( From ec64f69298da1584cc555e5c9077c62ed96e798a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:31:21 +0000 Subject: [PATCH 09/25] Normalize the deprecated `MetricSample.bounds` property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The property's docstring promises normalized, merged bounds, but it only delivered that for a `BoundsSet` (whose `bounds` are already normalized). For an `InvalidBoundsSet` — whose `bounds` are the raw, unmerged wire data — it returned the valid entries unmerged, so the same property behaved inconsistently depending on the arm of the union. Run the valid entries through `BoundsSet` in both cases, so the deprecated property always returns the merged, normalized bounds it documents, regardless of whether the underlying set is valid. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 5 ++++- tests/metrics/test_sample_metric_sample.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index ef519516..fecebf67 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -300,7 +300,10 @@ def bounds(self) -> list[Bounds]: Returns: The valid bounds in `bounds_set`. """ - return [bound for bound in self.bounds_set.bounds if isinstance(bound, Bounds)] + valid = tuple( + bound for bound in self.bounds_set.bounds if isinstance(bound, Bounds) + ) + return list(BoundsSet(bounds=valid).bounds) def as_single_value( self, *, aggregation_method: AggregationMethod = AggregationMethod.AVG diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index 4a5a93a1..e9f42bdb 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -174,6 +174,24 @@ def test_deprecated_bounds_property(now: datetime) -> None: assert sample.bounds == [Bounds(lower=-10.0, upper=10.0)] +def test_deprecated_bounds_property_normalizes_invalid_set(now: datetime) -> None: + """The deprecated `bounds` property returns normalized valid bounds for an invalid set.""" + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=InvalidBoundsSet( + bounds=( + Bounds(lower=1.0, upper=5.0), + Bounds(lower=3.0, upper=8.0), # overlaps the previous -> merged + InvalidBounds(lower=10.0, upper=-10.0), # dropped + ) + ), + ) + with pytest.deprecated_call(): + assert sample.bounds == [Bounds(lower=1.0, upper=8.0)] + + def test_bounds_and_bounds_set_raises(now: datetime) -> None: """Passing both `bounds` and `bounds_set` raises `TypeError`.""" with pytest.raises(TypeError, match="not both"): From 43ea977821066a9d72b68b3b659d5bf720609478 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:18:43 +0000 Subject: [PATCH 10/25] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 5564d68e..bac25102 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -62,6 +62,14 @@ * `frequenz.client.common.metrics.Bounds.__str__` now renders as `[lower,upper]` (no space after the comma) to match the compact format used by `Lifetime` and to compose cleanly with the `` marker on `InvalidBounds`. +* `frequenz.client.common.metrics.MetricSample.bounds` is now deprecated; use `bounds_set` instead. + + The field type changed from `list[Bounds]` to `BoundsSet | InvalidBoundsSet` (see New Features). Reads and construction remain backward compatible: passing the `bounds=` keyword argument still works (it builds a `BoundsSet` and emits a `DeprecationWarning`), and reading `MetricSample.bounds` still returns the valid `Bounds` as a `list` (also emitting a `DeprecationWarning`). The compatibility property returns only the valid, normalized bounds, so it may differ from the raw wire list when bounds overlapped or touched. + +* `frequenz.client.common.metrics.proto.v1alpha8.metric_sample_from_proto_with_issues` no longer drops invalid bounds or reports them as a major issue. + + Malformed bounds are now preserved in the returned `MetricSample.bounds_set` as an `InvalidBoundsSet` (validity is encoded in the type), so the previous "bounds for ... is invalid, ignoring these bounds" major issue is no longer produced. + ## New Features * Added 4 new electrical component classes for categories that previously collapsed into `UnrecognizedElectricalComponent`: @@ -91,6 +99,7 @@ * `frequenz.client.common.grid.DeliveryArea.get_code_type()` * `frequenz.client.common.metrics.MetricConnection.get_category()` * `frequenz.client.common.metrics.MetricSample.get_metric()` + * `frequenz.client.common.metrics.MetricSample.get_bounds_set()` * `frequenz.client.common.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()` * Added new delivery-area class hierarchy: @@ -105,6 +114,18 @@ * Added `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto2` returning `Bounds | InvalidBounds`. This is the replacement for the now-deprecated `bounds_from_proto`. +* `frequenz.client.common.metrics.Bounds` gained containment check capabilities: + + * `value in bounds` (`__contains__`) tests membership, inclusive on both ends, with a `None` bound meaning unbounded in that direction. + * `bool(bounds)` and `bounds.is_bounded()` report whether the bounds restrict anything; a fully unbounded `Bounds()` is falsy. + +* Added a new bounds-set class hierarchy: + + * `frequenz.client.common.metrics.BoundsSet` — a normalized union of `Bounds` with an efficient `value in bounds_set` membership test. Overlapping and touching bounds are merged on construction, and the empty set is the unbounded set (it contains every value and is falsy). + * `frequenz.client.common.metrics.InvalidBoundsSet` — a set built from bounds that included at least one `InvalidBounds`; it preserves all the raw bounds unmerged and provides no membership test. + +* Added a new `frequenz.client.common.metrics.MetricSample.bounds_set` field, typed `BoundsSet | InvalidBoundsSet`, replacing the deprecated `bounds` list (see Upgrading). Malformed wire bounds are preserved as an `InvalidBoundsSet` instead of being dropped. Use `get_bounds_set()` to resolve it to a valid `BoundsSet` or a clear `InvalidBoundsSetError`. + * Added a new `frequenz.client.common.types.Location` type together with the `frequenz.client.common.types.proto.v1alpha8.location_from_proto` conversion function. * Added a new `frequenz.client.common.microgrid.Microgrid` type, together with the `frequenz.client.common.microgrid.proto.v1alpha8.microgrid_from_proto` conversion function. From cd4ab2659c07bfee1758a93576deb17046dd0f2d Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 11:58:59 +0000 Subject: [PATCH 11/25] Add `FloatInt` type alias MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PEP 484's numeric tower makes `int` assignable to any `float`-annotated parameter or field, even under `mypy --strict`, while at runtime `isinstance(1, float)` is `False`. Any field annotated `float` can therefore silently store an `int`, and code unwrapping it with `match … case float():` falls through to `assert_never()`, calls to `float`-only methods like `hex()` crash, and `type()`-based dispatch misbehaves — despite everything type-checking cleanly. There is no clean fix in current Python (see the discussion in #250): runtime coercion at ingress was prototyped and measured ~2.3× slower on hot-path types, structural `Protocol` tricks don't close the widened variable and `Sequence` covariance holes, and narrowing match arms one by one leaves the annotation lying. So the decision is to stop lying instead: annotate every such value as `float | int`, which is what PEP 484 actually admits, at zero runtime cost. It is also forward-compatible with the typing-council proposal to make `float` mean `float | int` (python/typing-council#46). Add the `FloatInt` alias as the canonical spelling of that union. The alias gives the workaround a single documented home — explaining the trap, the `bool ⊂ int` leak. Follow-up commits migrate the existing `float` annotations to it. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/__init__.py | 2 ++ src/frequenz/client/common/_float.py | 50 ++++++++++++++++++++++++++ tests/test_float.py | 21 +++++++++++ 3 files changed, 73 insertions(+) create mode 100644 src/frequenz/client/common/_float.py create mode 100644 tests/test_float.py diff --git a/src/frequenz/client/common/__init__.py b/src/frequenz/client/common/__init__.py index 0580c58d..9dd94a7c 100644 --- a/src/frequenz/client/common/__init__.py +++ b/src/frequenz/client/common/__init__.py @@ -10,9 +10,11 @@ UnrecognizedEnumValueError, UnspecifiedEnumValueError, ) +from ._float import FloatInt __all__ = [ "ClientCommonError", + "FloatInt", "InvalidAttributeError", "MissingFieldError", "UnrecognizedEnumValueError", diff --git a/src/frequenz/client/common/_float.py b/src/frequenz/client/common/_float.py new file mode 100644 index 00000000..fcdce535 --- /dev/null +++ b/src/frequenz/client/common/_float.py @@ -0,0 +1,50 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Honest type alias for floating-point values.""" + +from typing import TypeAlias + +FloatInt: TypeAlias = float | int +"""A `float` that may actually be an `int` at runtime. + +[PEP 484's numeric tower](https://peps.python.org/pep-0484/#the-numeric-tower) +makes `int` assignable to any `float`-annotated parameter or field, so a plain +`float` annotation is a lie: type checkers (even `mypy --strict`) happily +accept `int` values, but `isinstance(1, float)` is `False` at runtime. That +breaks `match … case float():` arms (an `int` value falls through to +`assert_never()`), calls to `float`-only methods like `hex()`, and any other +code dispatching on the concrete runtime type. + +This library instead annotates such values as `FloatInt`, making the +heterogeneity explicit: type checkers will push code reading these values to +handle both branches, typically by matching with `case float() | int():`. See +[issue #250](https://github.com/frequenz-floss/frequenz-client-common-python/issues/250) +for the full analysis and the alternatives that were rejected. + +Danger: + `bool` is a subclass of `int`, so `True` and `False` also satisfy this + alias. This is inherent to Python's type system and not guarded against. + +Example: + ```python + from typing import assert_never + + from frequenz.client.common import FloatInt + + + def describe(value: FloatInt | None) -> str: + match value: + case float() | int(): + return f"number {value}" + case None: + return "nothing" + case unexpected: + assert_never(unexpected) + + + assert describe(1) == "number 1" + assert describe(1.5) == "number 1.5" + assert describe(None) == "nothing" + ``` +""" diff --git a/tests/test_float.py b/tests/test_float.py new file mode 100644 index 00000000..ba1b6454 --- /dev/null +++ b/tests/test_float.py @@ -0,0 +1,21 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for the `FloatInt` type alias.""" + +from frequenz.client.common import FloatInt + + +def test_alias_covers_float_and_int() -> None: + """The alias is exactly the `float | int` union.""" + assert FloatInt == float | int + + +def test_alias_admits_the_numeric_tower() -> None: + """`float`, `int` and (inherently) `bool` values all satisfy the alias.""" + assert isinstance(1.5, FloatInt) + assert isinstance(1, FloatInt) + # `bool` is a subclass of `int`, this is documented as not guarded against. + assert isinstance(True, FloatInt) + assert not isinstance("1.5", FloatInt) + assert not isinstance(None, FloatInt) From 558e20e5935d2117f32f85dc9f14794b078ca6df Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:00:53 +0000 Subject: [PATCH 12/25] Use `FloatInt` in metric bounds Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 15 ++++++++------- tests/metrics/_bounds/test_bounds.py | 18 ++++++++++++++++-- tests/metrics/_bounds/test_bounds_set.py | 13 ++++++++++++- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 3cd812e9..5b051b1d 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -11,6 +11,7 @@ from typing import Any, Self from .._exception import InvalidAttributeError +from .._float import FloatInt @dataclasses.dataclass(frozen=True, kw_only=True) @@ -22,13 +23,13 @@ class BaseBounds: malformed wire data. """ - lower: float | int | None = None + lower: FloatInt | None = None """The lower bound. If `None`, there is no lower bound. """ - upper: float | int | None = None + upper: FloatInt | None = None """The upper bound. If `None`, there is no upper bound. @@ -72,7 +73,7 @@ def __str__(self) -> str: """Return a string representation of these bounds.""" return f"[{self.lower},{self.upper}]" - def __contains__(self, item: float | None) -> bool: + def __contains__(self, item: FloatInt | None) -> bool: """Check whether a value is within these bounds. The bounds are inclusive on both ends, and a `None` bound means these @@ -173,7 +174,7 @@ def __init__( ) -def _end_covers_start(upper: float | None, lower: float | None) -> bool: +def _end_covers_start(upper: FloatInt | None, lower: FloatInt | None) -> bool: """Return whether an upper bound reaches a lower bound, treating `None` as ±∞. Args: @@ -190,7 +191,7 @@ def _end_covers_start(upper: float | None, lower: float | None) -> bool: return not upper < lower -def _max_upper(first: float | None, second: float | None) -> float | None: +def _max_upper(first: FloatInt | None, second: FloatInt | None) -> FloatInt | None: """Return the larger of two upper bounds, where `None` means +∞. Args: @@ -226,7 +227,7 @@ def _sort_and_merge_bounds(bounds: Iterable[Bounds]) -> tuple[Bounds, ...]: return () with_none_lower: list[Bounds] = [] - with_real_lower: list[tuple[float, Bounds]] = [] + with_real_lower: list[tuple[FloatInt, Bounds]] = [] for bound in all_bounds: if bound.lower is None: with_none_lower.append(bound) @@ -306,7 +307,7 @@ def __post_init__(self) -> None: """Normalize the bounds by sorting and merging overlapping ones.""" object.__setattr__(self, "bounds", _sort_and_merge_bounds(self.bounds)) - def __contains__(self, item: float | None) -> bool: + def __contains__(self, item: FloatInt | None) -> bool: """Check whether a value is within any bounds of this set. Args: diff --git a/tests/metrics/_bounds/test_bounds.py b/tests/metrics/_bounds/test_bounds.py index 6d38cdb0..e448cabb 100644 --- a/tests/metrics/_bounds/test_bounds.py +++ b/tests/metrics/_bounds/test_bounds.py @@ -8,6 +8,7 @@ import pytest +from frequenz.client.common import FloatInt from frequenz.client.common.metrics import BaseBounds, Bounds @@ -27,10 +28,11 @@ def test_is_base_bounds_subclass() -> None: (-10.0, -10.0), (0.0, 10.0), (-10, 0.0), + (-10, 10), # the numeric tower lets plain `int` bounds in (0.0, 0.0), ], ) -def test_creation(lower: float | int | None, upper: float | int | None) -> None: +def test_creation(lower: FloatInt | None, upper: FloatInt | None) -> None: """Test creation of Bounds with valid values.""" bounds = Bounds(lower=lower, upper=upper) assert bounds.lower == lower @@ -52,6 +54,8 @@ def test_str_representation() -> None: """Test string representation of Bounds.""" bounds = Bounds(lower=-10.0, upper=10.0) assert str(bounds) == "[-10.0,10.0]" + # `int` bounds keep their `int` repr; values are stored untouched. + assert str(Bounds(lower=-10, upper=10)) == "[-10,10]" def test_equality() -> None: @@ -65,6 +69,11 @@ def test_equality() -> None: assert bounds2 != bounds3 +def test_equality_int_float() -> None: + """`int` and `float` bounds with the same value compare equal (`1 == 1.0`).""" + assert Bounds(lower=-10, upper=10) == Bounds(lower=-10.0, upper=10.0) + + def test_hash() -> None: """Test that Bounds objects can be used in sets and as dictionary keys.""" bounds1 = Bounds(lower=-10.0, upper=10.0) @@ -94,10 +103,15 @@ def test_hash() -> None: (-10.0, None, 1e9, True), # unbounded above (-10.0, None, -10.0, True), (-10.0, None, -10.1, False), + (-10, 10, 5, True), # `int` bounds and `int` items work the same + (-10, 10, 10, True), + (-10, 10, 11, False), + (-10.0, 10.0, 10, True), # `int` item against `float` bounds + (-10, 10, 10.1, False), # `float` item against `int` bounds ], ) def test_contains( - lower: float | None, upper: float | None, item: float, expected: bool + lower: FloatInt | None, upper: FloatInt | None, item: FloatInt, expected: bool ) -> None: """Test membership with `in`, inclusive on both ends.""" assert (item in Bounds(lower=lower, upper=upper)) is expected diff --git a/tests/metrics/_bounds/test_bounds_set.py b/tests/metrics/_bounds/test_bounds_set.py index c44c6670..2209b5d9 100644 --- a/tests/metrics/_bounds/test_bounds_set.py +++ b/tests/metrics/_bounds/test_bounds_set.py @@ -7,6 +7,7 @@ import pytest +from frequenz.client.common import FloatInt from frequenz.client.common.metrics import Bounds, BoundsSet @@ -126,9 +127,11 @@ def test_all_covering_halves_collapse_to_empty() -> None: (15.0, True), (20.0, True), (21.0, False), + (3, True), # `int` items work the same + (6, False), ], ) -def test_contains(item: float, expected: bool) -> None: +def test_contains(item: FloatInt, expected: bool) -> None: """Membership tests the union of all bounds, inclusive on both ends.""" bounds_set = BoundsSet( bounds=(Bounds(lower=1.0, upper=5.0), Bounds(lower=15.0, upper=20.0)) @@ -148,6 +151,14 @@ def test_contains_nan() -> None: assert math.nan not in BoundsSet(bounds=(Bounds(lower=1.0, upper=5.0),)) +def test_int_bounds_normalize_with_float_bounds() -> None: + """`int` bounds sort, merge and membership-test seamlessly with `float` ones.""" + result = BoundsSet(bounds=(Bounds(lower=1, upper=5), Bounds(lower=5.0, upper=10.0))) + assert result.bounds == (Bounds(lower=1, upper=10.0),) + assert 7 in result + assert 0 not in result + + def test_str() -> None: """The string form joins members with a union symbol.""" bounds_set = BoundsSet( From f2c4f36d8566063580193e2b52af8cfd4c8e5b6e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:02:28 +0000 Subject: [PATCH 13/25] Use `FloatInt` in metric samples Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 17 +++++---- tests/metrics/test_sample_aggregated_value.py | 17 +++++++-- tests/metrics/test_sample_metric_sample.py | 38 +++++++++++++++++-- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index fecebf67..d00de53c 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -13,6 +13,7 @@ from typing_extensions import deprecated from .._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError +from .._float import FloatInt from ._bounds import Bounds, BoundsSet, InvalidBoundsSet, InvalidBoundsSetError from ._metric import Metric @@ -45,16 +46,16 @@ class AggregatedMetricValue: are available. """ - avg: float + avg: FloatInt """The derived average value of the metric.""" - min: float | None + min: FloatInt | None """The minimum measured value of the metric.""" - max: float | None + max: FloatInt | None """The maximum measured value of the metric.""" - raw: Sequence[float] + raw: Sequence[FloatInt] """All the raw individual values (it might be empty if not provided by the component).""" def __str__(self) -> str: @@ -193,7 +194,7 @@ class MetricSample: `MetricSample.get_metric()` to obtain a known member or a clear error. """ - value: float | AggregatedMetricValue | None + value: FloatInt | AggregatedMetricValue | None """The value of the sampled metric.""" bounds_set: BoundsSet | InvalidBoundsSet @@ -244,7 +245,7 @@ def __init__( *, sample_time: datetime, metric: Metric | int, - value: float | AggregatedMetricValue | None, + value: FloatInt | AggregatedMetricValue | None, bounds_set: BoundsSet | InvalidBoundsSet | None = None, bounds: list[Bounds] | None = None, connection: MetricConnection | None = None, @@ -307,10 +308,10 @@ def bounds(self) -> list[Bounds]: def as_single_value( self, *, aggregation_method: AggregationMethod = AggregationMethod.AVG - ) -> float | None: + ) -> FloatInt | None: """Return the value of this sample as a single value. - If [`value`][..value] is a `float`, it is returned as is. If `value` + If [`value`][..value] is a number, it is returned as is. If `value` is an [`AggregatedMetricValue`][...AggregatedMetricValue], the value is aggregated using the provided `aggregation_method`. diff --git a/tests/metrics/test_sample_aggregated_value.py b/tests/metrics/test_sample_aggregated_value.py index e672b4d9..43719d33 100644 --- a/tests/metrics/test_sample_aggregated_value.py +++ b/tests/metrics/test_sample_aggregated_value.py @@ -5,6 +5,7 @@ import pytest +from frequenz.client.common import FloatInt from frequenz.client.common.metrics import AggregatedMetricValue @@ -27,13 +28,21 @@ "avg:5.0", id="minimal_data", ), + pytest.param( + 5, + 1, + 10, + [1, 5.0, 10], + "avg:5", + id="int_data", + ), ], ) def test_creation_and_str( - avg: float, - min_val: float | None, - max_val: float | None, - raw: list[float], + avg: FloatInt, + min_val: FloatInt | None, + max_val: FloatInt | None, + raw: list[FloatInt], expected_str: str, ) -> None: """Test AggregatedMetricValue creation and string representation.""" diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index e9f42bdb..4d255b73 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -8,6 +8,7 @@ import pytest from frequenz.client.common import ( + FloatInt, UnrecognizedEnumValueError, UnspecifiedEnumValueError, ) @@ -58,7 +59,7 @@ def now() -> datetime: ) def test_creation( now: datetime, - value: float | AggregatedMetricValue | None, + value: FloatInt | AggregatedMetricValue | None, connection: MetricConnection | None, ) -> None: """Test MetricSample creation with different value types.""" @@ -112,12 +113,30 @@ def test_creation( }, id="none_value", ), + pytest.param( + 5, + { + AggregationMethod.AVG: 5, + AggregationMethod.MIN: 5, + AggregationMethod.MAX: 5, + }, + id="simple_int_value", + ), + pytest.param( + AggregatedMetricValue(avg=5, min=1, max=10, raw=[1, 5, 10]), + { + AggregationMethod.AVG: 5, + AggregationMethod.MIN: 1, + AggregationMethod.MAX: 10, + }, + id="aggregated_int_value", + ), ], ) def test_as_single_value( now: datetime, - value: float | AggregatedMetricValue | None, - method_results: dict[AggregationMethod, float | None], + value: FloatInt | AggregatedMetricValue | None, + method_results: dict[AggregationMethod, FloatInt | None], ) -> None: """Test MetricSample.as_single_value with different value types and methods.""" bounds_set = BoundsSet(bounds=(Bounds(lower=-10.0, upper=10.0),)) @@ -133,6 +152,19 @@ def test_as_single_value( assert sample.as_single_value(aggregation_method=method) == expected +def test_as_single_value_returns_int_untouched(now: datetime) -> None: + """An `int` value is returned as is, without coercion to `float`.""" + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5, + bounds_set=BoundsSet(), + ) + result = sample.as_single_value() + assert result == 5 + assert type(result) is int # pylint: disable=unidiomatic-typecheck + + def test_multiple_bounds(now: datetime) -> None: """Test MetricSample creation with multiple bounds.""" bounds_set = BoundsSet( From f7011dca6d5480c9d9bf121b4d180265ded151da Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:04:53 +0000 Subject: [PATCH 14/25] Use `FloatInt` in `Location` Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/types/_location.py | 47 ++++++++-------- .../types/_location/test_invalid_latitude.py | 7 +++ .../types/_location/test_invalid_longitude.py | 7 +++ tests/types/_location/test_location.py | 55 +++++++++++++------ 4 files changed, 75 insertions(+), 41 deletions(-) diff --git a/src/frequenz/client/common/types/_location.py b/src/frequenz/client/common/types/_location.py index 90e12d03..bd946ed8 100644 --- a/src/frequenz/client/common/types/_location.py +++ b/src/frequenz/client/common/types/_location.py @@ -7,13 +7,14 @@ from typing import assert_never from .._exception import InvalidAttributeError, MissingFieldError +from .._float import FloatInt class InvalidLatitudeError(InvalidAttributeError): """Raised when a semantic accessor sees a latitude outside `[-90, 90]`. A well-formed latitude lies in the closed interval `[-90, 90]`. The raw - out-of-range float is available as `value`. + out-of-range number is available as `value`. This is also a [`ValueError`][] for convenience. """ @@ -22,7 +23,7 @@ def __init__( self, instance: object, attr_name: str, - value: float, + value: FloatInt, message: str | None = None, ) -> None: """Initialize this error. @@ -34,7 +35,7 @@ def __init__( message: A custom error message. If `None`, a default message mentioning the invalid value is used. """ - self.value: float = value + self.value: FloatInt = value """The out-of-range latitude value.""" super().__init__( @@ -53,7 +54,7 @@ class InvalidLongitudeError(InvalidAttributeError): """Raised when a semantic accessor sees a longitude outside `[-180, 180]`. A well-formed longitude lies in the closed interval `[-180, 180]`. The raw - out-of-range float is available as `value`. + out-of-range number is available as `value`. This is also a [`ValueError`][] for convenience. """ @@ -62,7 +63,7 @@ def __init__( self, instance: object, attr_name: str, - value: float, + value: FloatInt, message: str | None = None, ) -> None: """Initialize this error. @@ -74,7 +75,7 @@ def __init__( message: A custom error message. If `None`, a default message mentioning the invalid value is used. """ - self.value: float = value + self.value: FloatInt = value """The out-of-range longitude value.""" super().__init__( @@ -136,7 +137,7 @@ class InvalidLatitude: Wraps a raw wire latitude that fell outside the well-formed range. """ - value: float + value: FloatInt """The raw out-of-range latitude value.""" def __str__(self) -> str: @@ -151,7 +152,7 @@ class InvalidLongitude: Wraps a raw wire longitude that fell outside the well-formed range. """ - value: float + value: FloatInt """The raw out-of-range longitude value.""" def __str__(self) -> str: @@ -194,33 +195,33 @@ class Location: validated value or a clear [`InvalidAttributeError`][...InvalidAttributeError] subclass. - Constructing a `Location` with a plain `float` or `str` that violates + Constructing a `Location` with a plain number or `str` that violates its invariant raises `ValueError`; use the corresponding `Invalid*` type to represent an out-of-invariant wire value. """ - latitude: float | InvalidLatitude + latitude: FloatInt | InvalidLatitude """The latitude. - A plain `float` when well-formed (in `[-90, 90]`); an + A plain number when well-formed (in `[-90, 90]`); an [`InvalidLatitude`][...InvalidLatitude] wrapper when the wire delivered an out-of-range value. Tip: Use [`Location.get_latitude()`][...Location.get_latitude] to obtain - a validated `float` or a clear error. + a validated number or a clear error. """ - longitude: float | InvalidLongitude + longitude: FloatInt | InvalidLongitude """The longitude. - A plain `float` when well-formed (in `[-180, 180]`); an + A plain number when well-formed (in `[-180, 180]`); an [`InvalidLongitude`][...InvalidLongitude] wrapper when the wire delivered an out-of-range value. Tip: Use [`Location.get_longitude()`][...Location.get_longitude] to obtain a - validated `float` or a clear error. + validated number or a clear error. """ country_code: str | InvalidCountryCode | None @@ -241,8 +242,8 @@ def __post_init__(self) -> None: """Enforce that plain (unwrapped) fields respect their invariants. Raises: - ValueError: If `latitude` is a plain `float` outside `[-90, 90]`; - if `longitude` is a plain `float` outside `[-180, 180]`; or + ValueError: If `latitude` is a plain number outside `[-90, 90]`; + if `longitude` is a plain number outside `[-180, 180]`; or if `country_code` is a plain `str` not exactly 2 characters long. To represent an invalid wire value, wrap it in the corresponding `Invalid*` type. @@ -272,11 +273,11 @@ def __post_init__(self) -> None: "invalid wire value" ) - def get_latitude(self) -> float: - """Return the latitude as a well-formed `float` in `[-90, 90]`. + def get_latitude(self) -> FloatInt: + """Return the latitude as a well-formed number in `[-90, 90]`. Returns: - The latitude, when it is a well-formed `float`. + The latitude, when it is a well-formed number. Raises: InvalidLatitudeError: If [`latitude`][..latitude] is an @@ -291,11 +292,11 @@ def get_latitude(self) -> float: case unknown: assert_never(unknown) - def get_longitude(self) -> float: - """Return the longitude as a well-formed `float` in `[-180, 180]`. + def get_longitude(self) -> FloatInt: + """Return the longitude as a well-formed number in `[-180, 180]`. Returns: - The longitude, when it is a well-formed `float`. + The longitude, when it is a well-formed number. Raises: InvalidLongitudeError: If [`longitude`][..longitude] is an diff --git a/tests/types/_location/test_invalid_latitude.py b/tests/types/_location/test_invalid_latitude.py index 5302e298..15d892ed 100644 --- a/tests/types/_location/test_invalid_latitude.py +++ b/tests/types/_location/test_invalid_latitude.py @@ -24,3 +24,10 @@ def test_equality() -> None: def test_str() -> None: """`InvalidLatitude.__str__` renders with a compact invalid marker.""" assert str(InvalidLatitude(value=91.0)) == "" + + +def test_int_value() -> None: + """An `int` value is stored untouched and renders like a `float`.""" + invalid = InvalidLatitude(value=91) + assert type(invalid.value) is int # pylint: disable=unidiomatic-typecheck + assert str(invalid) == "" diff --git a/tests/types/_location/test_invalid_longitude.py b/tests/types/_location/test_invalid_longitude.py index 1596f030..5c6f0e47 100644 --- a/tests/types/_location/test_invalid_longitude.py +++ b/tests/types/_location/test_invalid_longitude.py @@ -24,3 +24,10 @@ def test_equality() -> None: def test_str() -> None: """`InvalidLongitude.__str__` renders with a compact invalid marker.""" assert str(InvalidLongitude(value=181.0)) == "" + + +def test_int_value() -> None: + """An `int` value is stored untouched and renders like a `float`.""" + invalid = InvalidLongitude(value=181) + assert type(invalid.value) is int # pylint: disable=unidiomatic-typecheck + assert str(invalid) == "" diff --git a/tests/types/_location/test_location.py b/tests/types/_location/test_location.py index 4908215b..3ad2b3a7 100644 --- a/tests/types/_location/test_location.py +++ b/tests/types/_location/test_location.py @@ -8,6 +8,7 @@ import pytest +from frequenz.client.common import FloatInt from frequenz.client.common._exception import MissingFieldError from frequenz.client.common.types import ( InvalidCountryCode, @@ -61,22 +62,22 @@ def test_construction_wrapped_invalid_country_code() -> None: @pytest.mark.parametrize( "latitude", - [-90.001, 90.001, math.nan, float("inf"), float("-inf")], - ids=["below_min", "above_max", "nan", "inf", "neg_inf"], + [-90.001, 90.001, math.nan, float("inf"), float("-inf"), -91, 91], + ids=["below_min", "above_max", "nan", "inf", "neg_inf", "int_below", "int_above"], ) -def test_construction_rejects_plain_invalid_latitude(latitude: float) -> None: - """A plain `float` latitude outside `[-90, 90]` is rejected at construction.""" +def test_construction_rejects_plain_invalid_latitude(latitude: FloatInt) -> None: + """A plain latitude number outside `[-90, 90]` is rejected at construction.""" with pytest.raises(ValueError, match=r"latitude .* is outside \[-90, 90\]"): Location(latitude=latitude, longitude=13.405, country_code="DE") @pytest.mark.parametrize( "longitude", - [-180.001, 180.001, math.nan, float("inf"), float("-inf")], - ids=["below_min", "above_max", "nan", "inf", "neg_inf"], + [-180.001, 180.001, math.nan, float("inf"), float("-inf"), -181, 181], + ids=["below_min", "above_max", "nan", "inf", "neg_inf", "int_below", "int_above"], ) -def test_construction_rejects_plain_invalid_longitude(longitude: float) -> None: - """A plain `float` longitude outside `[-180, 180]` is rejected at construction.""" +def test_construction_rejects_plain_invalid_longitude(longitude: FloatInt) -> None: + """A plain longitude number outside `[-180, 180]` is rejected at construction.""" with pytest.raises(ValueError, match=r"longitude .* is outside \[-180, 180\]"): Location(latitude=52.52, longitude=longitude, country_code="DE") @@ -117,15 +118,23 @@ def test_dataclasses_replace_enforces_invariant() -> None: @pytest.mark.parametrize( "latitude", - [-90.0, 0.0, 90.0], - ids=["min_boundary", "middle", "max_boundary"], + [-90.0, 0.0, 90.0, -90, 45, 90], + ids=["min_boundary", "middle", "max_boundary", "int_min", "int_middle", "int_max"], ) -def test_get_latitude_returns_valid(latitude: float) -> None: - """`get_latitude()` returns the stored `float` when well-formed.""" +def test_get_latitude_returns_valid(latitude: FloatInt) -> None: + """`get_latitude()` returns the stored number when well-formed.""" location = Location(latitude=latitude, longitude=13.405, country_code="DE") assert location.get_latitude() == pytest.approx(latitude) +def test_get_latitude_returns_int_untouched() -> None: + """An `int` latitude is returned as is, without coercion to `float`.""" + location = Location(latitude=45, longitude=13.405, country_code="DE") + result = location.get_latitude() + assert result == 45 + assert type(result) is int # pylint: disable=unidiomatic-typecheck + + def test_get_latitude_raises_for_wrapper() -> None: """`get_latitude()` raises `InvalidLatitudeError` when latitude is wrapped.""" location = Location( @@ -146,15 +155,23 @@ def test_get_latitude_raises_for_wrapper() -> None: @pytest.mark.parametrize( "longitude", - [-180.0, 0.0, 180.0], - ids=["min_boundary", "middle", "max_boundary"], + [-180.0, 0.0, 180.0, -180, 90, 180], + ids=["min_boundary", "middle", "max_boundary", "int_min", "int_middle", "int_max"], ) -def test_get_longitude_returns_valid(longitude: float) -> None: - """`get_longitude()` returns the stored `float` when well-formed.""" +def test_get_longitude_returns_valid(longitude: FloatInt) -> None: + """`get_longitude()` returns the stored number when well-formed.""" location = Location(latitude=52.52, longitude=longitude, country_code="DE") assert location.get_longitude() == pytest.approx(longitude) +def test_get_longitude_returns_int_untouched() -> None: + """An `int` longitude is returned as is, without coercion to `float`.""" + location = Location(latitude=52.52, longitude=90, country_code="DE") + result = location.get_longitude() + assert result == 90 + assert type(result) is int # pylint: disable=unidiomatic-typecheck + + def test_get_longitude_raises_for_wrapper() -> None: """`get_longitude()` raises `InvalidLongitudeError` when longitude is wrapped.""" location = Location( @@ -259,6 +276,7 @@ def test_get_country_code_or_none_raises_for_wrapper() -> None: InvalidCountryCode(value="DEU"), "(,)", ), + (45, 90, "DE", "DE(45.00,90.00)"), ], ids=[ "valid", @@ -267,11 +285,12 @@ def test_get_country_code_or_none_raises_for_wrapper() -> None: "invalid_lat", "invalid_lon", "all_invalid", + "int_lat_lon", ], ) def test_str( - latitude: float | InvalidLatitude, - longitude: float | InvalidLongitude, + latitude: FloatInt | InvalidLatitude, + longitude: FloatInt | InvalidLongitude, country_code: str | InvalidCountryCode | None, expected: str, ) -> None: From 380314fe754002472108309a077aee043a9df7ba Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:05:59 +0000 Subject: [PATCH 15/25] Use `FloatInt` in `PowerTransformer` Signed-off-by: Leandro Lucarella --- .../microgrid/electrical_components/_power_transformer.py | 5 +++-- .../electrical_components/test_power_transformer.py | 8 +++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py b/src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py index d86f90e9..51f41bb2 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py @@ -5,6 +5,7 @@ import dataclasses +from ..._float import FloatInt from ._electrical_component import ElectricalComponent @@ -22,13 +23,13 @@ class PowerTransformer(ElectricalComponent): than the input power. """ - primary_voltage: float + primary_voltage: FloatInt """The primary voltage of the transformer, in volts. This is the input voltage that is stepped up or down. """ - secondary_voltage: float + secondary_voltage: FloatInt """The secondary voltage of the transformer, in volts. This is the output voltage that is the result of stepping the primary diff --git a/tests/microgrid/electrical_components/test_power_transformer.py b/tests/microgrid/electrical_components/test_power_transformer.py index 9f8eb7f0..4a127f90 100644 --- a/tests/microgrid/electrical_components/test_power_transformer.py +++ b/tests/microgrid/electrical_components/test_power_transformer.py @@ -5,6 +5,7 @@ import pytest +from frequenz.client.common import FloatInt from frequenz.client.common.microgrid import MicrogridId from frequenz.client.common.microgrid.electrical_components import ( ElectricalComponentId, @@ -25,13 +26,14 @@ def microgrid_id() -> MicrogridId: @pytest.mark.parametrize( - "primary, secondary", [(400.0, 230.0), (0.0, 0.0), (230.0, 400.0), (-230.0, -400.0)] + "primary, secondary", + [(400.0, 230.0), (0.0, 0.0), (230.0, 400.0), (-230.0, -400.0), (400, 230)], ) def test_creation_ok( component_id: ElectricalComponentId, microgrid_id: MicrogridId, - primary: float, - secondary: float, + primary: FloatInt, + secondary: FloatInt, ) -> None: """Test PowerTransformer component initialization with different voltages.""" power_transformer = PowerTransformer( From c2395af5c53debd51063c5f5170d65c1a496f003 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:07:17 +0000 Subject: [PATCH 16/25] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index bac25102..f37b2067 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -70,6 +70,14 @@ Malformed bounds are now preserved in the returned `MetricSample.bounds_set` as an `InvalidBoundsSet` (validity is encoded in the type), so the previous "bounds for ... is invalid, ignoring these bounds" major issue is no longer produced. +* `float`-typed fields and accessors are now annotated with the new `FloatInt` (`float | int`) type alias (see New Features), to be honest about what PEP 484's numeric tower actually admits. These symbols are affected: + + * `frequenz.client.common.metrics.AggregatedMetricValue`: the `avg`, `min`, `max` and `raw` fields. + * `frequenz.client.common.metrics.MetricSample`: the `value` field and the `as_single_value()` return type. + * `frequenz.client.common.metrics.Bounds`: the `lower` and `upper` fields (shared with the new `BaseBounds` / `InvalidBounds` hierarchy). + + Runtime behavior is completely unchanged: these fields could always end up storing `int` values (`x: float = 1` is legal even under `mypy --strict`), the annotations just didn't admit it. Reads that assign to `float`-typed destinations or do plain arithmetic keep type-checking as before. However, code that pattern-matches these values with a bare `case float():` arm — a latent runtime crash, since `isinstance(1, float)` is `False` — will now be flagged as non-exhaustive by strict type checkers and should be widened to `case float() | int():`, and calling `float`-only methods (e.g. `hex()`) on them now requires an explicit `float(...)` conversion. + ## New Features * Added 4 new electrical component classes for categories that previously collapsed into `UnrecognizedElectricalComponent`: @@ -138,6 +146,11 @@ * Added a new `frequenz.client.common.microgrid.Microgrid` type with a raising `is_active()` method, together with the `frequenz.client.common.microgrid.proto.v1alpha8.microgrid_from_proto` conversion function. +* Added `frequenz.client.common.FloatInt`, a type alias for `float | int`. + + PEP 484's numeric tower makes `int` assignable wherever `float` is annotated, even under `mypy --strict`, while at runtime `isinstance(1, float)` is `False` — so a plain `float` annotation silently admits values that crash `match … case float():` arms and `float`-only methods like `hex()`. The library now spells such annotations `FloatInt` instead of lying (see Upgrading); the alias docstring documents the trap in detail, including the inherent `bool ⊂ int` leak. New numeric fields (`Location` latitudes/longitudes, `PowerTransformer` voltages, bounds and bounds sets) use it as well. Values loaded from protobuf are unaffected in practice, as the wire always delivers real `float`s. + ## Bug Fixes * Fixed `EnumParityTest` so protobuf values whose Python member name exists with a different number fail parity checks instead of being treated as unmirrored protobuf values. +* Fixed potential unexpected exceptions due to type-checking accepting `int` for code annotated to only accept `float`. Fixes #250. From fc2996b2f29048f12411d89a236510345675196c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 21 Jul 2026 10:47:20 +0200 Subject: [PATCH 17/25] Remove `None` from `MetricConnection.name` The protobuf message field is required, so the string will always come. Adding `None` as a possibility in the type system forces user code to deal with `None`, and adds an ambiguity between `None` and `""`. So, as we did with other `name` or similar string fields, it is better to just leave it as a pure `str` and let users deal with the empty string more easily, without the need to special-case `None`. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 4 ++-- .../client/common/metrics/proto/v1alpha8/_sample.py | 2 +- .../proto/v1alpha8/test_sample_metric_connection.py | 2 +- tests/metrics/test_sample_metric_connection.py | 12 ++++++------ 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index d00de53c..a22fc409 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -117,7 +117,7 @@ class MetricConnection: `MetricConnection.get_category()` to obtain a known member or a clear error. """ - name: str | None = None + name: str = "" """The name of the specific connection from which the metric was obtained. This is expected to be populated when the same [`Metric`][...Metric] variant @@ -133,7 +133,7 @@ def __str__(self) -> str: if isinstance(self.category, int) else f"" ) - if self.name is not None: + if self.name: return f"{category_name}({self.name})" return category_name diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py index 6b8a28cc..d86ffc07 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py @@ -69,7 +69,7 @@ def metric_connection_from_proto_with_issues( return MetricConnection( category=category, - name=message.name or None, + name=message.name, ) diff --git a/tests/metrics/proto/v1alpha8/test_sample_metric_connection.py b/tests/metrics/proto/v1alpha8/test_sample_metric_connection.py index 1df6e096..9a4b7501 100644 --- a/tests/metrics/proto/v1alpha8/test_sample_metric_connection.py +++ b/tests/metrics/proto/v1alpha8/test_sample_metric_connection.py @@ -95,6 +95,6 @@ def test_with_empty_name() -> None: ) assert connection.category == MetricConnectionCategory.PV - assert connection.name is None + assert not connection.name assert not major_issues assert not minor_issues diff --git a/tests/metrics/test_sample_metric_connection.py b/tests/metrics/test_sample_metric_connection.py index 092ef7d3..d6888c9a 100644 --- a/tests/metrics/test_sample_metric_connection.py +++ b/tests/metrics/test_sample_metric_connection.py @@ -17,9 +17,9 @@ [ pytest.param( MetricConnectionCategory.BATTERY, - None, + "", "", - id="enum_category_no_name", + id="enum_category_empty_name", ), pytest.param( MetricConnectionCategory.PV, @@ -29,9 +29,9 @@ ), pytest.param( 999, - None, + "", "999", - id="int_category_no_name", + id="int_category_empty_name", ), pytest.param( 999, @@ -43,7 +43,7 @@ ) def test_str_representation( category: MetricConnectionCategory | int, - name: str | None, + name: str, expected_str: str, ) -> None: """Test string representation of MetricConnection.""" @@ -75,7 +75,7 @@ def test_creation_default_name() -> None: """Test MetricConnection creation with default name.""" connection = MetricConnection(category=MetricConnectionCategory.AMBIENT) assert connection.category == MetricConnectionCategory.AMBIENT - assert connection.name is None + assert not connection.name def test_equality() -> None: From c8de59ac398cd32df70b2cbdc5862d1d0b5de332 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 21 Jul 2026 09:37:29 +0000 Subject: [PATCH 18/25] Add `CategorySpecificInfo` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a small frozen dataclass pairing the protobuf `category_specific_info` variant name (`kind`) with the leftover fields that this client version did not translate into typed attributes. An electrical component may carry category-specific info on the wire. Known fields are decoded into typed attributes on the concrete component, but anything left over — because the category is unrecognized, or because a newer API version added fields this client doesn't know yet — was either dropped or kept as a bare `dict` with no indication of which variant it came from. This type gives that leftover data a documented home together with the variant name needed to interpret it. It is hashable like the components that will carry it. The leftover `fields` mapping may hold arbitrary, possibly unhashable values, and folding them into the hash — even through `repr()` — is unsafe: values that compare equal can hash or repr differently (e.g. `1 == 1.0 == True`), which would break the invariant that equal objects hash equally. So `fields` is excluded from the hash and instances hash on `kind` alone, mirroring `metric_config_bounds` on the component. Collisions between instances that share a `kind` but differ in `fields` are then possible, but harmless and highly unlikely in practice. A follow-up commit adopts it as the `category_specific_info` field on every electrical component. Signed-off-by: Leandro Lucarella --- .../electrical_components/__init__.py | 2 + .../_category_specific_info.py | 38 +++++++++++ .../test_category_specific_info.py | 68 +++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py create mode 100644 tests/microgrid/electrical_components/test_category_specific_info.py diff --git a/src/frequenz/client/common/microgrid/electrical_components/__init__.py b/src/frequenz/client/common/microgrid/electrical_components/__init__.py index 5f4484bc..79165ef2 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/__init__.py +++ b/src/frequenz/client/common/microgrid/electrical_components/__init__.py @@ -14,6 +14,7 @@ from ._breaker import Breaker from ._capacitor_bank import CapacitorBank from ._category import ElectricalComponentCategory +from ._category_specific_info import CategorySpecificInfo from ._chp import Chp from ._converter import Converter from ._crypto_miner import CryptoMiner @@ -81,6 +82,7 @@ "BatteryTypes", "Breaker", "CapacitorBank", + "CategorySpecificInfo", "Chp", "Converter", "CryptoMiner", diff --git a/src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py b/src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py new file mode 100644 index 00000000..41f65320 --- /dev/null +++ b/src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py @@ -0,0 +1,38 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Category specific info carried by an electrical component.""" + +import dataclasses +from collections.abc import Mapping +from typing import Any + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class CategorySpecificInfo: + """The category specific info carried by an electrical component. + + A protobuf electrical component may carry a `category_specific_info` variant + with extra fields tied to its category. Fields this library version + understands are translated into typed attributes on the concrete component + (e.g. the battery type). Anything left over — either because the component's + category is not recognized, or because a newer API version added fields this + client doesn't know yet — is preserved here so callers can still inspect the + raw values. + """ + + kind: str + """The name of the info variant carried on the wire (e.g. `"battery"`).""" + + fields: Mapping[str, Any] = dataclasses.field( + default_factory=dict, + # Excluded from the hash: values may be unhashable (e.g. lists), and even + # repr()-folding them breaks the eq/hash invariant since values that + # compare equal can differ under repr()/hash() (e.g. 1 == 1.0 == True). + # Instances hash on `kind` alone, mirroring `metric_config_bounds`. + hash=False, + ) + """The leftover fields not translated into typed attributes. + + The keys are the protobuf field names and the values their decoded content. + """ diff --git a/tests/microgrid/electrical_components/test_category_specific_info.py b/tests/microgrid/electrical_components/test_category_specific_info.py new file mode 100644 index 00000000..4433cc5b --- /dev/null +++ b/tests/microgrid/electrical_components/test_category_specific_info.py @@ -0,0 +1,68 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for the category specific info carried by electrical components.""" + +from frequenz.client.common.microgrid.electrical_components import ( + CategorySpecificInfo, +) + + +def test_construction() -> None: + """The kind and leftover fields are exposed as given.""" + info = CategorySpecificInfo(kind="battery", fields={"foo": "bar"}) + + assert info.kind == "battery" + assert info.fields == {"foo": "bar"} + + +def test_fields_default_to_empty() -> None: + """The leftover fields default to an empty mapping.""" + info = CategorySpecificInfo(kind="inverter") + + assert info.fields == {} + + +def test_equality() -> None: + """Equality considers both the kind and the leftover fields.""" + a = CategorySpecificInfo(kind="battery", fields={"x": 1}) + b = CategorySpecificInfo(kind="battery", fields={"x": 1}) + c = CategorySpecificInfo(kind="battery", fields={"x": 2}) + d = CategorySpecificInfo(kind="inverter", fields={"x": 1}) + + assert a == b + assert a != c + assert a != d + + +def test_equal_instances_hash_equally() -> None: + """Instances equal by content hash equally and dedupe in a set.""" + a = CategorySpecificInfo(kind="battery", fields={"x": 1, "y": 2}) + b = CategorySpecificInfo(kind="battery", fields={"y": 2, "x": 1}) + + assert a == b + assert hash(a) == hash(b) + assert len({a, b}) == 1 + + +def test_equal_values_hash_equally_regardless_of_type() -> None: + """Values that compare equal but repr differently still hash equally. + + Guards against deriving the hash from ``repr(value)``: ``1``, ``1.0`` and + ``True`` compare equal, so instances carrying them must hash equally and + deduplicate in a set (equal objects are required to have equal hashes). + """ + a = CategorySpecificInfo(kind="battery", fields={"x": 1}) + b = CategorySpecificInfo(kind="battery", fields={"x": 1.0}) + c = CategorySpecificInfo(kind="battery", fields={"x": True}) + + assert a == b == c + assert hash(a) == hash(b) == hash(c) + assert len({a, b, c}) == 1 + + +def test_hashable_with_unhashable_values() -> None: + """Fields carrying unhashable values can still be hashed.""" + info = CategorySpecificInfo(kind="battery", fields={"x": [1, 2]}) + + assert isinstance(hash(info), int) From 237495c0bc61f59800703b6b2599705bd3f2bbd1 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 21 Jul 2026 10:07:26 +0000 Subject: [PATCH 19/25] Store carried category specific info on every component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopt `CategorySpecificInfo | None` as the `category_specific_info` field on `ElectricalComponent`, replacing the bare `Mapping[str, Any]` that only the mismatched component ever populated. The protobuf `category_specific_info` oneof is now decoded once into its `kind` plus a dict of its fields, then each concrete component drops the fields it translated into typed attributes (e.g. a battery's `type`), keeping the `kind` and any leftover. The result: * recognized components keep the variant `kind` with an empty leftover; * `UnrecognizedElectricalComponent` now preserves the carried info instead of dropping it — previously the escape hatch was empty for the very class that needs it most; * `MismatchedCategoryElectricalComponent` keeps the full info (nothing was translated), recording exactly which variant disagreed with the category; * components carrying no variant get `None`. Because `CategorySpecificInfo` is hashable, the field no longer needs `hash=False` and now participates in the component hash like the other fields. Signed-off-by: Leandro Lucarella --- .../_electrical_component.py | 29 ++++---- .../proto/v1alpha8/_electrical_component.py | 68 ++++++++++++++++--- .../proto/v1alpha8/conftest.py | 3 +- .../test_electrical_component_base.py | 7 +- .../test_electrical_component_simple.py | 8 ++- .../proto/v1alpha8/test_raw_storage.py | 29 ++++++++ .../test_electrical_component_base.py | 23 ++++--- 7 files changed, 127 insertions(+), 40 deletions(-) diff --git a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py index 2b225cbe..037f00f8 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -12,6 +12,7 @@ from ...metrics import Bounds, InvalidBounds, InvalidBoundsError, Metric from .. import MicrogridId from .._lifetime import InvalidLifetime, InvalidLifetimeError, Lifetime +from ._category_specific_info import CategorySpecificInfo from ._ids import ElectricalComponentId DefaultT = TypeVar("DefaultT") @@ -81,7 +82,7 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes # dict is not hashable, so we don't use this field to calculate the hash. # This shouldn't be a problem since it is very unlikely that two components # with all other attributes being equal would have different category - # specific metadata, so hash collisions should be still very unlikely. + # specific info, so hash collisions should be still very unlikely. hash=False, ) ) @@ -103,21 +104,17 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes when a valid [`Bounds`][.....metrics.Bounds] is required. """ - category_specific_metadata: Mapping[str, Any] = dataclasses.field( - default_factory=dict, - # dict is not hashable, so we don't use this field to calculate the hash. This - # shouldn't be a problem since it is very unlikely that two components with all - # other attributes being equal would have different category specific metadata, - # so hash collisions should be still very unlikely. - hash=False, - ) - """The category specific metadata of this electrical component. - - Note: - This should not be used normally, it is only useful when accessing a newer - version of the API where the client doesn't know about the new metadata fields - yet (i.e. for use with - [`UnrecognizedElectricalComponent`][...UnrecognizedElectricalComponent]). + category_specific_info: CategorySpecificInfo | None = None + """The category specific info carried by this component, if any. + + This is `None` when the wire carried no category-specific info variant. + Otherwise it holds a + [`CategorySpecificInfo`][...CategorySpecificInfo] recording the variant + `kind` together with any fields that were not translated into typed + attributes on this component. The leftover fields are empty when everything + was translated, and non-empty when the category or its variant is not + recognized, or when a newer API version added fields this client version + doesn't know yet. """ def __new__(cls, *_: Any, **__: Any) -> Self: diff --git a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py index 8bc40b6e..e004f46a 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py @@ -6,7 +6,7 @@ import logging import warnings from collections.abc import Mapping, Sequence -from typing import Any, Final, NamedTuple, TypeAlias, assert_never, overload +from typing import Final, NamedTuple, TypeAlias, assert_never, overload from frequenz.api.common.v1alpha8.microgrid.electrical_components import ( electrical_components_pb2, @@ -29,6 +29,7 @@ from ..._breaker import Breaker from ..._capacitor_bank import CapacitorBank from ..._category import ElectricalComponentCategory +from ..._category_specific_info import CategorySpecificInfo from ..._chp import Chp from ..._converter import Converter from ..._crypto_miner import CryptoMiner @@ -900,8 +901,8 @@ class _ElectricalComponentBaseData(NamedTuple): [`Bounds`][frequenz.client.common.metrics.Bounds]. """ - category_specific_info: dict[str, Any] - """The category-specific metadata extracted from the protobuf message.""" + category_specific_info: CategorySpecificInfo | None + """The category specific info extracted from the protobuf message, if any.""" provides_telemetry: bool | int """Whether the electrical component provides telemetry, or `None` if unknown.""" @@ -910,7 +911,31 @@ class _ElectricalComponentBaseData(NamedTuple): """Whether the electrical component accepts control, or `None` if unknown.""" category_mismatched: bool = False - """Whether the declared category and the carried metadata disagree.""" + """Whether the declared category and the carried info disagree.""" + + +def _leftover_info( + info: CategorySpecificInfo | None, *translated_keys: str +) -> CategorySpecificInfo | None: + """Return the info without the fields translated into typed attributes. + + The variant `kind` is preserved whenever info was carried, so an empty + result still records which variant the wire carried. + + Args: + info: The full info carried on the wire, or `None` if none was. + *translated_keys: The field names already translated into typed + attributes on the target component. + + Returns: + The info without `translated_keys`, or `None` if no info was carried. + """ + if info is None: + return None + leftover = { + key: value for key, value in info.fields.items() if key not in translated_keys + } + return CategorySpecificInfo(kind=info.kind, fields=leftover) # pylint: disable-next=too-many-locals @@ -952,11 +977,16 @@ def _electrical_component_base_from_proto_with_issues( major_issues.append(f"category {category} is unrecognized") category_specific_info_kind = message.category_specific_info.WhichOneof("kind") - category_specific_info: dict[str, Any] = {} + category_specific_info: CategorySpecificInfo | None = None if category_specific_info_kind is not None: - category_specific_info = MessageToDict( - getattr(message.category_specific_info, category_specific_info_kind), - always_print_fields_with_no_presence=True, + category_specific_info = CategorySpecificInfo( + kind=category_specific_info_kind, + fields=MessageToDict( + getattr( + message.category_specific_info, category_specific_info_kind + ), + always_print_fields_with_no_presence=True, + ), ) category_mismatched = False @@ -1020,7 +1050,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, - category_specific_metadata=base_data.category_specific_info, + category_specific_info=base_data.category_specific_info, metric_config_bounds=base_data.metric_config_bounds, ) match base_data.category: @@ -1035,6 +1065,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=base_data.category_specific_info, metric_config_bounds=base_data.metric_config_bounds, ) case ( @@ -1068,6 +1099,7 @@ def electrical_component_from_proto_with_issues( metric_config_bounds=base_data.metric_config_bounds, ) case ElectricalComponentCategory.BATTERY: + battery_info = _leftover_info(base_data.category_specific_info, "type") raw_battery_type = message.category_specific_info.battery.type battery_class = _BATTERY_CLASS_BY_PROTO_TYPE.get(raw_battery_type) if raw_battery_type == ( @@ -1088,6 +1120,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=battery_info, metric_config_bounds=base_data.metric_config_bounds, type=raw_battery_type, ) @@ -1100,9 +1133,13 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=battery_info, metric_config_bounds=base_data.metric_config_bounds, ) case ElectricalComponentCategory.EV_CHARGER: + ev_charger_info = _leftover_info( + base_data.category_specific_info, "type" + ) raw_ev_charger_type = message.category_specific_info.ev_charger.type ev_charger_class = _EV_CHARGER_CLASS_BY_PROTO_TYPE.get( raw_ev_charger_type @@ -1125,6 +1162,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=ev_charger_info, metric_config_bounds=base_data.metric_config_bounds, type=raw_ev_charger_type, ) @@ -1137,9 +1175,13 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=ev_charger_info, metric_config_bounds=base_data.metric_config_bounds, ) case ElectricalComponentCategory.GRID_CONNECTION_POINT: + grid_info = _leftover_info( + base_data.category_specific_info, "ratedFuseCurrent" + ) rated_fuse_current = ( message.category_specific_info.grid_connection_point.rated_fuse_current ) @@ -1153,10 +1195,12 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=grid_info, metric_config_bounds=base_data.metric_config_bounds, rated_fuse_current=rated_fuse_current, ) case ElectricalComponentCategory.INVERTER: + inverter_info = _leftover_info(base_data.category_specific_info, "type") raw_inverter_type = message.category_specific_info.inverter.type inverter_class = _INVERTER_CLASS_BY_PROTO_TYPE.get(raw_inverter_type) if raw_inverter_type == ( @@ -1177,6 +1221,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=inverter_info, metric_config_bounds=base_data.metric_config_bounds, type=raw_inverter_type, ) @@ -1189,9 +1234,13 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=inverter_info, metric_config_bounds=base_data.metric_config_bounds, ) case ElectricalComponentCategory.POWER_TRANSFORMER: + power_transformer_info = _leftover_info( + base_data.category_specific_info, "primary", "secondary" + ) return PowerTransformer( id=base_data.component_id, microgrid_id=base_data.microgrid_id, @@ -1201,6 +1250,7 @@ def electrical_component_from_proto_with_issues( _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, _allow_construction=True, + category_specific_info=power_transformer_info, metric_config_bounds=base_data.metric_config_bounds, primary_voltage=message.category_specific_info.power_transformer.primary, secondary_voltage=message.category_specific_info.power_transformer.secondary, diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py b/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py index 1d611a6c..10dcbddf 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py @@ -60,7 +60,7 @@ def default_component_base_data( category=ElectricalComponentCategory.UNSPECIFIED, lifetime=DEFAULT_LIFETIME, metric_config_bounds={Metric.AC_ENERGY_ACTIVE: Bounds(lower=0, upper=100)}, - category_specific_info={}, + category_specific_info=None, provides_telemetry=True, accepts_control=True, category_mismatched=False, @@ -81,7 +81,6 @@ def assert_base_data( assert base_data.accepts_control == other._accepts_control # pylint: enable=protected-access assert base_data.metric_config_bounds == other.metric_config_bounds - assert base_data.category_specific_info == other.category_specific_metadata _OPERATIONAL_MODE_BY_BOOLS: dict[ diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py index e94a1a1b..34bf6843 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py @@ -15,6 +15,7 @@ from frequenz.client.common.metrics import Bounds, InvalidBounds, Metric from frequenz.client.common.microgrid import InvalidLifetime, Lifetime from frequenz.client.common.microgrid.electrical_components import ( + CategorySpecificInfo, ElectricalComponentCategory, ) from frequenz.client.common.microgrid.electrical_components.proto.v1alpha8._electrical_component import ( # noqa: E501 @@ -102,7 +103,7 @@ def test_missing_category_specific_info( category=ElectricalComponentCategory.UNSPECIFIED, lifetime=Lifetime(), metric_config_bounds={}, - category_specific_info={}, + category_specific_info=None, ) proto = base_data_as_proto(base_data) proto.ClearField("operational_lifetime") @@ -147,7 +148,9 @@ def test_category_specific_info_mismatch( minor_issues: list[str] = [] base_data = default_component_base_data._replace( category=ElectricalComponentCategory.GRID_CONNECTION_POINT, - category_specific_info={"type": "BATTERY_TYPE_LI_ION"}, + category_specific_info=CategorySpecificInfo( + kind="battery", fields={"type": "BATTERY_TYPE_LI_ION"} + ), category_mismatched=True, ) proto = base_data_as_proto(base_data) diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py index 2a6de99d..6ac3ba44 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py @@ -14,6 +14,7 @@ from frequenz.client.common.microgrid.electrical_components import ( Breaker, CapacitorBank, + CategorySpecificInfo, Chp, Converter, CryptoMiner, @@ -94,7 +95,9 @@ def test_category_mismatch( minor_issues: list[str] = [] base_data = default_component_base_data._replace( category=1, # GRID_CONNECTION_POINT - category_specific_info={"type": "BATTERY_TYPE_LI_ION"}, + category_specific_info=CategorySpecificInfo( + kind="battery", fields={"type": "BATTERY_TYPE_LI_ION"} + ), category_mismatched=True, ) proto = base_data_as_proto(base_data) @@ -113,6 +116,9 @@ def test_category_mismatch( assert not minor_issues assert isinstance(component, MismatchedCategoryElectricalComponent) assert_base_data(base_data, component) + assert component.category_specific_info == CategorySpecificInfo( + kind="battery", fields={"type": "BATTERY_TYPE_LI_ION"} + ) assert electrical_component_class_to_proto(component) == (1, None) diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py b/tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py index 8efcc6ac..b21cbb6e 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py @@ -11,6 +11,7 @@ ) from frequenz.client.common.microgrid.electrical_components import ( + CategorySpecificInfo, ElectricalComponentCategory, LiIonBattery, UnrecognizedBattery, @@ -87,6 +88,34 @@ def test_recognized_classes_carry_no_category_or_type( assert "type=" not in text +def test_recognized_class_keeps_info_kind_with_empty_fields( + default_component_base_data: _ElectricalComponentBaseData, +) -> None: + """A recognized component keeps the info kind with empty leftover fields.""" + battery = _li_ion_battery(default_component_base_data) + assert battery.category_specific_info == CategorySpecificInfo( + kind="battery", fields={} + ) + + +def test_unrecognized_category_preserves_info( + default_component_base_data: _ElectricalComponentBaseData, +) -> None: + """An unrecognized category preserves the carried info verbatim.""" + base_data = default_component_base_data._replace(category=999) + proto = base_data_as_proto(base_data) + proto.category_specific_info.battery.type = ( + electrical_components_pb2.BATTERY_TYPE_LI_ION + ) + + component = electrical_component_from_proto(proto) + + assert isinstance(component, UnrecognizedElectricalComponent) + assert component.category_specific_info == CategorySpecificInfo( + kind="battery", fields={"type": "BATTERY_TYPE_LI_ION"} + ) + + def test_unrecognized_type_shows_in_repr( default_component_base_data: _ElectricalComponentBaseData, ) -> None: diff --git a/tests/microgrid/electrical_components/test_electrical_component_base.py b/tests/microgrid/electrical_components/test_electrical_component_base.py index b04ad6e5..c626613c 100644 --- a/tests/microgrid/electrical_components/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/test_electrical_component_base.py @@ -25,6 +25,7 @@ MicrogridId, ) from frequenz.client.common.microgrid.electrical_components import ( + CategorySpecificInfo, ElectricalComponent, ElectricalComponentId, ) @@ -99,14 +100,14 @@ def test_creation_with_defaults() -> None: assert component.model == "Test Model" assert component.operational_lifetime == Lifetime() assert component.metric_config_bounds == {} - assert component.category_specific_metadata == {} + assert component.category_specific_info is None def test_creation_full() -> None: """Test electrical component creation with all attributes.""" bounds = Bounds(lower=-100.0, upper=100.0) metric_config_bounds: dict[Metric | int, Bounds] = {Metric.AC_POWER_ACTIVE: bounds} - metadata = {"key1": "value1", "key2": 42} + info = CategorySpecificInfo(kind="battery", fields={"key1": "value1", "key2": 42}) component = _TestElectricalComponent( id=ElectricalComponentId(1), @@ -114,7 +115,7 @@ def test_creation_full() -> None: name="test-component", model="Test Manufacturer Test Model", metric_config_bounds=metric_config_bounds, - category_specific_metadata=metadata, + category_specific_info=info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -123,7 +124,7 @@ def test_creation_full() -> None: assert component.name == "test-component" assert component.model == "Test Manufacturer Test Model" assert component.metric_config_bounds == metric_config_bounds - assert component.category_specific_metadata == metadata + assert component.category_specific_info == info def test_accessors_return_values_when_set() -> None: @@ -372,7 +373,9 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name="test", model="Test Model", metric_config_bounds={Metric.AC_POWER_ACTIVE: Bounds(lower=-100.0, upper=100.0)}, - category_specific_metadata={"key": "value"}, + category_specific_info=CategorySpecificInfo( + kind="battery", fields={"key": "value"} + ), _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -384,7 +387,7 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name=COMPONENT.name, model=COMPONENT.model, metric_config_bounds={Metric.AC_POWER_ACTIVE: Bounds(lower=-200.0, upper=200.0)}, - category_specific_metadata={"different": "metadata"}, + category_specific_info=COMPONENT.category_specific_info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -396,7 +399,7 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name="different", model=COMPONENT.model, metric_config_bounds=COMPONENT.metric_config_bounds, - category_specific_metadata=COMPONENT.category_specific_metadata, + category_specific_info=COMPONENT.category_specific_info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -408,7 +411,7 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name=COMPONENT.name, model=COMPONENT.model, metric_config_bounds=COMPONENT.metric_config_bounds, - category_specific_metadata=COMPONENT.category_specific_metadata, + category_specific_info=COMPONENT.category_specific_info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -420,7 +423,7 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name=COMPONENT.name, model=COMPONENT.model, metric_config_bounds=COMPONENT.metric_config_bounds, - category_specific_metadata=COMPONENT.category_specific_metadata, + category_specific_info=COMPONENT.category_specific_info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, @@ -432,7 +435,7 @@ def test_is_operational_now(mock_datetime: Mock) -> None: name=COMPONENT.name, model=COMPONENT.model, metric_config_bounds=COMPONENT.metric_config_bounds, - category_specific_metadata=COMPONENT.category_specific_metadata, + category_specific_info=COMPONENT.category_specific_info, _provides_telemetry=True, _accepts_control=True, _allow_construction=True, From 4839813e4e152d041768c35aeb6bae718b9276f1 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 21 Jul 2026 10:26:02 +0000 Subject: [PATCH 20/25] Rework `ElectricalComponent.__str__` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the `{id}<{Class}>{name}` rendering with `{id}:{name}:{Class}`, and override `__str__` on the problematic subclasses so the raw wire value that makes them problematic stays visible in the marker: * recognized components render as `CID42:bat1:LiIonBattery` (the name is always literal, so an empty name reads as `CID42::LiIonBattery`) * `UnrecognizedBattery`/`UnrecognizedEvCharger`/`UnrecognizedInverter` render as `CID42:bat1:Battery:type=99` — the hardcoded base label plus the raw type * `UnrecognizedElectricalComponent` renders as `CID42:comp1:category=999` — the raw, unrecognized category * `MismatchedCategoryElectricalComponent` renders as `CID42:comp1:mismatched:category=5:kind=inverter` — the declared category (as its enum name) against the carried variant kind. Each subclass overrides `__str__` in full rather than sharing a `__str__` hook on the base: the format is a single short f-string, the tests pin it down, and the flat overrides read better than the indirection a hook would need. Signed-off-by: Leandro Lucarella --- .../electrical_components/_battery.py | 4 ++ .../_electrical_component.py | 3 +- .../electrical_components/_ev_charger.py | 4 ++ .../electrical_components/_inverter.py | 4 ++ .../electrical_components/_problematic.py | 14 ++++++- .../electrical_components/test_battery.py | 18 +++++++++ .../test_electrical_component_base.py | 4 +- .../electrical_components/test_ev_charger.py | 18 +++++++++ .../electrical_components/test_inverter.py | 18 +++++++++ .../electrical_components/test_problematic.py | 38 +++++++++++++++++++ 10 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/frequenz/client/common/microgrid/electrical_components/_battery.py b/src/frequenz/client/common/microgrid/electrical_components/_battery.py index bddc90c8..99a8621f 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_battery.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_battery.py @@ -44,6 +44,10 @@ class UnrecognizedBattery(Battery, ProblematicElectricalComponent): type: int """The raw type of this battery, not recognized by this library version.""" + def __str__(self) -> str: + """Return a string representation exposing the raw type.""" + return f"{self.id}:{self.name}:Battery:type={self.type}" + BatteryTypes: TypeAlias = ( LiIonBattery | NaIonBattery | UnrecognizedBattery | UnspecifiedBattery diff --git a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py index 037f00f8..bbde0005 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -324,5 +324,4 @@ def identity(self) -> tuple[ElectricalComponentId, MicrogridId]: def __str__(self) -> str: """Return a human-readable string representation of this instance.""" - name = f":{self.name}" if self.name else "" - return f"{self.id}<{type(self).__name__}>{name}" + return f"{self.id}:{self.name}:{type(self).__name__}" diff --git a/src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py b/src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py index 5deaaa30..feb4d281 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py @@ -49,6 +49,10 @@ class UnrecognizedEvCharger(EvCharger, ProblematicElectricalComponent): type: int """The raw type of this EV charger, not recognized by this library version.""" + def __str__(self) -> str: + """Return a string representation exposing the raw type.""" + return f"{self.id}:{self.name}:EvCharger:type={self.type}" + EvChargerTypes: TypeAlias = ( UnspecifiedEvCharger diff --git a/src/frequenz/client/common/microgrid/electrical_components/_inverter.py b/src/frequenz/client/common/microgrid/electrical_components/_inverter.py index 260b3304..d4cb7db5 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_inverter.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_inverter.py @@ -49,6 +49,10 @@ class UnrecognizedInverter(Inverter, ProblematicElectricalComponent): type: int """The raw type of this inverter, not recognized by this library version.""" + def __str__(self) -> str: + """Return a string representation exposing the raw type.""" + return f"{self.id}:{self.name}:Inverter:type={self.type}" + InverterTypes: TypeAlias = ( UnspecifiedInverter diff --git a/src/frequenz/client/common/microgrid/electrical_components/_problematic.py b/src/frequenz/client/common/microgrid/electrical_components/_problematic.py index 91f1e176..10ec6318 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_problematic.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_problematic.py @@ -37,17 +37,27 @@ class UnrecognizedElectricalComponent(ProblematicElectricalComponent): category: int """The raw category of this component, not recognized by this library version.""" + def __str__(self) -> str: + """Return a string representation exposing the raw category.""" + return f"{self.id}:{self.name}:category={self.category}" + @dataclasses.dataclass(frozen=True, kw_only=True) class MismatchedCategoryElectricalComponent(ProblematicElectricalComponent): """An electrical component with a mismatch in the category. This electrical component declared a category but carries category specific - metadata that doesn't match the declared category. + info that doesn't match the declared category. """ category: int """The raw category declared by this component. - It doesn't match the carried category specific metadata. + It doesn't match the carried category specific info. """ + + def __str__(self) -> str: + """Return a string representation exposing the category mismatch.""" + info = self.category_specific_info + kind = info.kind if info is not None else "" + return f"{self.id}:{self.name}:mismatched:category={self.category}:kind={kind}" diff --git a/tests/microgrid/electrical_components/test_battery.py b/tests/microgrid/electrical_components/test_battery.py index 6cca936b..badea6e7 100644 --- a/tests/microgrid/electrical_components/test_battery.py +++ b/tests/microgrid/electrical_components/test_battery.py @@ -147,3 +147,21 @@ def test_recognized_battery_types_are_not_problematic( assert not isinstance(battery, ProblematicElectricalComponent) assert isinstance(battery, Battery) + + +def test_unrecognized_battery_str( + component_id: ElectricalComponentId, microgrid_id: MicrogridId +) -> None: + """`UnrecognizedBattery.__str__` exposes the raw type after the base label.""" + battery = UnrecognizedBattery( + id=component_id, + microgrid_id=microgrid_id, + name="bat1", + model="Test Model", + type=999, + _provides_telemetry=True, + _accepts_control=True, + _allow_construction=True, + ) + + assert str(battery) == "CID42:bat1:Battery:type=999" diff --git a/tests/microgrid/electrical_components/test_electrical_component_base.py b/tests/microgrid/electrical_components/test_electrical_component_base.py index c626613c..c75edc0e 100644 --- a/tests/microgrid/electrical_components/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/test_electrical_component_base.py @@ -297,8 +297,8 @@ def test_get_metric_config_bounds_invalid_raises_despite_default() -> None: @pytest.mark.parametrize( "name,expected_str", [ - ("", "CID1<_TestElectricalComponent>"), - ("test-component", "CID1<_TestElectricalComponent>:test-component"), + ("", "CID1::_TestElectricalComponent"), + ("test-component", "CID1:test-component:_TestElectricalComponent"), ], ids=["no-name", "with-name"], ) diff --git a/tests/microgrid/electrical_components/test_ev_charger.py b/tests/microgrid/electrical_components/test_ev_charger.py index b1a50e1a..9d480c34 100644 --- a/tests/microgrid/electrical_components/test_ev_charger.py +++ b/tests/microgrid/electrical_components/test_ev_charger.py @@ -148,3 +148,21 @@ def test_recognized_ev_charger_types_are_not_problematic( assert not isinstance(charger, ProblematicElectricalComponent) assert isinstance(charger, EvCharger) + + +def test_unrecognized_ev_charger_str( + component_id: ElectricalComponentId, microgrid_id: MicrogridId +) -> None: + """`UnrecognizedEvCharger.__str__` exposes the raw type after the base label.""" + charger = UnrecognizedEvCharger( + id=component_id, + microgrid_id=microgrid_id, + name="evc1", + model="Test Model", + type=999, + _provides_telemetry=True, + _accepts_control=True, + _allow_construction=True, + ) + + assert str(charger) == "CID42:evc1:EvCharger:type=999" diff --git a/tests/microgrid/electrical_components/test_inverter.py b/tests/microgrid/electrical_components/test_inverter.py index 92a1b261..3207c56a 100644 --- a/tests/microgrid/electrical_components/test_inverter.py +++ b/tests/microgrid/electrical_components/test_inverter.py @@ -148,3 +148,21 @@ def test_recognized_inverter_types_are_not_problematic( assert not isinstance(inverter, ProblematicElectricalComponent) assert isinstance(inverter, Inverter) + + +def test_unrecognized_inverter_str( + component_id: ElectricalComponentId, microgrid_id: MicrogridId +) -> None: + """`UnrecognizedInverter.__str__` exposes the raw type after the base label.""" + inverter = UnrecognizedInverter( + id=component_id, + microgrid_id=microgrid_id, + name="inv1", + model="Test Model", + type=999, + _provides_telemetry=True, + _accepts_control=True, + _allow_construction=True, + ) + + assert str(inverter) == "CID42:inv1:Inverter:type=999" diff --git a/tests/microgrid/electrical_components/test_problematic.py b/tests/microgrid/electrical_components/test_problematic.py index c814ff26..fb32a92e 100644 --- a/tests/microgrid/electrical_components/test_problematic.py +++ b/tests/microgrid/electrical_components/test_problematic.py @@ -7,6 +7,7 @@ from frequenz.client.common.microgrid import MicrogridId from frequenz.client.common.microgrid.electrical_components import ( + CategorySpecificInfo, ElectricalComponentId, MismatchedCategoryElectricalComponent, ProblematicElectricalComponent, @@ -126,3 +127,40 @@ def test_unrecognized_component_type( assert component.microgrid_id == microgrid_id assert component.name == "unrecognized_component" assert component.category == 999 + + +def test_unrecognized_component_str( + component_id: ElectricalComponentId, microgrid_id: MicrogridId +) -> None: + """`UnrecognizedElectricalComponent.__str__` exposes the raw category.""" + component = UnrecognizedElectricalComponent( + id=component_id, + microgrid_id=microgrid_id, + name="comp1", + model="Test Model", + category=999, + _provides_telemetry=True, + _accepts_control=True, + _allow_construction=True, + ) + + assert str(component) == "CID42:comp1:category=999" + + +def test_mismatched_category_component_str( + component_id: ElectricalComponentId, microgrid_id: MicrogridId +) -> None: + """`MismatchedCategoryElectricalComponent.__str__` exposes the mismatch.""" + component = MismatchedCategoryElectricalComponent( + id=component_id, + microgrid_id=microgrid_id, + name="comp1", + model="Test Model", + category=5, # BATTERY + category_specific_info=CategorySpecificInfo(kind="inverter", fields={}), + _provides_telemetry=True, + _accepts_control=True, + _allow_construction=True, + ) + + assert str(component) == "CID42:comp1:mismatched:category=5:kind=inverter" From baa37c585343c5711bfc34c8f5b49adf07710963 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 21 Jul 2026 10:44:32 +0000 Subject: [PATCH 21/25] Store the declared category name on mismatched components `MismatchedCategoryElectricalComponent.__str__` could only show the declared category as its raw int (`category=5`), because resolving the name would mean using the deprecated `ElectricalComponentCategory` wrapper enum. Store a `category_name` alongside the raw `category`, resolved by the converter from the protobuf enum descriptor (which is not deprecated) with the long `ELECTRICAL_COMPONENT_CATEGORY_` prefix stripped, so `__str__` renders the short, readable name: `CID42:comp1:mismatched:category=BATTERY:kind=inverter` Only the mismatched component needs it: its declared category is a recognized value that its class name hides. Unrecognized components have no name to show (that is what makes them unrecognized), and every other component encodes its category in its class name. Signed-off-by: Leandro Lucarella --- .../electrical_components/_problematic.py | 11 ++++++++- .../proto/v1alpha8/_electrical_component.py | 23 +++++++++++++++++++ .../test_electrical_component_simple.py | 1 + .../electrical_components/test_problematic.py | 3 ++- 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/microgrid/electrical_components/_problematic.py b/src/frequenz/client/common/microgrid/electrical_components/_problematic.py index 10ec6318..8ffa03fb 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_problematic.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_problematic.py @@ -56,8 +56,17 @@ class MismatchedCategoryElectricalComponent(ProblematicElectricalComponent): It doesn't match the carried category specific info. """ + category_name: str | None = None + """The short protobuf name of the declared category, or `None` if unknown. + + This is the protobuf enum name without its long prefix (e.g. `"BATTERY"`). + It is normally set, since a mismatched component declares a recognized + category. + """ + def __str__(self) -> str: """Return a string representation exposing the category mismatch.""" info = self.category_specific_info kind = info.kind if info is not None else "" - return f"{self.id}:{self.name}:mismatched:category={self.category}:kind={kind}" + category = self.category_name or self.category + return f"{self.id}:{self.name}:mismatched:category={category}:kind={kind}" diff --git a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py index e004f46a..bdb65ba3 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py @@ -914,6 +914,28 @@ class _ElectricalComponentBaseData(NamedTuple): """Whether the declared category and the carried info disagree.""" +_CATEGORY_NAME_PREFIX = "ELECTRICAL_COMPONENT_CATEGORY_" + + +def _category_name(category: int) -> str | None: + """Return the short protobuf enum name for a category, or `None` if unknown. + + Args: + category: The raw protobuf category value. + + Returns: + The protobuf enum name without its `ELECTRICAL_COMPONENT_CATEGORY_` + prefix (e.g. `"BATTERY"`), or `None` when the value is not a known + protobuf enum value. + """ + proto_enum = electrical_components_pb2.ElectricalComponentCategory + try: + name = proto_enum.Name(proto_enum.ValueType(category)) + except ValueError: + return None + return name.removeprefix(_CATEGORY_NAME_PREFIX) + + def _leftover_info( info: CategorySpecificInfo | None, *translated_keys: str ) -> CategorySpecificInfo | None: @@ -1046,6 +1068,7 @@ def electrical_component_from_proto_with_issues( name=base_data.name, model=base_data.model, category=message.category, + category_name=_category_name(message.category), operational_lifetime=base_data.lifetime, _provides_telemetry=base_data.provides_telemetry, _accepts_control=base_data.accepts_control, diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py index 6ac3ba44..0d04d921 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py @@ -119,6 +119,7 @@ def test_category_mismatch( assert component.category_specific_info == CategorySpecificInfo( kind="battery", fields={"type": "BATTERY_TYPE_LI_ION"} ) + assert component.category_name == "GRID_CONNECTION_POINT" assert electrical_component_class_to_proto(component) == (1, None) diff --git a/tests/microgrid/electrical_components/test_problematic.py b/tests/microgrid/electrical_components/test_problematic.py index fb32a92e..df8704fd 100644 --- a/tests/microgrid/electrical_components/test_problematic.py +++ b/tests/microgrid/electrical_components/test_problematic.py @@ -157,10 +157,11 @@ def test_mismatched_category_component_str( name="comp1", model="Test Model", category=5, # BATTERY + category_name="BATTERY", category_specific_info=CategorySpecificInfo(kind="inverter", fields={}), _provides_telemetry=True, _accepts_control=True, _allow_construction=True, ) - assert str(component) == "CID42:comp1:mismatched:category=5:kind=inverter" + assert str(component) == "CID42:comp1:mismatched:category=BATTERY:kind=inverter" From eae39a583dcb9ed9dcb48c8c14d104ae4828f68e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:43:29 +0000 Subject: [PATCH 22/25] Standardize the invalid marker in `MetricConnection.__str__` Render `` when the category is unspecified (the raw `0` or the deprecated `UNSPECIFIED` member). Unknown non-zero ints keep rendering bare: they are forward-compatible values this client version doesn't know yet, not invariant violations. The format is slightly changed to make it more familiar with other `__str__` representations: * `None`/`""` are not merged and kept literal * `` -> bare `...` for enum values and `:cat=...` for `int` values for compactness Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 20 +++++++++++-------- .../metrics/test_sample_metric_connection.py | 20 +++++++++++++++---- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index a22fc409..1a69393f 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -128,14 +128,18 @@ class MetricConnection: def __str__(self) -> str: """Return a string representation of this connection.""" - category_name = ( - str(self.category) - if isinstance(self.category, int) - else f"" - ) - if self.name: - return f"{category_name}({self.name})" - return category_name + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=DeprecationWarning) + match self.category: + case 0 | MetricConnectionCategory.UNSPECIFIED: + category_name = "cat=" + case MetricConnectionCategory() as category: + category_name = category.name + case int() as category: + category_name = f"cat={category}" + case unexpected: + assert_never(unexpected) + return f"{self.name}:{category_name}" def get_category(self) -> MetricConnectionCategory: """Return the connection category as a known enum member. diff --git a/tests/metrics/test_sample_metric_connection.py b/tests/metrics/test_sample_metric_connection.py index d6888c9a..1b2cfe1b 100644 --- a/tests/metrics/test_sample_metric_connection.py +++ b/tests/metrics/test_sample_metric_connection.py @@ -18,27 +18,39 @@ pytest.param( MetricConnectionCategory.BATTERY, "", - "", + ":BATTERY", id="enum_category_empty_name", ), pytest.param( MetricConnectionCategory.PV, "dc_pv_0", - "(dc_pv_0)", + "dc_pv_0:PV", id="enum_category_with_name", ), pytest.param( 999, "", - "999", + ":cat=999", id="int_category_empty_name", ), pytest.param( 999, "unknown_connection", - "999(unknown_connection)", + "unknown_connection:cat=999", id="int_category_with_name", ), + pytest.param( + 0, + "", + ":cat=", + id="unspecified_int_empty_name", + ), + pytest.param( + 0, + "conn", + "conn:cat=", + id="unspecified_int_with_name", + ), ], ) def test_str_representation( From 238b033a85728c841db46c8ceab3f4c8e2da33ac Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:51:00 +0000 Subject: [PATCH 23/25] Add `MetricSample.__str__` `MetricSample` had no `__str__`, so it fell back to the verbose dataclass `__repr__`. Add a compact one rendering `{metric}={value}`, with `@{connection}` appended when a connection is set. The metric follows the same convention as `MetricConnection.__str__`: a known member renders as its name, the unspecified sentinel (`0` / `UNSPECIFIED`) as ``, and an unknown int bare. `MetricSample` is the `instance` reported by `get_metric()` (`Unrecognized` / `UnspecifiedEnumValueError`) and `get_bounds_set()` (`InvalidBoundsSetError`); without a `__str__` those messages embedded the whole dataclass repr. A short representation keeps the errors readable. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 18 ++++++ tests/metrics/test_sample_metric_sample.py | 61 +++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index 1a69393f..0d774a04 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -290,6 +290,24 @@ def __init__( object.__setattr__(self, "bounds_set", bounds_set) object.__setattr__(self, "connection", connection) + def __str__(self) -> str: + """Return a compact string representation of this sample.""" + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=DeprecationWarning) + match self.metric: + case 0 | Metric.UNSPECIFIED: + metric = "" + case Metric() as known: + metric = known.name + case int() as unknown: + metric = str(unknown) + case unexpected: + assert_never(unexpected) + sample = f"{metric}={self.value}" + if self.connection is not None: + sample = f"{sample}@{self.connection}" + return sample + @property @deprecated("`MetricSample.bounds` is deprecated; use `bounds_set` instead.") def bounds(self) -> list[Bounds]: diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index 4d255b73..69f0defd 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -22,6 +22,7 @@ InvalidBoundsSetError, Metric, MetricConnection, + MetricConnectionCategory, MetricSample, ) @@ -78,6 +79,66 @@ def test_creation( assert sample.connection == connection +@pytest.mark.parametrize( + "metric, value, connection, expected", + [ + pytest.param( + Metric.AC_POWER_ACTIVE, + 5.0, + None, + "AC_POWER_ACTIVE=5.0", + id="known_metric", + ), + pytest.param( + Metric.AC_POWER_ACTIVE, + 5.0, + MetricConnection( + category=MetricConnectionCategory.BATTERY, name="dc_battery_0" + ), + "AC_POWER_ACTIVE=5.0@dc_battery_0:BATTERY", + id="with_connection", + ), + pytest.param( + 0, + None, + None, + "=None", + id="unspecified_metric", + ), + pytest.param( + 99999, + 42, + None, + "99999=42", + id="unrecognized_metric", + ), + pytest.param( + Metric.AC_POWER_ACTIVE, + AggregatedMetricValue(avg=5.0, min=1.0, max=10.0, raw=[1.0, 5.0, 10.0]), + None, + "AC_POWER_ACTIVE=avg:5.0", + id="aggregated_value", + ), + ], +) +def test_str( + now: datetime, + metric: Metric | int, + value: FloatInt | AggregatedMetricValue | None, + connection: MetricConnection | None, + expected: str, +) -> None: + """`MetricSample.__str__` renders a compact `metric=value` summary.""" + sample = MetricSample( + sample_time=now, + metric=metric, + value=value, + bounds_set=BoundsSet(), + connection=connection, + ) + assert str(sample) == expected + + @pytest.mark.parametrize( "value, method_results", [ From e4d64f295446885a98765c431eec62d7437e26f8 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:54:28 +0000 Subject: [PATCH 24/25] Use `str` instead of `repr` for values error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The default (and custom) messages of the accessor errors interpolated the offending value with `!r`. Switch the non-string values to plain `{value}` so wrapper values render through their compact `__str__` — e.g. `` instead of `InvalidBounds(lower=10.0, upper=-10.0)` — which surfaces the `` markers in the message and keeps it readable. Covered: `UnrecognizedEnumValueError`, `InvalidLatitudeError`, `InvalidLongitudeError`, `InvalidLifetimeError`, `InvalidBoundsError`, `InvalidBoundsSetError` and `InvalidDeliveryAreaError`, plus the custom messages raised by `ElectricalComponent.provides_telemetry()`, `accepts_control()` and `get_metric_config_bounds()`, and `Microgrid.is_active()`. String values keep `!r`: an invalid `country_code` may carry surprising characters, and quoting keeps them visible. The `attr_name` likewise keeps `!r`, the idiomatic way to show an attribute name. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/_exception.py | 2 +- src/frequenz/client/common/grid/_delivery_area.py | 2 +- src/frequenz/client/common/metrics/_bounds.py | 4 ++-- src/frequenz/client/common/microgrid/_lifetime.py | 2 +- src/frequenz/client/common/microgrid/_microgrid.py | 2 +- .../electrical_components/_electrical_component.py | 6 +++--- src/frequenz/client/common/types/_location.py | 4 ++-- .../grid/_delivery_area/test_invalid_delivery_area_error.py | 2 +- tests/metrics/_bounds/test_invalid_bounds_error.py | 2 +- tests/metrics/_bounds/test_invalid_bounds_set_error.py | 2 +- tests/microgrid/_lifetime/test_invalid_lifetime_error.py | 2 +- 11 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/frequenz/client/common/_exception.py b/src/frequenz/client/common/_exception.py index dd7317b4..54313175 100644 --- a/src/frequenz/client/common/_exception.py +++ b/src/frequenz/client/common/_exception.py @@ -70,7 +70,7 @@ def __init__( ( message if message is not None - else f"unrecognized enum value {value!r} for attribute {attr_name!r} in {instance}" + else f"unrecognized enum value {value} for attribute {attr_name!r} in {instance}" ), ) diff --git a/src/frequenz/client/common/grid/_delivery_area.py b/src/frequenz/client/common/grid/_delivery_area.py index 578476ae..c51ba40e 100644 --- a/src/frequenz/client/common/grid/_delivery_area.py +++ b/src/frequenz/client/common/grid/_delivery_area.py @@ -274,7 +274,7 @@ def __init__( """The invalid delivery area instance that caused this error.""" message = ( - f"invalid delivery area {delivery_area!r} for attribute {attr_name!r} in {instance}" + f"invalid delivery area {delivery_area} for attribute {attr_name!r} in {instance}" if message is None else message ) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 5b051b1d..8f9fe3f7 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -169,7 +169,7 @@ def __init__( ( message if message is not None - else f"invalid bounds {bounds!r} for attribute {attr_name!r} in {instance}" + else f"invalid bounds {bounds} for attribute {attr_name!r} in {instance}" ), ) @@ -419,6 +419,6 @@ def __init__( ( message if message is not None - else f"invalid bounds set {bounds_set!r} for attribute {attr_name!r} in {instance}" + else f"invalid bounds set {bounds_set} for attribute {attr_name!r} in {instance}" ), ) diff --git a/src/frequenz/client/common/microgrid/_lifetime.py b/src/frequenz/client/common/microgrid/_lifetime.py index e28c5736..c35bce31 100644 --- a/src/frequenz/client/common/microgrid/_lifetime.py +++ b/src/frequenz/client/common/microgrid/_lifetime.py @@ -150,6 +150,6 @@ def __init__( ( message if message is not None - else f"invalid lifetime {lifetime!r} for attribute {attr_name!r} in {instance}" + else f"invalid lifetime {lifetime} for attribute {attr_name!r} in {instance}" ), ) diff --git a/src/frequenz/client/common/microgrid/_microgrid.py b/src/frequenz/client/common/microgrid/_microgrid.py index 799c76fc..bac17238 100644 --- a/src/frequenz/client/common/microgrid/_microgrid.py +++ b/src/frequenz/client/common/microgrid/_microgrid.py @@ -117,7 +117,7 @@ def is_active(self) -> bool: self, "_active", value, - f"unrecognized status of microgrid {self}: {value!r}", + f"unrecognized status of microgrid {self}: {value}", ) case unknown: assert_never(unknown) diff --git a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py index bbde0005..f2680ab1 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -162,7 +162,7 @@ def provides_telemetry(self) -> bool: self, "_provides_telemetry", value, - f"operational mode {value!r} of {self} is not a recognized " + f"operational mode {value} of {self} is not a recognized " "ElectricalComponentOperationalMode; telemetry availability " "is unknown", ) @@ -195,7 +195,7 @@ def accepts_control(self) -> bool: self, "_accepts_control", value, - f"operational mode {value!r} of {self} is not a recognized " + f"operational mode {value} of {self} is not a recognized " "ElectricalComponentOperationalMode; control availability " "is unknown", ) @@ -257,7 +257,7 @@ def get_metric_config_bounds( self, "metric_config_bounds", invalid, - f"invalid bounds {invalid!r} for metric {metric} in {self}", + f"invalid bounds {invalid} for metric {metric} in {self}", ) case Bounds() as valid: return valid diff --git a/src/frequenz/client/common/types/_location.py b/src/frequenz/client/common/types/_location.py index bd946ed8..248066e0 100644 --- a/src/frequenz/client/common/types/_location.py +++ b/src/frequenz/client/common/types/_location.py @@ -44,7 +44,7 @@ def __init__( ( message if message is not None - else f"invalid latitude {value!r} for attribute {attr_name!r} in " + else f"invalid latitude {value} for attribute {attr_name!r} in " f"{instance}; must be in [-90, 90]" ), ) @@ -84,7 +84,7 @@ def __init__( ( message if message is not None - else f"invalid longitude {value!r} for attribute {attr_name!r} in " + else f"invalid longitude {value} for attribute {attr_name!r} in " f"{instance}; must be in [-180, 180]" ), ) diff --git a/tests/grid/_delivery_area/test_invalid_delivery_area_error.py b/tests/grid/_delivery_area/test_invalid_delivery_area_error.py index 9bb7be28..cb1c205d 100644 --- a/tests/grid/_delivery_area/test_invalid_delivery_area_error.py +++ b/tests/grid/_delivery_area/test_invalid_delivery_area_error.py @@ -15,7 +15,7 @@ def test_default_message() -> None: error = InvalidDeliveryAreaError("some-instance", "delivery_area", invalid) assert error.delivery_area is invalid assert ( - "invalid delivery area InvalidDeliveryArea(code='', code_type=0) for " + f"invalid delivery area {invalid} for " "attribute 'delivery_area' in some-instance" == str(error) ) diff --git a/tests/metrics/_bounds/test_invalid_bounds_error.py b/tests/metrics/_bounds/test_invalid_bounds_error.py index cd12e742..80e06499 100644 --- a/tests/metrics/_bounds/test_invalid_bounds_error.py +++ b/tests/metrics/_bounds/test_invalid_bounds_error.py @@ -14,7 +14,7 @@ def test_default_message() -> None: assert error.bounds is invalid assert ( - str(error) == f"invalid bounds {invalid!r} for attribute 'config_bounds' " + str(error) == f"invalid bounds {invalid} for attribute 'config_bounds' " "in some-instance" ) diff --git a/tests/metrics/_bounds/test_invalid_bounds_set_error.py b/tests/metrics/_bounds/test_invalid_bounds_set_error.py index 6f44eac4..d1d62da0 100644 --- a/tests/metrics/_bounds/test_invalid_bounds_set_error.py +++ b/tests/metrics/_bounds/test_invalid_bounds_set_error.py @@ -21,7 +21,7 @@ def test_default_message() -> None: assert error.bounds_set is invalid assert ( - str(error) == f"invalid bounds set {invalid!r} for attribute 'bounds_set' " + str(error) == f"invalid bounds set {invalid} for attribute 'bounds_set' " "in some-instance" ) diff --git a/tests/microgrid/_lifetime/test_invalid_lifetime_error.py b/tests/microgrid/_lifetime/test_invalid_lifetime_error.py index e8e56f75..9a917266 100644 --- a/tests/microgrid/_lifetime/test_invalid_lifetime_error.py +++ b/tests/microgrid/_lifetime/test_invalid_lifetime_error.py @@ -17,7 +17,7 @@ def test_default_message(present: datetime, future: datetime) -> None: assert error.lifetime is invalid assert ( str(error) - == f"invalid lifetime {invalid!r} for attribute 'operational_lifetime' " + == f"invalid lifetime {invalid} for attribute 'operational_lifetime' " "in some-instance" ) From 515fed5b28ac4f4305ad4a64539adc4757ce911e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 20 Jul 2026 12:56:20 +0000 Subject: [PATCH 25/25] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index f37b2067..6f2a78d1 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -62,6 +62,12 @@ * `frequenz.client.common.metrics.Bounds.__str__` now renders as `[lower,upper]` (no space after the comma) to match the compact format used by `Lifetime` and to compose cleanly with the `` marker on `InvalidBounds`. +* Several `__str__` representations were standardized around the `` marker, so a `grep '`, and unknown non-zero categories render as `cat=`. + * `frequenz.client.common.metrics.MetricSample` gained a compact `__str__` (`metric=value`, plus `@connection` when a connection is set) instead of falling back to the dataclass `repr`. + * `UnrecognizedElectricalComponent`, `MismatchedCategoryElectricalComponent`, `UnrecognizedBattery`, `UnrecognizedEvCharger` and `UnrecognizedInverter` now expose their raw wire `category` / `type` in `__str__` (e.g. `CID1:comp1:Inverter:type=99`), instead of hiding it behind the class name alone. These values are merely unrecognized (forward-compatible), not invariant violations, so they use a plain `:field=value` detail rather than the `` marker. + * `frequenz.client.common.metrics.MetricSample.bounds` is now deprecated; use `bounds_set` instead. The field type changed from `list[Bounds]` to `BoundsSet | InvalidBoundsSet` (see New Features). Reads and construction remain backward compatible: passing the `bounds=` keyword argument still works (it builds a `BoundsSet` and emits a `DeprecationWarning`), and reading `MetricSample.bounds` still returns the valid `Bounds` as a `list` (also emitting a `DeprecationWarning`). The compatibility property returns only the valid, normalized bounds, so it may differ from the raw wire list when bounds overlapped or touched.