From 858edabe82d8d3eb8028b4fee889af64e0246aca Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:39:27 +0000 Subject: [PATCH 01/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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.