From 02664ffe11e57cc1ca680c142d7ba64a7897f8d5 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 13 Jul 2026 13:04:28 +0200 Subject: [PATCH 01/26] Stop logging issues in `microgrid_from_proto()` Now all issues in a `Microgrid` object loaded from a protobuf message are reported "on-demand" via code, so users can decide how to deal with errors, so there is no need to report them via log warnings anymore. Signed-off-by: Leandro Lucarella --- .../microgrid/proto/v1alpha8/_microgrid.py | 26 +--------------- .../proto/v1alpha8/test_microgrid.py | 30 +------------------ 2 files changed, 2 insertions(+), 54 deletions(-) diff --git a/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py b/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py index ff220472..5a7ae505 100644 --- a/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py +++ b/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py @@ -3,8 +3,6 @@ """Loading of Microgrid objects from protobuf messages.""" -import logging - from frequenz.api.common.v1alpha8.microgrid import microgrid_pb2 from ....grid import DeliveryArea, InvalidDeliveryArea @@ -15,9 +13,6 @@ from ..._ids import EnterpriseId, MicrogridId from ..._microgrid import Microgrid -_logger = logging.getLogger(__name__) - - _ACTIVE_BY_STATUS: dict[int, bool] = { microgrid_pb2.MICROGRID_STATUS_ACTIVE: True, microgrid_pb2.MICROGRID_STATUS_INACTIVE: False, @@ -49,32 +44,13 @@ def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid: Returns: The corresponding [`Microgrid`][....Microgrid] object. """ - major_issues: list[str] = [] - delivery_area: DeliveryArea | InvalidDeliveryArea | None = None if message.HasField("delivery_area"): delivery_area = delivery_area_from_proto2(message.delivery_area) - else: - major_issues.append("delivery_area is missing") location: Location | None = None if message.HasField("location"): location = location_from_proto(message.location) - else: - major_issues.append("location is missing") - - active = _microgrid_status_to_active(message.status) - if message.status == microgrid_pb2.MICROGRID_STATUS_UNSPECIFIED: - major_issues.append("status is unspecified") - elif not isinstance(active, bool): - major_issues.append("status is unrecognized") - - if major_issues: - _logger.warning( - "Found issues in microgrid: %s | Protobuf message:\n%s", - ", ".join(major_issues), - message, - ) # The wrapper uses create_time, but the protobuf field remains create_timestamp. return Microgrid( @@ -84,6 +60,6 @@ def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid: delivery_area=delivery_area, location=location, create_time=datetime_from_proto(message.create_timestamp), - _active=active, + _active=_microgrid_status_to_active(message.status), _allow_construction=True, ) diff --git a/tests/microgrid/proto/v1alpha8/test_microgrid.py b/tests/microgrid/proto/v1alpha8/test_microgrid.py index 49487044..2297df3e 100644 --- a/tests/microgrid/proto/v1alpha8/test_microgrid.py +++ b/tests/microgrid/proto/v1alpha8/test_microgrid.py @@ -47,9 +47,6 @@ class _ProtoConversionTestCase: is unspecified, or any other raw `int` when the status is unrecognized. """ - expected_log: tuple[str, str] | None = None - """Whether to expect a log during conversion (level, message).""" - def _assert_active(info: Microgrid, expected_active: bool | int) -> None: """Assert that ``info._active`` matches ``expected_active`` and the accessor agrees.""" @@ -94,10 +91,6 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: has_name=True, status=microgrid_pb2.MICROGRID_STATUS_ACTIVE, expected_active=True, - expected_log=( - "WARNING", - "Found issues in microgrid: delivery_area is missing", - ), ), _ProtoConversionTestCase( name="no_location", @@ -106,7 +99,6 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: has_name=True, status=microgrid_pb2.MICROGRID_STATUS_ACTIVE, expected_active=True, - expected_log=("WARNING", "Found issues in microgrid: location is missing"), ), _ProtoConversionTestCase( name="empty_name", @@ -123,10 +115,6 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: has_name=True, status=microgrid_pb2.MICROGRID_STATUS_UNSPECIFIED, expected_active=0, - expected_log=( - "WARNING", - "Found issues in microgrid: status is unspecified", - ), ), _ProtoConversionTestCase( name="unrecognized_status", @@ -135,10 +123,6 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: has_name=True, status=999, # Unknown status value expected_active=999, - expected_log=( - "WARNING", - "Found issues in microgrid: status is unrecognized", - ), ), ], ids=lambda case: case.name, @@ -148,12 +132,10 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: ) @patch("frequenz.client.common.microgrid.proto.v1alpha8._microgrid.location_from_proto") @patch("frequenz.client.common.microgrid.proto.v1alpha8._microgrid.datetime_from_proto") -# pylint: disable-next=too-many-arguments,too-many-positional-arguments,too-many-branches def test_from_proto( mock_datetime_from_proto: Mock, mock_location_from_proto: Mock, mock_delivery_area_from_proto: Mock, - caplog: pytest.LogCaptureFixture, case: _ProtoConversionTestCase, ) -> None: """Test conversion from protobuf message to Microgrid.""" @@ -201,8 +183,7 @@ def test_from_proto( proto.location.country_code = "DE" # Run the conversion - with caplog.at_level("DEBUG"): - info = microgrid_from_proto(proto) + info = microgrid_from_proto(proto) # Verify the result assert info.id == MicrogridId(1234) @@ -232,12 +213,3 @@ def test_from_proto( else: mock_location_from_proto.assert_not_called() assert info.location is None - - # Verify logging behavior - if case.expected_log: - expected_level, expected_message = case.expected_log - assert len(caplog.records) == 1 - assert caplog.records[0].levelname == expected_level - assert expected_message in caplog.records[0].message - else: - assert len(caplog.records) == 0 From 2806358d7427c4e0a139ad7002b049814a8398a1 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:48:16 +0000 Subject: [PATCH 02/26] Add `BaseBounds` abstract base class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a `BaseBounds` frozen dataclass that will become the common supertype of the well-formed `Bounds` and the malformed variants added later in this series. It carries the `lower` and `upper` fields with their attribute docstrings, mirrors the `BaseLifetime` pattern (an abstract dataclass guarded by `__new__` against direct instantiation), and is exported from `frequenz.client.common.metrics`. `Bounds` itself is not touched yet — the field lift and the inheritance relationship come in a follow-up commit — so that this step reads as a pure addition on its own. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 3 +- src/frequenz/client/common/metrics/_bounds.py | 29 +++++++++++++++++++ tests/metrics/test_bounds.py | 8 ++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index 45a2bbba..7e6c88f8 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 Bounds +from ._bounds import BaseBounds, Bounds from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -16,6 +16,7 @@ __all__ = [ "AggregatedMetricValue", "AggregationMethod", + "BaseBounds", "Bounds", "Metric", "MetricConnection", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index de59bde5..4f3f6487 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -5,6 +5,35 @@ """Definitions for bounds.""" import dataclasses +from typing import Any, Self + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class BaseBounds: + """A base class for well-formed and malformed metric bounds. + + This class cannot be instantiated directly. Use [`Bounds`][..Bounds] for a + valid pair of bounds. + """ + + lower: float | int | None = None + """The lower bound. + + If `None`, there is no lower bound. + """ + + upper: float | int | None = None + """The upper bound. + + If `None`, there is no upper bound. + """ + + # pylint: disable-next=unused-argument + def __new__(cls, *args: Any, **kwargs: Any) -> Self: + """Prevent instantiation of this class.""" + if cls is BaseBounds: + raise TypeError(f"Cannot instantiate {cls.__name__} directly") + return super().__new__(cls) @dataclasses.dataclass(frozen=True, kw_only=True) diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index f33b22de..66fd1c8b 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -7,7 +7,13 @@ import pytest -from frequenz.client.common.metrics import Bounds +from frequenz.client.common.metrics import BaseBounds, Bounds + + +def test_base_bounds_cannot_be_instantiated_directly() -> None: + """`BaseBounds` refuses direct instantiation.""" + with pytest.raises(TypeError, match="Cannot instantiate BaseBounds directly"): + BaseBounds() @pytest.mark.parametrize( From 59a1e0f6937929b3b3de373bb8b00ed97bae7648 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:49:30 +0000 Subject: [PATCH 03/26] Make `Bounds` inherit from `BaseBounds` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lift the `lower` and `upper` fields off `Bounds` — they now come from `BaseBounds` as `float | int | None` — and declare the subclass relationship explicitly. The `__post_init__` invariant and the current `__str__` are unchanged; the only behavioral effect is that `isinstance(Bounds(...), BaseBounds)` now returns `True`, matching what subsequent commits rely on for the `Bounds | InvalidBounds` union. The class docstring gains a `Note:` spelling out the `ValueError` raised on invariant violation. The forward reference to `InvalidBounds` is deferred until that type actually exists to keep `mkdocs --strict` happy. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 16 ++++------------ tests/metrics/test_bounds.py | 5 +++++ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 4f3f6487..4e1a2ba0 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -37,24 +37,16 @@ def __new__(cls, *args: Any, **kwargs: Any) -> Self: @dataclasses.dataclass(frozen=True, kw_only=True) -class Bounds: +class Bounds(BaseBounds): """A set of lower and upper bounds for any metric. The lower bound must be less than or equal to the upper bound. The units of the bounds are always the same as the related metric. - """ - - lower: float | None = None - """The lower bound. - If `None`, there is no lower bound. - """ - - upper: float | None = None - """The upper bound. - - If `None`, there is no upper bound. + Note: + Raises a `ValueError` if [`lower`][.lower] is greater than + [`upper`][.upper]. """ def __post_init__(self) -> None: diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index 66fd1c8b..a76ef12a 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -16,6 +16,11 @@ def test_base_bounds_cannot_be_instantiated_directly() -> None: BaseBounds() +def test_bounds_is_base_bounds_subclass() -> None: + """`Bounds` is a subclass of `BaseBounds`.""" + assert issubclass(Bounds, BaseBounds) + + @pytest.mark.parametrize( "lower, upper", [ From 2fe2afd3101fe0b1a09611ef350fbdabf6082b18 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:50:38 +0000 Subject: [PATCH 04/26] Use `[lower,upper]` format in `Bounds.__str__` Drop the space after the comma in the compact rendering so bounds now print as e.g. `[-10.0,10.0]`, aligning with `Lifetime`'s `({start},{end}]` shape. Keeping the two `__str__` methods visually consistent matters because the upcoming `InvalidBounds` will wrap this same rendering in the `` marker, and any drift between the two would leak into that composed form. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_bounds.py | 2 +- tests/metrics/test_bounds.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 4e1a2ba0..d012eb64 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -63,4 +63,4 @@ def __post_init__(self) -> None: def __str__(self) -> str: """Return a string representation of these bounds.""" - return f"[{self.lower}, {self.upper}]" + return f"[{self.lower},{self.upper}]" diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index a76ef12a..06d7f2d3 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -56,7 +56,7 @@ def test_invalid_values() -> None: 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]" + assert str(bounds) == "[-10.0,10.0]" def test_equality() -> None: From e5149050f763398d7fffdf68d1cb37f7437ab7c2 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:53:28 +0000 Subject: [PATCH 05/26] Add `InvalidBounds` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a second `BaseBounds` leaf that carries bounds data received from the wire even when it violates the `Bounds` invariant (`lower <= upper`). The type enforces no invariants of its own, so callers can inspect the raw values without accidentally treating them as a valid range — while `isinstance(x, Bounds)` still guarantees the invariant holds for well-formed data. The class ships a dedicated `__str__` that reuses the compact `[lower,upper]` shape and wraps it in the shared `` marker used by other invalid types in this codebase. Now that the sibling exists, the deferred forward references from `BaseBounds` and `Bounds` are filled in — both docstrings link back to `InvalidBounds` — and the type is re-exported alongside `BaseBounds` and `Bounds` from `frequenz.client.common.metrics`. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 3 +- src/frequenz/client/common/metrics/_bounds.py | 22 +++++++- tests/metrics/test_bounds.py | 51 ++++++++++++++++++- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index 7e6c88f8..eed273fd 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 +from ._bounds import BaseBounds, Bounds, InvalidBounds from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -18,6 +18,7 @@ "AggregationMethod", "BaseBounds", "Bounds", + "InvalidBounds", "Metric", "MetricConnection", "MetricConnectionCategory", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index d012eb64..79dc0206 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -13,7 +13,8 @@ class BaseBounds: """A base class for well-formed and malformed metric bounds. This class cannot be instantiated directly. Use [`Bounds`][..Bounds] for a - valid pair of bounds. + valid pair of bounds or [`InvalidBounds`][..InvalidBounds] to preserve + malformed wire data. """ lower: float | int | None = None @@ -46,7 +47,8 @@ class Bounds(BaseBounds): Note: Raises a `ValueError` if [`lower`][.lower] is greater than - [`upper`][.upper]. + [`upper`][.upper]. Use [`InvalidBounds`][..InvalidBounds] to + represent malformed bounds data received from the wire. """ def __post_init__(self) -> None: @@ -64,3 +66,19 @@ def __post_init__(self) -> None: def __str__(self) -> str: """Return a string representation of these bounds.""" return f"[{self.lower},{self.upper}]" + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class InvalidBounds(BaseBounds): + """Metric bounds with malformed data received from the wire. + + This class preserves bounds data that fails the invariants required for + a well-formed [`Bounds`][..Bounds], allowing callers to inspect the raw + values without accidentally using them for range checks. Use a semantic + accessor, such as `ElectricalComponent.get_metric_config_bounds()`, to + receive a clear error on invalid data. + """ + + def __str__(self) -> str: + """Return a compact string representation of these invalid bounds.""" + return f"" diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index 06d7f2d3..8ffb480e 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -7,7 +7,7 @@ import pytest -from frequenz.client.common.metrics import BaseBounds, Bounds +from frequenz.client.common.metrics import BaseBounds, Bounds, InvalidBounds def test_base_bounds_cannot_be_instantiated_directly() -> None: @@ -81,3 +81,52 @@ def test_hash() -> None: bounds_dict = {bounds1: "test1", bounds3: "test2"} assert len(bounds_dict) == 2 + + +def test_invalid_bounds_is_base_bounds_subclass() -> None: + """`InvalidBounds` is a subclass of `BaseBounds`.""" + assert issubclass(InvalidBounds, BaseBounds) + + +def test_invalid_bounds_is_not_bounds_subclass() -> None: + """`InvalidBounds` is a sibling of `Bounds`, not a subclass.""" + assert not issubclass(InvalidBounds, Bounds) + + +def test_invalid_bounds_accepts_invalid_range() -> None: + """`InvalidBounds` preserves an upper bound below its lower bound.""" + bounds = InvalidBounds(lower=10.0, upper=-10.0) + assert bounds.lower == 10.0 + assert bounds.upper == -10.0 + + +def test_invalid_bounds_accepts_valid_looking_values() -> None: + """`InvalidBounds` enforces no invariants and accepts any values.""" + bounds = InvalidBounds(lower=-10.0, upper=10.0) + assert bounds.lower == -10.0 + assert bounds.upper == 10.0 + + +def test_invalid_bounds_str_representation() -> None: + """`InvalidBounds.__str__` wraps the compact form in ``.""" + assert str(InvalidBounds(lower=10.0, upper=-10.0)) == "" + assert str(InvalidBounds()) == "" + + +def test_invalid_bounds_equality() -> None: + """Test equality comparison of `InvalidBounds` objects.""" + bounds1 = InvalidBounds(lower=10.0, upper=-10.0) + bounds2 = InvalidBounds(lower=10.0, upper=-10.0) + bounds3 = InvalidBounds(lower=5.0, upper=-5.0) + + assert bounds1 == bounds2 + assert bounds1 != bounds3 + + +def test_invalid_bounds_hash() -> None: + """Test that `InvalidBounds` objects can be used in sets and as dict keys.""" + bounds1 = InvalidBounds(lower=10.0, upper=-10.0) + bounds2 = InvalidBounds(lower=10.0, upper=-10.0) + bounds3 = InvalidBounds(lower=5.0, upper=-5.0) + + assert len({bounds1, bounds2, bounds3}) == 2 From 8ef2355b9e78896b58c2998446af0ac93d62805c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:55:00 +0000 Subject: [PATCH 06/26] Add `MissingBounds` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a third `BaseBounds` leaf for the specific case of a wire entry that named a metric but carried no bounds data at all (e.g. a `MetricConfigBounds` entry without a `config_bounds` field). Making this distinct from an unbounded well-formed `Bounds(lower=None, upper=None)` lets callers tell "the server said this metric has no bounds" from "the server forgot to tell us the bounds" and react accordingly. `MissingBounds` is a subclass of `InvalidBounds` on purpose. Container types stay simple — `Bounds | InvalidBounds` still covers everything a converter can produce — and users who care about the missing case narrow with a `case MissingBounds()` arm before the generic `case InvalidBounds()` in a `match` statement. Instances always carry `lower` and `upper` as `None`; `__post_init__` rejects any attempt to attach values so the type's only affordance stays the pure "missing" signal. `__str__` returns the greppable `` marker. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 3 +- src/frequenz/client/common/metrics/_bounds.py | 35 ++++++++++++ tests/metrics/test_bounds.py | 54 ++++++++++++++++++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index eed273fd..364c7b5d 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 +from ._bounds import BaseBounds, Bounds, InvalidBounds, MissingBounds from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -23,4 +23,5 @@ "MetricConnection", "MetricConnectionCategory", "MetricSample", + "MissingBounds", ] diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index 79dc0206..f2857b18 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -82,3 +82,38 @@ class InvalidBounds(BaseBounds): def __str__(self) -> str: """Return a compact string representation of these invalid bounds.""" return f"" + + +@dataclasses.dataclass(frozen=True, kw_only=True) +class MissingBounds(InvalidBounds): + """Metric bounds that were present on the wire but carried no data. + + Some wire messages name a metric without providing any bounds values + (e.g. a `MetricConfigBounds` entry whose `config_bounds` field was not + set). This class flags that situation explicitly so callers can decide + how to handle it — treat the metric as unbounded, raise, or report — + instead of the ambiguity of an unbounded [`Bounds`][..Bounds] that + happens to have both fields as `None`. + + Being a subclass of [`InvalidBounds`][..InvalidBounds] keeps container + typing simple: a `Bounds | InvalidBounds` union catches the missing + case, and callers who want to distinguish it narrow with a + ``case MissingBounds()`` arm before the generic + ``case InvalidBounds()``. + + Instances always carry [`lower`][.lower] and [`upper`][.upper] as + `None`. + + Note: + Raises a `ValueError` if [`lower`][.lower] or [`upper`][.upper] + is set to anything other than `None`. + """ + + def __post_init__(self) -> None: + """Validate that no bound values are carried.""" + if self.lower is not None or self.upper is not None: + raise ValueError("MissingBounds cannot carry bound values") + + def __str__(self) -> str: + """Return a compact string representation of these missing bounds.""" + return "" diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index 8ffb480e..de04dd50 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -7,7 +7,12 @@ import pytest -from frequenz.client.common.metrics import BaseBounds, Bounds, InvalidBounds +from frequenz.client.common.metrics import ( + BaseBounds, + Bounds, + InvalidBounds, + MissingBounds, +) def test_base_bounds_cannot_be_instantiated_directly() -> None: @@ -130,3 +135,50 @@ def test_invalid_bounds_hash() -> None: bounds3 = InvalidBounds(lower=5.0, upper=-5.0) assert len({bounds1, bounds2, bounds3}) == 2 + + +def test_missing_bounds_is_invalid_bounds_subclass() -> None: + """`MissingBounds` is a subclass of `InvalidBounds` (and `BaseBounds`).""" + assert issubclass(MissingBounds, InvalidBounds) + assert issubclass(MissingBounds, BaseBounds) + + +def test_missing_bounds_defaults_to_none() -> None: + """`MissingBounds` carries no bound values by default.""" + bounds = MissingBounds() + assert bounds.lower is None + assert bounds.upper is None + + +@pytest.mark.parametrize( + "lower, upper", + [ + (1.0, None), + (None, 1.0), + (1.0, 2.0), + (0.0, 0.0), + ], +) +def test_missing_bounds_rejects_values( + lower: float | int | None, upper: float | int | None +) -> None: + """`MissingBounds` refuses to carry any bound values.""" + with pytest.raises( + ValueError, match=re.escape("MissingBounds cannot carry bound values") + ): + MissingBounds(lower=lower, upper=upper) + + +def test_missing_bounds_str_representation() -> None: + """`MissingBounds.__str__` returns the `` marker.""" + assert str(MissingBounds()) == "" + + +def test_missing_bounds_not_equal_to_invalid_bounds() -> None: + """Dataclass equality is class-scoped: `MissingBounds() != InvalidBounds()`.""" + assert MissingBounds() != InvalidBounds() + + +def test_missing_bounds_equality() -> None: + """Two `MissingBounds` instances always compare equal.""" + assert MissingBounds() == MissingBounds() From f8e093e77c8c19550e422f069f074c470b3292d3 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:56:45 +0000 Subject: [PATCH 07/26] Add `InvalidBoundsError` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce `InvalidBoundsError` as the exception that upcoming `get_metric_config_bounds()`-style semantic accessors will raise when the underlying wire data was parsed into an `InvalidBounds` (or its `MissingBounds` subclass). It mirrors `InvalidLifetimeError` field-for-field: `instance` and `attr_name` come from the shared `InvalidAttributeError` base, plus a typed `.bounds: InvalidBounds` attribute so callers can inspect the offending values in a `try/except`. The default message embeds `repr(bounds)` so the offending `lower`/`upper` values (the full `InvalidBounds(...)` dataclass representation, or a `MissingBounds` shape) are visible without the caller having to unpack `.bounds`, and a `message` override is accepted so callers with more context can supply their own text. Because `InvalidAttributeError` already inherits from `ValueError`, existing catch-all `except ValueError:` sites keep working — the new type only adds structure for callers that want it. Exported from `frequenz.client.common.metrics` next to `InvalidBounds` and `MissingBounds`. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 9 +++- src/frequenz/client/common/metrics/_bounds.py | 41 +++++++++++++++++++ tests/metrics/test_bounds.py | 39 ++++++++++++++++++ 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index 364c7b5d..c47fc35c 100644 --- a/src/frequenz/client/common/metrics/__init__.py +++ b/src/frequenz/client/common/metrics/__init__.py @@ -3,7 +3,13 @@ """Metrics definitions.""" -from ._bounds import BaseBounds, Bounds, InvalidBounds, MissingBounds +from ._bounds import ( + BaseBounds, + Bounds, + InvalidBounds, + InvalidBoundsError, + MissingBounds, +) from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -19,6 +25,7 @@ "BaseBounds", "Bounds", "InvalidBounds", + "InvalidBoundsError", "Metric", "MetricConnection", "MetricConnectionCategory", diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index f2857b18..cc807338 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -7,6 +7,8 @@ import dataclasses from typing import Any, Self +from .._exception import InvalidAttributeError + @dataclasses.dataclass(frozen=True, kw_only=True) class BaseBounds: @@ -117,3 +119,42 @@ def __post_init__(self) -> None: def __str__(self) -> str: """Return a compact string representation of these missing bounds.""" return "" + + +class InvalidBoundsError(InvalidAttributeError): + """Raised when a semantic accessor sees invalid metric bounds. + + The offending [`InvalidBounds`][..InvalidBounds] is available as the + [`bounds`][.bounds] 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: InvalidBounds, + 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: The invalid bounds instance. + message: A custom error message. If `None`, a default message mentioning + the invalid bounds is used. + """ + self.bounds: InvalidBounds = bounds + """The invalid bounds that caused this error.""" + + super().__init__( + instance, + attr_name, + ( + message + if message is not None + else f"invalid bounds {bounds!r} for attribute {attr_name!r} in {instance}" + ), + ) diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index de04dd50..e7fccef8 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -7,10 +7,12 @@ import pytest +from frequenz.client.common import InvalidAttributeError from frequenz.client.common.metrics import ( BaseBounds, Bounds, InvalidBounds, + InvalidBoundsError, MissingBounds, ) @@ -182,3 +184,40 @@ def test_missing_bounds_not_equal_to_invalid_bounds() -> None: def test_missing_bounds_equality() -> None: """Two `MissingBounds` instances always compare equal.""" assert MissingBounds() == MissingBounds() + + +def test_invalid_bounds_error_default_message() -> None: + """`InvalidBoundsError` builds a default message from the invalid bounds.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + error = InvalidBoundsError("some-instance", "config_bounds", invalid) + + assert error.bounds is invalid + assert ( + str(error) + == f"invalid bounds {invalid!r} for attribute 'config_bounds' " + "in some-instance" + ) + + +def test_invalid_bounds_error_custom_message() -> None: + """`InvalidBoundsError` accepts a custom message.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + error = InvalidBoundsError( + "some-instance", + "config_bounds", + invalid, + message="bad bounds from server", + ) + + assert error.bounds is invalid + assert str(error) == "bad bounds from server" + + +def test_invalid_bounds_error_is_invalid_attribute_error() -> None: + """`InvalidBoundsError` is an `InvalidAttributeError`.""" + assert issubclass(InvalidBoundsError, InvalidAttributeError) + + +def test_invalid_bounds_error_is_value_error() -> None: + """`InvalidBoundsError` is a `ValueError`.""" + assert issubclass(InvalidBoundsError, ValueError) From bb21019db62a360ccef24b525edba97d76ed06c7 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 11:58:25 +0000 Subject: [PATCH 08/26] Add `bounds_from_proto2` conversion function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce the wire converter callers should migrate to: `bounds_from_proto2` returns `Bounds | InvalidBounds` and therefore lifts the "did the server send a valid range?" question into the type system instead of relying on a `ValueError` escape hatch. The implementation mirrors `lifetime_from_proto` on `err-lifetime` — extract `lower`/`upper` with `HasField`, try to build a well-formed `Bounds`, and fall back to `InvalidBounds` when the invariant fires — so both converters read the same way. A present-but-empty message still becomes an unbounded `Bounds()`, matching what the legacy `bounds_from_proto` produces today; the new function never returns `MissingBounds` because "there was no bounds message at all" is a fact only the containing message knows about, and classifying it belongs to the caller that saw the missing field, not this converter. Signed-off-by: Leandro Lucarella --- .../common/metrics/proto/v1alpha8/__init__.py | 7 +- .../common/metrics/proto/v1alpha8/_bounds.py | 25 +++++- tests/metrics/proto/v1alpha8/test_bounds.py | 79 ++++++++++++++++++- 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py b/src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py index efc7c6f0..757a0542 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py @@ -3,7 +3,11 @@ """Conversion of metrics enums from/to protobuf v1alpha8.""" -from ._bounds import bounds_from_proto, bounds_from_proto_with_issues +from ._bounds import ( + bounds_from_proto, + bounds_from_proto2, + bounds_from_proto_with_issues, +) from ._metric import metric_from_proto, metric_to_proto from ._metric_connection_category import ( metric_connection_category_from_proto, @@ -18,6 +22,7 @@ __all__ = [ "aggregated_metric_sample_from_proto", "bounds_from_proto", + "bounds_from_proto2", "bounds_from_proto_with_issues", "metric_connection_category_from_proto", "metric_connection_category_to_proto", diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py index 8e9fba4f..24c4c6e5 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py @@ -5,7 +5,7 @@ from frequenz.api.common.v1alpha8.metrics import bounds_pb2 -from ..._bounds import Bounds +from ..._bounds import Bounds, InvalidBounds def bounds_from_proto(message: bounds_pb2.Bounds) -> Bounds: # noqa: DOC502 @@ -26,6 +26,29 @@ def bounds_from_proto(message: bounds_pb2.Bounds) -> Bounds: # noqa: DOC502 ) +def bounds_from_proto2( + message: bounds_pb2.Bounds, +) -> Bounds | InvalidBounds: + """Create bounds from a protobuf message, preserving malformed data. + + Args: + message: The protobuf message to convert. + + Returns: + A [`Bounds`][....Bounds] when the values form a valid range, or an + [`InvalidBounds`][....InvalidBounds] preserving values that + violate `lower <= upper`. A present but empty protobuf message + becomes an unbounded `Bounds()`. + """ + lower = message.lower if message.HasField("lower") else None + upper = message.upper if message.HasField("upper") else None + try: + return Bounds(lower=lower, upper=upper) + except ValueError: + pass + return InvalidBounds(lower=lower, upper=upper) + + def bounds_from_proto_with_issues( message: bounds_pb2.Bounds, *, diff --git a/tests/metrics/proto/v1alpha8/test_bounds.py b/tests/metrics/proto/v1alpha8/test_bounds.py index 0444ccde..a75c959c 100644 --- a/tests/metrics/proto/v1alpha8/test_bounds.py +++ b/tests/metrics/proto/v1alpha8/test_bounds.py @@ -8,8 +8,10 @@ import pytest from frequenz.api.common.v1alpha8.metrics import bounds_pb2 +from frequenz.client.common.metrics import Bounds, InvalidBounds from frequenz.client.common.metrics.proto.v1alpha8 import ( bounds_from_proto, + bounds_from_proto2, bounds_from_proto_with_issues, ) @@ -27,10 +29,10 @@ class ProtoConversionTestCase: has_upper: bool """Whether to include upper bound in the protobuf message.""" - lower: float | None + lower: float | int | None """The lower bound value to set.""" - upper: float | None + upper: float | int | None """The upper bound value to set.""" @@ -122,3 +124,76 @@ def test_from_proto_with_issues_invalid() -> None: in major_issues[0] ) assert not minor_issues + + +@pytest.mark.parametrize( + "case", + [ + ProtoConversionTestCase( + name="full", + has_lower=True, + has_upper=True, + lower=-10.0, + upper=10.0, + ), + ProtoConversionTestCase( + name="no_upper_bound", + has_lower=True, + has_upper=False, + lower=-10.0, + upper=None, + ), + ProtoConversionTestCase( + name="no_lower_bound", + has_lower=False, + has_upper=True, + lower=None, + upper=10.0, + ), + ProtoConversionTestCase( + name="no_both_bounds", + has_lower=False, + has_upper=False, + lower=None, + upper=None, + ), + ], + ids=lambda case: case.name, +) +def test_from_proto2_valid(case: ProtoConversionTestCase) -> None: + """`bounds_from_proto2` returns a `Bounds` for well-formed messages.""" + proto = bounds_pb2.Bounds() + if case.has_lower and case.lower is not None: + proto.lower = case.lower + if case.has_upper and case.upper is not None: + proto.upper = case.upper + + bounds = bounds_from_proto2(proto) + + assert isinstance(bounds, Bounds) + assert not isinstance(bounds, InvalidBounds) + assert bounds.lower == case.lower + assert bounds.upper == case.upper + + +def test_from_proto2_invalid() -> None: + """`bounds_from_proto2` returns `InvalidBounds` when `lower > upper`.""" + proto = bounds_pb2.Bounds(lower=10.0, upper=-10.0) + + bounds = bounds_from_proto2(proto) + + assert isinstance(bounds, InvalidBounds) + assert not isinstance(bounds, Bounds) + assert bounds.lower == 10.0 + assert bounds.upper == -10.0 + + +def test_from_proto2_empty_message_is_unbounded_bounds() -> None: + """A present but empty protobuf message becomes an unbounded `Bounds()`.""" + bounds = bounds_from_proto2(bounds_pb2.Bounds()) + + assert isinstance(bounds, Bounds) + assert not isinstance(bounds, InvalidBounds) + assert bounds == Bounds() + assert bounds.lower is None + assert bounds.upper is None From 974335016873a3e60a03074d4a1d239ab545b454 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:02:03 +0000 Subject: [PATCH 09/26] Deprecate `bounds_from_proto` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark `bounds_from_proto` with `typing_extensions.deprecated` and point users at `bounds_from_proto2` in a `Warning: Deprecated` docstring admonition, matching the pattern already used by `delivery_area_from_proto`. The body is unchanged — external callers that ignore the warning still get exactly the same `Bounds` object (or `ValueError`) as before. Two internal call sites are converted so the library itself does not depend on the deprecated function anymore: * `_sample.py::_metric_bounds_from_proto` switches to `bounds_from_proto2` and a `match` over `Bounds` / `InvalidBounds` with `assert_never` for exhaustiveness. Observable behavior is preserved: well-formed entries land in the returned list, invalid ones are skipped and reported via `major_issues`. The reported message keeps the shape `bounds for is invalid (), ignoring these bounds`; the parenthetical now renders the `InvalidBounds.__str__` marker (``) instead of the raw exception text, which is a strict quality improvement (the marker is greppable and consistent with the other invalid types) — the affected assertion in `test_sample_metric_sample.py` is updated to match. * `bounds_from_proto_with_issues` inlines the `Bounds(...)` construction so it no longer routes through the deprecated helper. Its released contract (return the `Bounds`, or `None` + a `ValueError` string appended to `major_issues`) is byte-for-byte preserved. The direct call sites in `tests/metrics/proto/v1alpha8/test_bounds.py` are wrapped in `pytest.deprecated_call(match="bounds_from_proto2")` so the emitted `DeprecationWarning` is asserted rather than merely tolerated. `_electrical_component.py::_metric_config_bounds_from_proto` still calls `bounds_from_proto` internally; deprecating the call site under `src/frequenz/client/common/microgrid/` is deferred to the Phase B follow-up so this commit stays scoped to the metrics layer. Runtime callers see the outer `warnings.catch_warnings()` wrap in `_electrical_component_base_from_proto_with_issues`; the pytest `filterwarnings = ["once::DeprecationWarning", ...]` setting keeps the direct microgrid unit test that reaches into the private helper from turning the leaked warning into an error. Signed-off-by: Leandro Lucarella --- .../common/metrics/proto/v1alpha8/_bounds.py | 16 +++++++++++- .../common/metrics/proto/v1alpha8/_sample.py | 25 +++++++++++-------- tests/metrics/proto/v1alpha8/test_bounds.py | 10 +++++++- .../v1alpha8/test_sample_metric_sample.py | 4 +-- 4 files changed, 40 insertions(+), 15 deletions(-) diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py index 24c4c6e5..02215da6 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py @@ -4,13 +4,24 @@ """Loading of Bounds objects from protobuf messages.""" from frequenz.api.common.v1alpha8.metrics import bounds_pb2 +from typing_extensions import deprecated from ..._bounds import Bounds, InvalidBounds +@deprecated( + "`bounds_from_proto` is deprecated; use " + "`bounds_from_proto2` (returns `Bounds | InvalidBounds`) instead." +) def bounds_from_proto(message: bounds_pb2.Bounds) -> Bounds: # noqa: DOC502 """Create a [`Bounds`][....Bounds] object from a protobuf message. + Warning: Deprecated + Use [`bounds_from_proto2`][..bounds_from_proto2] instead. The new + converter distinguishes well-formed from malformed data at the + type level (`Bounds | InvalidBounds`) rather than raising a + `ValueError` when the invariant fires. + Args: message: The protobuf message to convert. @@ -66,7 +77,10 @@ def bounds_from_proto_with_issues( The corresponding [`Bounds`][....Bounds] object. """ try: - return bounds_from_proto(message) + return Bounds( + lower=message.lower if message.HasField("lower") else None, + upper=message.upper if message.HasField("upper") else None, + ) except ValueError as exc: major_issues.append(str(exc)) return None diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py index 6e804abe..5eaef157 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py @@ -4,11 +4,12 @@ """Loading of MetricSample and AggregatedMetricValue objects from protobuf messages.""" from collections.abc import Sequence +from typing import assert_never from frequenz.api.common.v1alpha8.metrics import bounds_pb2, metrics_pb2 from ....proto import datetime_from_proto -from ..._bounds import Bounds +from ..._bounds import Bounds, InvalidBounds from ..._metric import Metric from ..._sample import ( AggregatedMetricValue, @@ -16,7 +17,7 @@ MetricConnectionCategory, MetricSample, ) -from ._bounds import bounds_from_proto +from ._bounds import bounds_from_proto2 from ._metric import metric_from_proto from ._metric_connection_category import metric_connection_category_from_proto @@ -144,14 +145,16 @@ def _metric_bounds_from_proto( """ bounds: list[Bounds] = [] for pb_bound in messages: - try: - bound = bounds_from_proto(pb_bound) - except ValueError as exc: - metric_name = metric if isinstance(metric, int) else metric.name - major_issues.append( - f"bounds for {metric_name} is invalid ({exc}), ignoring these bounds" - ) - continue - bounds.append(bound) + match bounds_from_proto2(pb_bound): + case Bounds() as bound: + bounds.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" + ) + case unknown: + assert_never(unknown) return bounds diff --git a/tests/metrics/proto/v1alpha8/test_bounds.py b/tests/metrics/proto/v1alpha8/test_bounds.py index a75c959c..6acd2756 100644 --- a/tests/metrics/proto/v1alpha8/test_bounds.py +++ b/tests/metrics/proto/v1alpha8/test_bounds.py @@ -78,12 +78,20 @@ def test_from_proto(case: ProtoConversionTestCase) -> None: if case.has_upper and case.upper is not None: proto.upper = case.upper - bounds = bounds_from_proto(proto) + with pytest.deprecated_call(match="bounds_from_proto2"): + bounds = bounds_from_proto(proto) assert bounds.lower == case.lower assert bounds.upper == case.upper +def test_from_proto_emits_deprecation_warning() -> None: + """`bounds_from_proto` itself is deprecated and warns on call.""" + proto = bounds_pb2.Bounds(lower=-10.0, upper=10.0) + with pytest.deprecated_call(match="bounds_from_proto2"): + bounds_from_proto(proto) + + def test_from_proto_with_issues_valid() -> None: """Test bounds_from_proto_with_issues with valid bounds.""" proto = bounds_pb2.Bounds() diff --git a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py index 54b82520..b17a1aa2 100644 --- a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py +++ b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py @@ -156,8 +156,8 @@ class _TestCase: ), expected_major_issues=[ ( - "bounds for AC_POWER_ACTIVE is invalid (Lower bound (10.0) must be " - "less than or equal to upper bound (-10.0)), ignoring these bounds" + "bounds for AC_POWER_ACTIVE is invalid (), " + "ignoring these bounds" ) ], ), From 0353d632966d418ff690a31a26050970d26fdb8c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:03:14 +0000 Subject: [PATCH 10/26] Deprecate `bounds_from_proto_with_issues` Mark `bounds_from_proto_with_issues` with `typing_extensions.deprecated` and point users at `bounds_from_proto2` in a `Warning: Deprecated` docstring admonition. Callers should read the returned type (`Bounds | InvalidBounds`) directly instead of the string side-channel in `major_issues`; the new converter carries the same information at the type level and does not need the caller to hand-inspect a list. The function body is unchanged so external callers that keep it around during the transition still get the exact behavior they've been relying on. The two direct call sites in `test_bounds.py` wrap the call in `pytest.deprecated_call(match="bounds_from_proto2")` so the emitted `DeprecationWarning` is asserted rather than tolerated, and a dedicated `test_from_proto_with_issues_emits_deprecation_warning` locks the warning message down as the last shipped guarantee. Signed-off-by: Leandro Lucarella --- .../common/metrics/proto/v1alpha8/_bounds.py | 12 +++++++++ tests/metrics/proto/v1alpha8/test_bounds.py | 25 ++++++++++++++----- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py index 02215da6..bce06af7 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py @@ -60,6 +60,11 @@ def bounds_from_proto2( return InvalidBounds(lower=lower, upper=upper) +@deprecated( + "`bounds_from_proto_with_issues` is deprecated; use " + "`bounds_from_proto2` (returns `Bounds | InvalidBounds`) and inspect " + "the returned type instead." +) def bounds_from_proto_with_issues( message: bounds_pb2.Bounds, *, @@ -68,6 +73,13 @@ def bounds_from_proto_with_issues( ) -> Bounds | None: # noqa: DOC502 """Create a [`Bounds`][....Bounds] object from a protobuf message, collecting issues. + Warning: Deprecated + Use [`bounds_from_proto2`][..bounds_from_proto2] instead and + inspect the returned type. The new converter distinguishes + well-formed from malformed data at the type level + (`Bounds | InvalidBounds`) rather than routing invalid data + through a side-channel string list. + Args: message: The protobuf message to convert. major_issues: A list to append major issues to. diff --git a/tests/metrics/proto/v1alpha8/test_bounds.py b/tests/metrics/proto/v1alpha8/test_bounds.py index 6acd2756..b839b722 100644 --- a/tests/metrics/proto/v1alpha8/test_bounds.py +++ b/tests/metrics/proto/v1alpha8/test_bounds.py @@ -101,9 +101,10 @@ def test_from_proto_with_issues_valid() -> None: major_issues: list[str] = [] minor_issues: list[str] = [] - bounds = bounds_from_proto_with_issues( - proto, major_issues=major_issues, minor_issues=minor_issues - ) + with pytest.deprecated_call(match="bounds_from_proto2"): + bounds = bounds_from_proto_with_issues( + proto, major_issues=major_issues, minor_issues=minor_issues + ) assert bounds is not None assert bounds.lower == -10.0 @@ -121,9 +122,10 @@ def test_from_proto_with_issues_invalid() -> None: major_issues: list[str] = [] minor_issues: list[str] = [] - bounds = bounds_from_proto_with_issues( - proto, major_issues=major_issues, minor_issues=minor_issues - ) + with pytest.deprecated_call(match="bounds_from_proto2"): + bounds = bounds_from_proto_with_issues( + proto, major_issues=major_issues, minor_issues=minor_issues + ) assert bounds is None assert len(major_issues) == 1 @@ -134,6 +136,17 @@ def test_from_proto_with_issues_invalid() -> None: assert not minor_issues +def test_from_proto_with_issues_emits_deprecation_warning() -> None: + """`bounds_from_proto_with_issues` itself is deprecated and warns on call.""" + proto = bounds_pb2.Bounds(lower=-10.0, upper=10.0) + major_issues: list[str] = [] + minor_issues: list[str] = [] + with pytest.deprecated_call(match="bounds_from_proto2"): + bounds_from_proto_with_issues( + proto, major_issues=major_issues, minor_issues=minor_issues + ) + + @pytest.mark.parametrize( "case", [ From 712285b7a54017953d5190ea8f3ea70ecb6b1e0d Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:22:54 +0000 Subject: [PATCH 11/26] Update `metric_config_bounds` to preserve invalid bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ElectricalComponent.metric_config_bounds` is unreleased (introduced on this branch), so its type changes in place — no deprecation dance needed. Now that validity is encoded in the type system by `Bounds | InvalidBounds`, we can drop the side-channel string reports that used to describe malformed or missing entries and let the map itself carry the information. This mirrors the `err-lifetime` cleanup in `dc6a541`. Concretely: * `ElectricalComponent.metric_config_bounds` becomes `Mapping[Metric | int, Bounds | InvalidBounds]`. The field docstring spells out that malformed values are preserved as `InvalidBounds` and that entries whose `config_bounds` was not set become the `MissingBounds` subclass so callers can tell "server said unbounded" from "server forgot to send bounds". `MissingBounds` deliberately does not appear in the annotation — it is an `InvalidBounds` subclass, and keeping the union two-armed lets `match`/`isinstance` on `InvalidBounds` catch both cases while a `case MissingBounds()` arm distinguishes them when desired. * `_ElectricalComponentBaseData.metric_config_bounds` follows the same type change, with its attribute docstring updated to match. * `_metric_config_bounds_from_proto` loses the `major_issues` / `minor_issues` keyword parameters entirely. For each entry: * the metric key handling is unchanged in effect (unspecified → raw `int` `0`, unrecognized → raw `int`, the existing `warnings.catch_warnings` suppression for the deprecated `Metric.UNSPECIFIED` member is kept), but the "unrecognized metric" minor issue is gone — the plain `int` key is already the signal; * a missing `config_bounds` field stores `MissingBounds()` instead of logging a major issue and dropping the entry; * a present `config_bounds` runs through `bounds_from_proto2` and its result (`Bounds | InvalidBounds`) is stored directly — invalid entries are no longer silently dropped; * a duplicated metric on the wire keeps the last entry silently, per proto3 map semantics (the previous "using the last one" major-issue string is dropped; a comment explains the semantics). * The `_electrical_component_base_from_proto_with_issues` caller drops the `major_issues=`/`minor_issues=` kwargs on the `_metric_config_bounds_from_proto` call; the rest of the component-level `_with_issues` machinery is untouched (lifetime and category checks still surface issues as before). The direct-caller tests in `test_electrical_component_base.py` are rewritten around the new signature: `stores_unspecified_as_int` drops the issue-list plumbing; new cases lock down that invalid bounds surface as an `InvalidBounds` entry (values preserved, not equal to any `Bounds`), that a missing `config_bounds` yields `MissingBounds()`, and that duplicated metrics keep the last entry. No other `metric_config_bounds` assertion in the wider suite depended on the removed issue strings, so no other tests changed. Signed-off-by: Leandro Lucarella --- .../_electrical_component.py | 26 ++++--- .../proto/v1alpha8/_electrical_component.py | 71 ++++++++----------- tests/metrics/test_bounds.py | 3 +- .../test_electrical_component_base.py | 54 +++++++++++--- 4 files changed, 92 insertions(+), 62 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 ee74b0a0..a614d611 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -9,7 +9,7 @@ from typing import Any, Self, assert_never from ..._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError -from ...metrics import Bounds, Metric +from ...metrics import Bounds, InvalidBounds, Metric from .. import MicrogridId from .._lifetime import InvalidLifetime, InvalidLifetimeError, Lifetime from ._ids import ElectricalComponentId @@ -72,19 +72,29 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes ) """Internal guard allowing construction only via the `*_from_proto` converters.""" - metric_config_bounds: Mapping[Metric | int, Bounds] = 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, + metric_config_bounds: Mapping[Metric | int, Bounds | InvalidBounds] = ( + 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 metric configuration bounds for this electrical component, keyed by metric. These bounds may be derived from the component configuration, manufacturer limits, or limits of other devices. + Malformed bounds received from the wire are preserved as + [`InvalidBounds`][.....metrics.InvalidBounds] instances so callers can + inspect the raw values without accidentally using them for range checks. + Entries that named a metric but carried no bounds data at all are stored + as [`MissingBounds`][.....metrics.MissingBounds] (a subclass of + `InvalidBounds`), letting callers distinguish "explicitly unbounded" from + "the server forgot to send bounds". + If an unspecified metric is received, it is stored as the plain `int` key `0` when loading from protobuf. Metrics unknown to this client version may also appear as plain `int` keys for forward-compatibility. 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 4a169f1a..b7ca54f3 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 @@ -13,8 +13,8 @@ ) from google.protobuf.json_format import MessageToDict -from .....metrics import Bounds, Metric -from .....metrics.proto.v1alpha8 import bounds_from_proto +from .....metrics import Bounds, InvalidBounds, Metric, MissingBounds +from .....metrics.proto.v1alpha8 import bounds_from_proto2 from .....proto import enum_from_proto from ...._ids import MicrogridId from ...._lifetime import InvalidLifetime, Lifetime @@ -891,8 +891,14 @@ class _ElectricalComponentBaseData(NamedTuple): lifetime: Lifetime | InvalidLifetime """The operational lifetime of the electrical component.""" - metric_config_bounds: dict[Metric | int, Bounds] - """The metric configuration bounds extracted from the protobuf message.""" + metric_config_bounds: dict[Metric | int, Bounds | InvalidBounds] + """The metric configuration bounds extracted from the protobuf message. + + Malformed entries are preserved as + [`InvalidBounds`][frequenz.client.common.metrics.InvalidBounds]; entries + whose `config_bounds` field was not set are stored as + [`MissingBounds`][frequenz.client.common.metrics.MissingBounds]. + """ category_specific_info: dict[str, Any] """The category-specific metadata extracted from the protobuf message.""" @@ -912,7 +918,7 @@ def _electrical_component_base_from_proto_with_issues( message: electrical_components_pb2.ElectricalComponent, *, major_issues: list[str], - minor_issues: list[str], + minor_issues: list[str], # pylint: disable=unused-argument ) -> _ElectricalComponentBaseData: """Extract base data from a protobuf message and collect issues. @@ -936,9 +942,7 @@ def _electrical_component_base_from_proto_with_issues( lifetime = _get_operational_lifetime_from_proto(message) metric_config_bounds = _metric_config_bounds_from_proto( - message.metric_config_bounds, - major_issues=major_issues, - minor_issues=minor_issues, + message.metric_config_bounds ) category = enum_from_proto(message.category, ElectricalComponentCategory) @@ -1207,59 +1211,40 @@ def electrical_component_from_proto_with_issues( def _metric_config_bounds_from_proto( message: Sequence[electrical_components_pb2.MetricConfigBounds], - *, - major_issues: list[str], - minor_issues: list[str], # pylint: disable=unused-argument -) -> dict[Metric | int, Bounds]: - """Convert a `MetricConfigBounds` message to a dictionary mapping `Metric` to `Bounds`. +) -> dict[Metric | int, Bounds | InvalidBounds]: + """Convert a `MetricConfigBounds` message to a dictionary mapping `Metric` to bounds. The keys of the result map are [`Metric`][frequenz.client.common.metrics.Metric] enum members (or `int` for - unrecognized values) and the values are - [`Bounds`][frequenz.client.common.metrics.Bounds] objects. + unrecognized values). Values are + [`Bounds`][frequenz.client.common.metrics.Bounds] for well-formed entries, + [`InvalidBounds`][frequenz.client.common.metrics.InvalidBounds] for entries + that carried bound values violating `lower <= upper`, and + [`MissingBounds`][frequenz.client.common.metrics.MissingBounds] for entries + that named a metric but did not carry a `config_bounds` field. + + Duplicated metrics on the wire follow proto3 map semantics: the last entry + wins silently. Args: message: The `MetricConfigBounds` message. - major_issues: A list to append major issues to. - minor_issues: A list to append minor issues to. Returns: The resulting dictionary mapping metrics to their bounds. """ - bounds: dict[Metric | int, Bounds] = {} + bounds: dict[Metric | int, Bounds | InvalidBounds] = {} for metric_bound in message: with warnings.catch_warnings(): warnings.filterwarnings("ignore", category=DeprecationWarning) metric = enum_from_proto(metric_bound.metric, Metric) - match metric: - case Metric.UNSPECIFIED: - metric = metric.value - case int(): - minor_issues.append( - f"metric_config_bounds has an unrecognized metric {metric}" - ) + if metric is Metric.UNSPECIFIED: + metric = metric.value if not metric_bound.HasField("config_bounds"): - major_issues.append( - f"metric_config_bounds for {metric} is present but missing " - "`config_bounds`, considering it unbounded", - ) + bounds[metric] = MissingBounds() continue - try: - bound = bounds_from_proto(metric_bound.config_bounds) - except ValueError as exc: - major_issues.append( - f"metric_config_bounds for {metric} is invalid ({exc}), considering " - "it as missing (i.e. unbouded)", - ) - continue - if metric in bounds: - major_issues.append( - f"metric_config_bounds for {metric} is duplicated in the message" - f"using the last one ({bound})", - ) - bounds[metric] = bound + bounds[metric] = bounds_from_proto2(metric_bound.config_bounds) return bounds diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py index e7fccef8..f7baa22b 100644 --- a/tests/metrics/test_bounds.py +++ b/tests/metrics/test_bounds.py @@ -193,8 +193,7 @@ def test_invalid_bounds_error_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!r} for attribute 'config_bounds' " "in some-instance" ) 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 2f9a6a3c..3c03a151 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 @@ -12,7 +12,7 @@ ) from google.protobuf.timestamp_pb2 import Timestamp -from frequenz.client.common.metrics import Bounds, Metric +from frequenz.client.common.metrics import Bounds, InvalidBounds, Metric, MissingBounds from frequenz.client.common.microgrid import InvalidLifetime, Lifetime from frequenz.client.common.microgrid.electrical_components import ( ElectricalComponentCategory, @@ -201,7 +201,7 @@ def test_invalid_lifetime( def _metric_bound( - metric_value: int, lower: float, upper: float + metric_value: int, lower: float | int, upper: float | int ) -> electrical_components_pb2.MetricConfigBounds: """Build a `MetricConfigBounds` proto for the given raw metric int and bounds.""" return electrical_components_pb2.MetricConfigBounds( @@ -212,21 +212,57 @@ def _metric_bound( def test_metric_config_bounds_stores_unspecified_as_int() -> None: """Test UNSPECIFIED metric bounds load as plain int key 0.""" - major_issues: list[str] = [] - minor_issues: list[str] = [] message = [ _metric_bound(int(Metric.UNSPECIFIED.value), 0.0, 1.0), _metric_bound(_UNKNOWN_METRIC_INT, 2.0, 3.0), _metric_bound(int(Metric.DC_VOLTAGE.value), 4.0, 5.0), ] - parsed = _metric_config_bounds_from_proto( - message, major_issues=major_issues, minor_issues=minor_issues - ) + parsed = _metric_config_bounds_from_proto(message) assert parsed[int(Metric.UNSPECIFIED.value)] == Bounds(lower=0.0, upper=1.0) assert Metric.UNSPECIFIED not in parsed assert parsed[_UNKNOWN_METRIC_INT] == Bounds(lower=2.0, upper=3.0) assert parsed[Metric.DC_VOLTAGE] == Bounds(lower=4.0, upper=5.0) - assert not major_issues - assert any(str(_UNKNOWN_METRIC_INT) in issue for issue in minor_issues) + + +def test_metric_config_bounds_preserves_invalid_bounds() -> None: + """Invalid bounds are preserved as `InvalidBounds` entries, not skipped.""" + message = [ + _metric_bound(int(Metric.DC_VOLTAGE.value), 10.0, -10.0), + _metric_bound(int(Metric.AC_POWER_ACTIVE.value), -5.0, 5.0), + ] + + parsed = _metric_config_bounds_from_proto(message) + + invalid = parsed[Metric.DC_VOLTAGE] + assert isinstance(invalid, InvalidBounds) + assert not isinstance(invalid, Bounds) + assert invalid.lower == 10.0 + assert invalid.upper == -10.0 + assert parsed[Metric.AC_POWER_ACTIVE] == Bounds(lower=-5.0, upper=5.0) + + +def test_metric_config_bounds_preserves_missing_config_bounds() -> None: + """An entry without a `config_bounds` field yields a `MissingBounds`.""" + entry = electrical_components_pb2.MetricConfigBounds( + metric=metrics_pb2.Metric.ValueType(int(Metric.DC_VOLTAGE.value)) + ) + entry.ClearField("config_bounds") + + parsed = _metric_config_bounds_from_proto([entry]) + + assert parsed[Metric.DC_VOLTAGE] == MissingBounds() + assert isinstance(parsed[Metric.DC_VOLTAGE], MissingBounds) + + +def test_metric_config_bounds_duplicated_metric_last_wins() -> None: + """A duplicated metric on the wire is kept as its last entry (proto3 map semantics).""" + message = [ + _metric_bound(int(Metric.DC_VOLTAGE.value), 0.0, 1.0), + _metric_bound(int(Metric.DC_VOLTAGE.value), 2.0, 3.0), + ] + + parsed = _metric_config_bounds_from_proto(message) + + assert parsed[Metric.DC_VOLTAGE] == Bounds(lower=2.0, upper=3.0) From 628ad81148444dbcee2fdd9b4deb562806e694af Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:26:14 +0000 Subject: [PATCH 12/26] Add safe accessors for metric config bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that `ElectricalComponent.metric_config_bounds` is `Mapping[Metric | int, Bounds | InvalidBounds]`, callers that want a valid `Bounds` for a given metric need to narrow the union themselves on every access — which is easy to get wrong and easy to skip. Add two higher-level accessors that do the narrowing once, mirroring the pattern established by `Microgrid.get_delivery_area()` / `get_delivery_area_or_none()` on this branch and `ElectricalComponent.get_operational_lifetime()` on `err-lifetime`: * `get_metric_config_bounds(metric: Metric) -> Bounds` — a plain `self.metric_config_bounds[metric]` lookup (so a `KeyError` still propagates naturally when no bounds are configured for the metric, documented with a `# noqa: DOC502` per the repo precedent), then a `match` that returns the `Bounds` unchanged and turns any `InvalidBounds` — including its `MissingBounds` subclass — into an `InvalidBoundsError`. The default message is overridden to include the metric name so the exception is self-describing without the caller having to unpack `.bounds`. * `get_metric_config_bounds_or_none(metric: Metric) -> Bounds | None` — same shape but backed by `.metric_config_bounds.get(metric)`, so an absent metric returns `None` instead of raising `KeyError`. Malformed or missing bounds still raise `InvalidBoundsError`; only the "no entry at all" case falls through as `None`. Both accessors take `metric: Metric` only. Raw-`int` keys represent forward-compat or unspecified values that never carry semantic bounds, so exposing them here would just create a footgun; callers who need that surface can read the mapping directly. With the getters in place, the `metric_config_bounds` field docstring gains a `Tip:` pointing at both of them (matching the shape used for `operational_lifetime` on `err-lifetime`), and `InvalidBoundsError` is imported from `...metrics`. Tests live next to the existing `provides_telemetry` / `accepts_control` accessor tests in `tests/microgrid/electrical_components/test_electrical_component_base.py` and cover every arm: valid entry returns the `Bounds`, absent metric raises `KeyError` (or returns `None` for the `_or_none` variant), an `InvalidBounds` entry raises `InvalidBoundsError` with `.bounds` preserving the values and the metric named in the message, a `MissingBounds` entry raises `InvalidBoundsError` with `.bounds` still being a `MissingBounds`. A small `_make_component` helper keeps the test bodies focused on the accessor behavior. Signed-off-by: Leandro Lucarella --- .../_electrical_component.py | 71 +++++++++++- .../test_electrical_component_base.py | 108 ++++++++++++++++-- 2 files changed, 171 insertions(+), 8 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 a614d611..6ee3ec6d 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -9,7 +9,7 @@ from typing import Any, Self, assert_never from ..._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError -from ...metrics import Bounds, InvalidBounds, Metric +from ...metrics import Bounds, InvalidBounds, InvalidBoundsError, Metric from .. import MicrogridId from .._lifetime import InvalidLifetime, InvalidLifetimeError, Lifetime from ._ids import ElectricalComponentId @@ -98,6 +98,11 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes If an unspecified metric is received, it is stored as the plain `int` key `0` when loading from protobuf. Metrics unknown to this client version may also appear as plain `int` keys for forward-compatibility. + + Tip: + Prefer [`get_metric_config_bounds()`][..get_metric_config_bounds] or + [`get_metric_config_bounds_or_none()`][..get_metric_config_bounds_or_none] + when a valid [`Bounds`][.....metrics.Bounds] is required. """ category_specific_metadata: Mapping[str, Any] = dataclasses.field( @@ -202,6 +207,70 @@ def accepts_control(self) -> bool: case unknown: assert_never(unknown) + def get_metric_config_bounds(self, metric: Metric) -> Bounds: # noqa: DOC502,DOC503 + """Return the configured bounds for a metric as a valid `Bounds`. + + Args: + metric: The metric whose bounds to retrieve. + + Returns: + The valid [`Bounds`][.....metrics.Bounds] configured for `metric`. + + Raises: + KeyError: If no bounds are configured for `metric`. + InvalidBoundsError: If the bounds configured for `metric` are + malformed (or absent from the wire, in which case the offending + value is a [`MissingBounds`][.....metrics.MissingBounds]). The + offending instance is available on the exception's `bounds` + attribute. + """ + bounds = self.metric_config_bounds[metric] + match bounds: + case InvalidBounds() as invalid: + raise InvalidBoundsError( + self, + "metric_config_bounds", + invalid, + f"invalid bounds {invalid!r} for metric {metric} in {self}", + ) + case Bounds() as valid: + return valid + case unknown: + assert_never(unknown) + + def get_metric_config_bounds_or_none(self, metric: Metric) -> Bounds | None: + """Return the configured bounds for a metric, or `None` when absent. + + Args: + metric: The metric whose bounds to retrieve. + + Returns: + The valid [`Bounds`][.....metrics.Bounds] configured for `metric`, + or `None` when no bounds are configured for it. + + Raises: + InvalidBoundsError: If the bounds configured for `metric` are + malformed (or absent from the wire, in which case the offending + value is a [`MissingBounds`][.....metrics.MissingBounds]). The + offending instance is available on the exception's `bounds` + attribute. + """ + bounds = self.metric_config_bounds.get(metric) + match bounds: + case None: + return None + case InvalidBounds() as invalid: + raise InvalidBoundsError( + self, + "metric_config_bounds", + invalid, + f"invalid bounds {invalid!r} for metric {metric} in {self}", + ) + case Bounds() as valid: + return valid + case unknown: + assert_never(unknown) + def get_operational_lifetime(self) -> Lifetime: """Return the operational lifetime as a valid `Lifetime`. diff --git a/tests/microgrid/electrical_components/test_electrical_component_base.py b/tests/microgrid/electrical_components/test_electrical_component_base.py index 8fed6c64..2177e848 100644 --- a/tests/microgrid/electrical_components/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/test_electrical_component_base.py @@ -8,8 +8,17 @@ import pytest -from frequenz.client.common import UnrecognizedEnumValueError, UnspecifiedEnumValueError -from frequenz.client.common.metrics import Bounds, Metric +from frequenz.client.common import ( + UnrecognizedEnumValueError, + UnspecifiedEnumValueError, +) +from frequenz.client.common.metrics import ( + Bounds, + InvalidBounds, + InvalidBoundsError, + Metric, + MissingBounds, +) from frequenz.client.common.microgrid import ( InvalidLifetime, InvalidLifetimeError, @@ -27,14 +36,19 @@ class _TestElectricalComponent(ElectricalComponent): def _make_component( - operational_lifetime: Lifetime | InvalidLifetime, + *, + operational_lifetime: Lifetime | InvalidLifetime = Lifetime(), + metric_config_bounds: dict[Metric | int, Bounds | InvalidBounds] | None = None, ) -> _TestElectricalComponent: """Build a test component with the given operational lifetime.""" + if metric_config_bounds is None: + metric_config_bounds = {} return _TestElectricalComponent( id=ElectricalComponentId(1), microgrid_id=MicrogridId(2), name="", model="Test Model", + metric_config_bounds=metric_config_bounds, operational_lifetime=operational_lifetime, _provides_telemetry=True, _accepts_control=True, @@ -170,7 +184,7 @@ def test_accessors_raise_when_unrecognized() -> None: def test_get_operational_lifetime_returns_valid() -> None: """`get_operational_lifetime()` returns a valid lifetime unchanged.""" lifetime = Lifetime() - component = _make_component(lifetime) + component = _make_component(operational_lifetime=lifetime) assert component.get_operational_lifetime() is lifetime @@ -181,7 +195,7 @@ def test_get_operational_lifetime_raises_invalid() -> None: start_time=datetime(2025, 2, 1, tzinfo=timezone.utc), end_time=datetime(2025, 1, 1, tzinfo=timezone.utc), ) - component = _make_component(invalid) + component = _make_component(operational_lifetime=invalid) with pytest.raises(InvalidLifetimeError) as exc_info: component.get_operational_lifetime() @@ -191,7 +205,7 @@ def test_get_operational_lifetime_raises_invalid() -> None: def test_is_operational_at_raises_for_invalid_lifetime() -> None: """`is_operational_at()` raises when the lifetime is invalid.""" component = _make_component( - InvalidLifetime( + operational_lifetime=InvalidLifetime( start_time=datetime(2025, 2, 1, tzinfo=timezone.utc), end_time=datetime(2025, 1, 1, tzinfo=timezone.utc), ) @@ -204,7 +218,7 @@ def test_is_operational_at_raises_for_invalid_lifetime() -> None: def test_is_operational_now_raises_for_invalid_lifetime() -> None: """`is_operational_now()` raises when the lifetime is invalid.""" component = _make_component( - InvalidLifetime( + operational_lifetime=InvalidLifetime( start_time=datetime(2025, 2, 1, tzinfo=timezone.utc), end_time=datetime(2025, 1, 1, tzinfo=timezone.utc), ) @@ -214,6 +228,86 @@ def test_is_operational_now_raises_for_invalid_lifetime() -> None: component.is_operational_now() +def test_get_metric_config_bounds_returns_valid_bounds() -> None: + """`get_metric_config_bounds` returns the configured `Bounds` for a metric.""" + bounds = Bounds(lower=-10.0, upper=10.0) + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: bounds}) + + result = component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + + assert result is bounds + + +def test_get_metric_config_bounds_absent_raises_key_error() -> None: + """`get_metric_config_bounds` raises `KeyError` when no bounds are configured.""" + component = _make_component(metric_config_bounds={}) + + with pytest.raises(KeyError): + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + + +def test_get_metric_config_bounds_invalid_raises_error() -> None: + """`get_metric_config_bounds` raises `InvalidBoundsError` for malformed entries.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: invalid}) + + with pytest.raises(InvalidBoundsError) as exc_info: + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + + assert exc_info.value.bounds is invalid + assert "AC_POWER_ACTIVE" in str(exc_info.value) + + +def test_get_metric_config_bounds_missing_raises_error() -> None: + """`get_metric_config_bounds` raises `InvalidBoundsError` for `MissingBounds`.""" + missing = MissingBounds() + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: missing}) + + with pytest.raises(InvalidBoundsError) as exc_info: + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + + assert exc_info.value.bounds is missing + assert isinstance(exc_info.value.bounds, MissingBounds) + + +def test_get_metric_config_bounds_or_none_returns_valid_bounds() -> None: + """`get_metric_config_bounds_or_none` returns the configured `Bounds`.""" + bounds = Bounds(lower=-10.0, upper=10.0) + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: bounds}) + + assert component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) is bounds + + +def test_get_metric_config_bounds_or_none_absent_returns_none() -> None: + """`get_metric_config_bounds_or_none` returns `None` when no bounds are configured.""" + component = _make_component(metric_config_bounds={}) + + assert component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) is None + + +def test_get_metric_config_bounds_or_none_invalid_raises_error() -> None: + """`get_metric_config_bounds_or_none` still raises for malformed entries.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: invalid}) + + with pytest.raises(InvalidBoundsError) as exc_info: + component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) + + assert exc_info.value.bounds is invalid + assert "AC_POWER_ACTIVE" in str(exc_info.value) + + +def test_get_metric_config_bounds_or_none_missing_raises_error() -> None: + """`get_metric_config_bounds_or_none` raises `InvalidBoundsError` for `MissingBounds`.""" + missing = MissingBounds() + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: missing}) + + with pytest.raises(InvalidBoundsError) as exc_info: + component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) + + assert exc_info.value.bounds is missing + + @pytest.mark.parametrize( "name,expected_str", [ From 285591764d28420b15c6d2fc4464da39d9364d73 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:28:35 +0000 Subject: [PATCH 13/26] Split `Bounds` tests into per-type files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/.../metrics/_bounds.py` now exposes five public symbols, so the test file grew too large. Split it following the repository convention: tests/metrics/test_bounds.py -> tests/metrics/_bounds/test_.py Test bodies and assertions are unchanged. No shared fixtures are needed, so no `conftest.py`. Test names drop redundant type prefixes now that the file itself names the target (e.g. `test_missing_bounds_rejects_values` → `test_rejects_values`, `test_invalid_bounds_str_representation` → `test_str_representation`). Signed-off-by: Leandro Lucarella --- tests/metrics/_bounds/__init__.py | 4 + tests/metrics/_bounds/test_base_bounds.py | 14 ++ tests/metrics/_bounds/test_bounds.py | 77 ++++++ tests/metrics/_bounds/test_invalid_bounds.py | 55 +++++ .../_bounds/test_invalid_bounds_error.py | 43 ++++ tests/metrics/_bounds/test_missing_bounds.py | 55 +++++ tests/metrics/test_bounds.py | 222 ------------------ 7 files changed, 248 insertions(+), 222 deletions(-) create mode 100644 tests/metrics/_bounds/__init__.py create mode 100644 tests/metrics/_bounds/test_base_bounds.py create mode 100644 tests/metrics/_bounds/test_bounds.py create mode 100644 tests/metrics/_bounds/test_invalid_bounds.py create mode 100644 tests/metrics/_bounds/test_invalid_bounds_error.py create mode 100644 tests/metrics/_bounds/test_missing_bounds.py delete mode 100644 tests/metrics/test_bounds.py diff --git a/tests/metrics/_bounds/__init__.py b/tests/metrics/_bounds/__init__.py new file mode 100644 index 00000000..221b7955 --- /dev/null +++ b/tests/metrics/_bounds/__init__.py @@ -0,0 +1,4 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for bounds types.""" diff --git a/tests/metrics/_bounds/test_base_bounds.py b/tests/metrics/_bounds/test_base_bounds.py new file mode 100644 index 00000000..a50ed989 --- /dev/null +++ b/tests/metrics/_bounds/test_base_bounds.py @@ -0,0 +1,14 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `BaseBounds`.""" + +import pytest + +from frequenz.client.common.metrics import BaseBounds + + +def test_cannot_be_instantiated_directly() -> None: + """`BaseBounds` refuses direct instantiation.""" + with pytest.raises(TypeError, match="Cannot instantiate BaseBounds directly"): + BaseBounds() diff --git a/tests/metrics/_bounds/test_bounds.py b/tests/metrics/_bounds/test_bounds.py new file mode 100644 index 00000000..ee3d5dd7 --- /dev/null +++ b/tests/metrics/_bounds/test_bounds.py @@ -0,0 +1,77 @@ +# License: MIT +# Copyright © 2025 Frequenz Energy-as-a-Service GmbH + +"""Tests for `Bounds`.""" + +import re + +import pytest + +from frequenz.client.common.metrics import BaseBounds, Bounds + + +def test_is_base_bounds_subclass() -> None: + """`Bounds` is a subclass of `BaseBounds`.""" + assert issubclass(Bounds, BaseBounds) + + +@pytest.mark.parametrize( + "lower, upper", + [ + (None, None), + (10.0, None), + (None, -10.0), + (-10.0, 10.0), + (10.0, 10.0), + (-10.0, -10.0), + (0.0, 10.0), + (-10, 0.0), + (0.0, 0.0), + ], +) +def test_creation(lower: float | int | None, upper: float | int | None) -> None: + """Test creation of Bounds with valid values.""" + bounds = Bounds(lower=lower, upper=upper) + assert bounds.lower == lower + assert bounds.upper == upper + + +def test_invalid_values() -> None: + """Test that Bounds creation fails with invalid values.""" + with pytest.raises( + ValueError, + match=re.escape( + "Lower bound (10.0) must be less than or equal to upper bound (-10.0)" + ), + ): + Bounds(lower=10.0, upper=-10.0) + + +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]" + + +def test_equality() -> None: + """Test equality comparison of Bounds objects.""" + bounds1 = Bounds(lower=-10.0, upper=10.0) + bounds2 = Bounds(lower=-10.0, upper=10.0) + bounds3 = Bounds(lower=-5.0, upper=5.0) + + assert bounds1 == bounds2 + assert bounds1 != bounds3 + assert bounds2 != bounds3 + + +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) + bounds2 = Bounds(lower=-10.0, upper=10.0) + bounds3 = Bounds(lower=-5.0, upper=5.0) + + bounds_set = {bounds1, bounds2, bounds3} + assert len(bounds_set) == 2 # bounds1 and bounds2 are equal + + bounds_dict = {bounds1: "test1", bounds3: "test2"} + assert len(bounds_dict) == 2 diff --git a/tests/metrics/_bounds/test_invalid_bounds.py b/tests/metrics/_bounds/test_invalid_bounds.py new file mode 100644 index 00000000..50506436 --- /dev/null +++ b/tests/metrics/_bounds/test_invalid_bounds.py @@ -0,0 +1,55 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `InvalidBounds`.""" + +from frequenz.client.common.metrics import BaseBounds, Bounds, InvalidBounds + + +def test_is_base_bounds_subclass() -> None: + """`InvalidBounds` is a subclass of `BaseBounds`.""" + assert issubclass(InvalidBounds, BaseBounds) + + +def test_is_not_bounds_subclass() -> None: + """`InvalidBounds` is a sibling of `Bounds`, not a subclass.""" + assert not issubclass(InvalidBounds, Bounds) + + +def test_accepts_invalid_range() -> None: + """`InvalidBounds` preserves an upper bound below its lower bound.""" + bounds = InvalidBounds(lower=10.0, upper=-10.0) + assert bounds.lower == 10.0 + assert bounds.upper == -10.0 + + +def test_accepts_valid_looking_values() -> None: + """`InvalidBounds` enforces no invariants and accepts any values.""" + bounds = InvalidBounds(lower=-10.0, upper=10.0) + assert bounds.lower == -10.0 + assert bounds.upper == 10.0 + + +def test_str_representation() -> None: + """`InvalidBounds.__str__` wraps the compact form in ``.""" + assert str(InvalidBounds(lower=10.0, upper=-10.0)) == "" + assert str(InvalidBounds()) == "" + + +def test_equality() -> None: + """Test equality comparison of `InvalidBounds` objects.""" + bounds1 = InvalidBounds(lower=10.0, upper=-10.0) + bounds2 = InvalidBounds(lower=10.0, upper=-10.0) + bounds3 = InvalidBounds(lower=5.0, upper=-5.0) + + assert bounds1 == bounds2 + assert bounds1 != bounds3 + + +def test_hash() -> None: + """Test that `InvalidBounds` objects can be used in sets and as dict keys.""" + bounds1 = InvalidBounds(lower=10.0, upper=-10.0) + bounds2 = InvalidBounds(lower=10.0, upper=-10.0) + bounds3 = InvalidBounds(lower=5.0, upper=-5.0) + + assert len({bounds1, bounds2, bounds3}) == 2 diff --git a/tests/metrics/_bounds/test_invalid_bounds_error.py b/tests/metrics/_bounds/test_invalid_bounds_error.py new file mode 100644 index 00000000..cd12e742 --- /dev/null +++ b/tests/metrics/_bounds/test_invalid_bounds_error.py @@ -0,0 +1,43 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `InvalidBoundsError`.""" + +from frequenz.client.common import InvalidAttributeError +from frequenz.client.common.metrics import InvalidBounds, InvalidBoundsError + + +def test_default_message() -> None: + """`InvalidBoundsError` builds a default message from the invalid bounds.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + error = InvalidBoundsError("some-instance", "config_bounds", invalid) + + assert error.bounds is invalid + assert ( + str(error) == f"invalid bounds {invalid!r} for attribute 'config_bounds' " + "in some-instance" + ) + + +def test_custom_message() -> None: + """`InvalidBoundsError` accepts a custom message.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + error = InvalidBoundsError( + "some-instance", + "config_bounds", + invalid, + message="bad bounds from server", + ) + + assert error.bounds is invalid + assert str(error) == "bad bounds from server" + + +def test_is_invalid_attribute_error() -> None: + """`InvalidBoundsError` is an `InvalidAttributeError`.""" + assert issubclass(InvalidBoundsError, InvalidAttributeError) + + +def test_is_value_error() -> None: + """`InvalidBoundsError` is a `ValueError`.""" + assert issubclass(InvalidBoundsError, ValueError) diff --git a/tests/metrics/_bounds/test_missing_bounds.py b/tests/metrics/_bounds/test_missing_bounds.py new file mode 100644 index 00000000..3c5c3c68 --- /dev/null +++ b/tests/metrics/_bounds/test_missing_bounds.py @@ -0,0 +1,55 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for `MissingBounds`.""" + +import re + +import pytest + +from frequenz.client.common.metrics import BaseBounds, InvalidBounds, MissingBounds + + +def test_is_invalid_bounds_subclass() -> None: + """`MissingBounds` is a subclass of `InvalidBounds` (and `BaseBounds`).""" + assert issubclass(MissingBounds, InvalidBounds) + assert issubclass(MissingBounds, BaseBounds) + + +def test_defaults_to_none() -> None: + """`MissingBounds` carries no bound values by default.""" + bounds = MissingBounds() + assert bounds.lower is None + assert bounds.upper is None + + +@pytest.mark.parametrize( + "lower, upper", + [ + (1.0, None), + (None, 1.0), + (1.0, 2.0), + (0.0, 0.0), + ], +) +def test_rejects_values(lower: float | int | None, upper: float | int | None) -> None: + """`MissingBounds` refuses to carry any bound values.""" + with pytest.raises( + ValueError, match=re.escape("MissingBounds cannot carry bound values") + ): + MissingBounds(lower=lower, upper=upper) + + +def test_str_representation() -> None: + """`MissingBounds.__str__` returns the `` marker.""" + assert str(MissingBounds()) == "" + + +def test_not_equal_to_invalid_bounds() -> None: + """Dataclass equality is class-scoped: `MissingBounds() != InvalidBounds()`.""" + assert MissingBounds() != InvalidBounds() + + +def test_equality() -> None: + """Two `MissingBounds` instances always compare equal.""" + assert MissingBounds() == MissingBounds() diff --git a/tests/metrics/test_bounds.py b/tests/metrics/test_bounds.py deleted file mode 100644 index f7baa22b..00000000 --- a/tests/metrics/test_bounds.py +++ /dev/null @@ -1,222 +0,0 @@ -# License: MIT -# Copyright © 2025 Frequenz Energy-as-a-Service GmbH - -"""Tests for the Bounds class.""" - -import re - -import pytest - -from frequenz.client.common import InvalidAttributeError -from frequenz.client.common.metrics import ( - BaseBounds, - Bounds, - InvalidBounds, - InvalidBoundsError, - MissingBounds, -) - - -def test_base_bounds_cannot_be_instantiated_directly() -> None: - """`BaseBounds` refuses direct instantiation.""" - with pytest.raises(TypeError, match="Cannot instantiate BaseBounds directly"): - BaseBounds() - - -def test_bounds_is_base_bounds_subclass() -> None: - """`Bounds` is a subclass of `BaseBounds`.""" - assert issubclass(Bounds, BaseBounds) - - -@pytest.mark.parametrize( - "lower, upper", - [ - (None, None), - (10.0, None), - (None, -10.0), - (-10.0, 10.0), - (10.0, 10.0), - (-10.0, -10.0), - (0.0, 10.0), - (-10, 0.0), - (0.0, 0.0), - ], -) -def test_creation(lower: float, upper: float) -> None: - """Test creation of Bounds with valid values.""" - bounds = Bounds(lower=lower, upper=upper) - assert bounds.lower == lower - assert bounds.upper == upper - - -def test_invalid_values() -> None: - """Test that Bounds creation fails with invalid values.""" - with pytest.raises( - ValueError, - match=re.escape( - "Lower bound (10.0) must be less than or equal to upper bound (-10.0)" - ), - ): - Bounds(lower=10.0, upper=-10.0) - - -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]" - - -def test_equality() -> None: - """Test equality comparison of Bounds objects.""" - bounds1 = Bounds(lower=-10.0, upper=10.0) - bounds2 = Bounds(lower=-10.0, upper=10.0) - bounds3 = Bounds(lower=-5.0, upper=5.0) - - assert bounds1 == bounds2 - assert bounds1 != bounds3 - assert bounds2 != bounds3 - - -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) - bounds2 = Bounds(lower=-10.0, upper=10.0) - bounds3 = Bounds(lower=-5.0, upper=5.0) - - bounds_set = {bounds1, bounds2, bounds3} - assert len(bounds_set) == 2 # bounds1 and bounds2 are equal - - bounds_dict = {bounds1: "test1", bounds3: "test2"} - assert len(bounds_dict) == 2 - - -def test_invalid_bounds_is_base_bounds_subclass() -> None: - """`InvalidBounds` is a subclass of `BaseBounds`.""" - assert issubclass(InvalidBounds, BaseBounds) - - -def test_invalid_bounds_is_not_bounds_subclass() -> None: - """`InvalidBounds` is a sibling of `Bounds`, not a subclass.""" - assert not issubclass(InvalidBounds, Bounds) - - -def test_invalid_bounds_accepts_invalid_range() -> None: - """`InvalidBounds` preserves an upper bound below its lower bound.""" - bounds = InvalidBounds(lower=10.0, upper=-10.0) - assert bounds.lower == 10.0 - assert bounds.upper == -10.0 - - -def test_invalid_bounds_accepts_valid_looking_values() -> None: - """`InvalidBounds` enforces no invariants and accepts any values.""" - bounds = InvalidBounds(lower=-10.0, upper=10.0) - assert bounds.lower == -10.0 - assert bounds.upper == 10.0 - - -def test_invalid_bounds_str_representation() -> None: - """`InvalidBounds.__str__` wraps the compact form in ``.""" - assert str(InvalidBounds(lower=10.0, upper=-10.0)) == "" - assert str(InvalidBounds()) == "" - - -def test_invalid_bounds_equality() -> None: - """Test equality comparison of `InvalidBounds` objects.""" - bounds1 = InvalidBounds(lower=10.0, upper=-10.0) - bounds2 = InvalidBounds(lower=10.0, upper=-10.0) - bounds3 = InvalidBounds(lower=5.0, upper=-5.0) - - assert bounds1 == bounds2 - assert bounds1 != bounds3 - - -def test_invalid_bounds_hash() -> None: - """Test that `InvalidBounds` objects can be used in sets and as dict keys.""" - bounds1 = InvalidBounds(lower=10.0, upper=-10.0) - bounds2 = InvalidBounds(lower=10.0, upper=-10.0) - bounds3 = InvalidBounds(lower=5.0, upper=-5.0) - - assert len({bounds1, bounds2, bounds3}) == 2 - - -def test_missing_bounds_is_invalid_bounds_subclass() -> None: - """`MissingBounds` is a subclass of `InvalidBounds` (and `BaseBounds`).""" - assert issubclass(MissingBounds, InvalidBounds) - assert issubclass(MissingBounds, BaseBounds) - - -def test_missing_bounds_defaults_to_none() -> None: - """`MissingBounds` carries no bound values by default.""" - bounds = MissingBounds() - assert bounds.lower is None - assert bounds.upper is None - - -@pytest.mark.parametrize( - "lower, upper", - [ - (1.0, None), - (None, 1.0), - (1.0, 2.0), - (0.0, 0.0), - ], -) -def test_missing_bounds_rejects_values( - lower: float | int | None, upper: float | int | None -) -> None: - """`MissingBounds` refuses to carry any bound values.""" - with pytest.raises( - ValueError, match=re.escape("MissingBounds cannot carry bound values") - ): - MissingBounds(lower=lower, upper=upper) - - -def test_missing_bounds_str_representation() -> None: - """`MissingBounds.__str__` returns the `` marker.""" - assert str(MissingBounds()) == "" - - -def test_missing_bounds_not_equal_to_invalid_bounds() -> None: - """Dataclass equality is class-scoped: `MissingBounds() != InvalidBounds()`.""" - assert MissingBounds() != InvalidBounds() - - -def test_missing_bounds_equality() -> None: - """Two `MissingBounds` instances always compare equal.""" - assert MissingBounds() == MissingBounds() - - -def test_invalid_bounds_error_default_message() -> None: - """`InvalidBoundsError` builds a default message from the invalid bounds.""" - invalid = InvalidBounds(lower=10.0, upper=-10.0) - error = InvalidBoundsError("some-instance", "config_bounds", invalid) - - assert error.bounds is invalid - assert ( - str(error) == f"invalid bounds {invalid!r} for attribute 'config_bounds' " - "in some-instance" - ) - - -def test_invalid_bounds_error_custom_message() -> None: - """`InvalidBoundsError` accepts a custom message.""" - invalid = InvalidBounds(lower=10.0, upper=-10.0) - error = InvalidBoundsError( - "some-instance", - "config_bounds", - invalid, - message="bad bounds from server", - ) - - assert error.bounds is invalid - assert str(error) == "bad bounds from server" - - -def test_invalid_bounds_error_is_invalid_attribute_error() -> None: - """`InvalidBoundsError` is an `InvalidAttributeError`.""" - assert issubclass(InvalidBoundsError, InvalidAttributeError) - - -def test_invalid_bounds_error_is_value_error() -> None: - """`InvalidBoundsError` is a `ValueError`.""" - assert issubclass(InvalidBoundsError, ValueError) From a8913a56416b184fe9ec013e140a55805905f03a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 17 Jul 2026 10:58:28 +0000 Subject: [PATCH 14/26] Remove `MissingBounds` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `MetricConfigBounds` entry without a `config_bounds` field is not an error: it is the explicit wire encoding of an unbounded metric. Protobuf does not serialize default values, so an all-defaults `Bounds` message is always absent from the wire — there is no other way to encode "unbounded" than omitting the message. Treating that as a defect was wrong. Drop the `MissingBounds` class and load entries without `config_bounds` as an unbounded `Bounds()` instead. This also simplifies the converter: an unset `config_bounds` field reads as the default empty message, which `bounds_from_proto2` already turns into an unbounded `Bounds()`, so the `HasField()` special case disappears entirely. Signed-off-by: Leandro Lucarella --- .../client/common/metrics/__init__.py | 9 +-- src/frequenz/client/common/metrics/_bounds.py | 35 ------------ .../_electrical_component.py | 16 ++---- .../proto/v1alpha8/_electrical_component.py | 22 ++++---- tests/metrics/_bounds/test_missing_bounds.py | 55 ------------------- .../test_electrical_component_base.py | 9 ++- .../test_electrical_component_base.py | 24 -------- 7 files changed, 20 insertions(+), 150 deletions(-) delete mode 100644 tests/metrics/_bounds/test_missing_bounds.py diff --git a/src/frequenz/client/common/metrics/__init__.py b/src/frequenz/client/common/metrics/__init__.py index c47fc35c..987f203c 100644 --- a/src/frequenz/client/common/metrics/__init__.py +++ b/src/frequenz/client/common/metrics/__init__.py @@ -3,13 +3,7 @@ """Metrics definitions.""" -from ._bounds import ( - BaseBounds, - Bounds, - InvalidBounds, - InvalidBoundsError, - MissingBounds, -) +from ._bounds import BaseBounds, Bounds, InvalidBounds, InvalidBoundsError from ._metric import Metric from ._sample import ( AggregatedMetricValue, @@ -30,5 +24,4 @@ "MetricConnection", "MetricConnectionCategory", "MetricSample", - "MissingBounds", ] diff --git a/src/frequenz/client/common/metrics/_bounds.py b/src/frequenz/client/common/metrics/_bounds.py index cc807338..30896b9a 100644 --- a/src/frequenz/client/common/metrics/_bounds.py +++ b/src/frequenz/client/common/metrics/_bounds.py @@ -86,41 +86,6 @@ def __str__(self) -> str: return f"" -@dataclasses.dataclass(frozen=True, kw_only=True) -class MissingBounds(InvalidBounds): - """Metric bounds that were present on the wire but carried no data. - - Some wire messages name a metric without providing any bounds values - (e.g. a `MetricConfigBounds` entry whose `config_bounds` field was not - set). This class flags that situation explicitly so callers can decide - how to handle it — treat the metric as unbounded, raise, or report — - instead of the ambiguity of an unbounded [`Bounds`][..Bounds] that - happens to have both fields as `None`. - - Being a subclass of [`InvalidBounds`][..InvalidBounds] keeps container - typing simple: a `Bounds | InvalidBounds` union catches the missing - case, and callers who want to distinguish it narrow with a - ``case MissingBounds()`` arm before the generic - ``case InvalidBounds()``. - - Instances always carry [`lower`][.lower] and [`upper`][.upper] as - `None`. - - Note: - Raises a `ValueError` if [`lower`][.lower] or [`upper`][.upper] - is set to anything other than `None`. - """ - - def __post_init__(self) -> None: - """Validate that no bound values are carried.""" - if self.lower is not None or self.upper is not None: - raise ValueError("MissingBounds cannot carry bound values") - - def __str__(self) -> str: - """Return a compact string representation of these missing bounds.""" - return "" - - class InvalidBoundsError(InvalidAttributeError): """Raised when a semantic accessor sees invalid metric bounds. 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 6ee3ec6d..700008cd 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -90,10 +90,6 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes Malformed bounds received from the wire are preserved as [`InvalidBounds`][.....metrics.InvalidBounds] instances so callers can inspect the raw values without accidentally using them for range checks. - Entries that named a metric but carried no bounds data at all are stored - as [`MissingBounds`][.....metrics.MissingBounds] (a subclass of - `InvalidBounds`), letting callers distinguish "explicitly unbounded" from - "the server forgot to send bounds". If an unspecified metric is received, it is stored as the plain `int` key `0` when loading from protobuf. Metrics unknown to this client version may also appear @@ -219,10 +215,8 @@ def get_metric_config_bounds(self, metric: Metric) -> Bounds: # noqa: DOC502,DO Raises: KeyError: If no bounds are configured for `metric`. InvalidBoundsError: If the bounds configured for `metric` are - malformed (or absent from the wire, in which case the offending - value is a [`MissingBounds`][.....metrics.MissingBounds]). The - offending instance is available on the exception's `bounds` - attribute. + malformed. The offending instance is available on the + exception's `bounds` attribute. """ bounds = self.metric_config_bounds[metric] match bounds: @@ -250,10 +244,8 @@ def get_metric_config_bounds_or_none(self, metric: Metric) -> Bounds | None: Raises: InvalidBoundsError: If the bounds configured for `metric` are - malformed (or absent from the wire, in which case the offending - value is a [`MissingBounds`][.....metrics.MissingBounds]). The - offending instance is available on the exception's `bounds` - attribute. + malformed. The offending instance is available on the + exception's `bounds` attribute. """ bounds = self.metric_config_bounds.get(metric) match bounds: 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 b7ca54f3..8bc40b6e 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 @@ -13,7 +13,7 @@ ) from google.protobuf.json_format import MessageToDict -from .....metrics import Bounds, InvalidBounds, Metric, MissingBounds +from .....metrics import Bounds, InvalidBounds, Metric from .....metrics.proto.v1alpha8 import bounds_from_proto2 from .....proto import enum_from_proto from ...._ids import MicrogridId @@ -896,8 +896,8 @@ class _ElectricalComponentBaseData(NamedTuple): Malformed entries are preserved as [`InvalidBounds`][frequenz.client.common.metrics.InvalidBounds]; entries - whose `config_bounds` field was not set are stored as - [`MissingBounds`][frequenz.client.common.metrics.MissingBounds]. + whose `config_bounds` field was not set load as an unbounded + [`Bounds`][frequenz.client.common.metrics.Bounds]. """ category_specific_info: dict[str, Any] @@ -1217,11 +1217,15 @@ def _metric_config_bounds_from_proto( The keys of the result map are [`Metric`][frequenz.client.common.metrics.Metric] enum members (or `int` for unrecognized values). Values are - [`Bounds`][frequenz.client.common.metrics.Bounds] for well-formed entries, + [`Bounds`][frequenz.client.common.metrics.Bounds] for well-formed entries and [`InvalidBounds`][frequenz.client.common.metrics.InvalidBounds] for entries - that carried bound values violating `lower <= upper`, and - [`MissingBounds`][frequenz.client.common.metrics.MissingBounds] for entries - that named a metric but did not carry a `config_bounds` field. + that carried bound values violating `lower <= upper`. + + An entry with no configured limits — its `config_bounds` submessage absent, + or present but empty — loads as an unbounded + [`Bounds`][frequenz.client.common.metrics.Bounds]: a `Bounds` with neither + `lower` nor `upper` set imposes no limit in either direction. Absence and a + present-but-empty submessage are intentionally treated the same. Duplicated metrics on the wire follow proto3 map semantics: the last entry wins silently. @@ -1240,10 +1244,6 @@ def _metric_config_bounds_from_proto( if metric is Metric.UNSPECIFIED: metric = metric.value - if not metric_bound.HasField("config_bounds"): - bounds[metric] = MissingBounds() - continue - bounds[metric] = bounds_from_proto2(metric_bound.config_bounds) return bounds diff --git a/tests/metrics/_bounds/test_missing_bounds.py b/tests/metrics/_bounds/test_missing_bounds.py deleted file mode 100644 index 3c5c3c68..00000000 --- a/tests/metrics/_bounds/test_missing_bounds.py +++ /dev/null @@ -1,55 +0,0 @@ -# License: MIT -# Copyright © 2026 Frequenz Energy-as-a-Service GmbH - -"""Tests for `MissingBounds`.""" - -import re - -import pytest - -from frequenz.client.common.metrics import BaseBounds, InvalidBounds, MissingBounds - - -def test_is_invalid_bounds_subclass() -> None: - """`MissingBounds` is a subclass of `InvalidBounds` (and `BaseBounds`).""" - assert issubclass(MissingBounds, InvalidBounds) - assert issubclass(MissingBounds, BaseBounds) - - -def test_defaults_to_none() -> None: - """`MissingBounds` carries no bound values by default.""" - bounds = MissingBounds() - assert bounds.lower is None - assert bounds.upper is None - - -@pytest.mark.parametrize( - "lower, upper", - [ - (1.0, None), - (None, 1.0), - (1.0, 2.0), - (0.0, 0.0), - ], -) -def test_rejects_values(lower: float | int | None, upper: float | int | None) -> None: - """`MissingBounds` refuses to carry any bound values.""" - with pytest.raises( - ValueError, match=re.escape("MissingBounds cannot carry bound values") - ): - MissingBounds(lower=lower, upper=upper) - - -def test_str_representation() -> None: - """`MissingBounds.__str__` returns the `` marker.""" - assert str(MissingBounds()) == "" - - -def test_not_equal_to_invalid_bounds() -> None: - """Dataclass equality is class-scoped: `MissingBounds() != InvalidBounds()`.""" - assert MissingBounds() != InvalidBounds() - - -def test_equality() -> None: - """Two `MissingBounds` instances always compare equal.""" - assert MissingBounds() == MissingBounds() 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 3c03a151..e94a1a1b 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 @@ -12,7 +12,7 @@ ) from google.protobuf.timestamp_pb2 import Timestamp -from frequenz.client.common.metrics import Bounds, InvalidBounds, Metric, MissingBounds +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 ( ElectricalComponentCategory, @@ -243,8 +243,8 @@ def test_metric_config_bounds_preserves_invalid_bounds() -> None: assert parsed[Metric.AC_POWER_ACTIVE] == Bounds(lower=-5.0, upper=5.0) -def test_metric_config_bounds_preserves_missing_config_bounds() -> None: - """An entry without a `config_bounds` field yields a `MissingBounds`.""" +def test_metric_config_bounds_absent_config_bounds_is_unbounded() -> None: + """An entry without a `config_bounds` field yields an unbounded `Bounds`.""" entry = electrical_components_pb2.MetricConfigBounds( metric=metrics_pb2.Metric.ValueType(int(Metric.DC_VOLTAGE.value)) ) @@ -252,8 +252,7 @@ def test_metric_config_bounds_preserves_missing_config_bounds() -> None: parsed = _metric_config_bounds_from_proto([entry]) - assert parsed[Metric.DC_VOLTAGE] == MissingBounds() - assert isinstance(parsed[Metric.DC_VOLTAGE], MissingBounds) + assert parsed[Metric.DC_VOLTAGE] == Bounds() def test_metric_config_bounds_duplicated_metric_last_wins() -> None: diff --git a/tests/microgrid/electrical_components/test_electrical_component_base.py b/tests/microgrid/electrical_components/test_electrical_component_base.py index 2177e848..39014847 100644 --- a/tests/microgrid/electrical_components/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/test_electrical_component_base.py @@ -17,7 +17,6 @@ InvalidBounds, InvalidBoundsError, Metric, - MissingBounds, ) from frequenz.client.common.microgrid import ( InvalidLifetime, @@ -258,18 +257,6 @@ def test_get_metric_config_bounds_invalid_raises_error() -> None: assert "AC_POWER_ACTIVE" in str(exc_info.value) -def test_get_metric_config_bounds_missing_raises_error() -> None: - """`get_metric_config_bounds` raises `InvalidBoundsError` for `MissingBounds`.""" - missing = MissingBounds() - component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: missing}) - - with pytest.raises(InvalidBoundsError) as exc_info: - component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) - - assert exc_info.value.bounds is missing - assert isinstance(exc_info.value.bounds, MissingBounds) - - def test_get_metric_config_bounds_or_none_returns_valid_bounds() -> None: """`get_metric_config_bounds_or_none` returns the configured `Bounds`.""" bounds = Bounds(lower=-10.0, upper=10.0) @@ -297,17 +284,6 @@ def test_get_metric_config_bounds_or_none_invalid_raises_error() -> None: assert "AC_POWER_ACTIVE" in str(exc_info.value) -def test_get_metric_config_bounds_or_none_missing_raises_error() -> None: - """`get_metric_config_bounds_or_none` raises `InvalidBoundsError` for `MissingBounds`.""" - missing = MissingBounds() - component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: missing}) - - with pytest.raises(InvalidBoundsError) as exc_info: - component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) - - assert exc_info.value.bounds is missing - - @pytest.mark.parametrize( "name,expected_str", [ From 1bc248fcc7705c408f94415ee7234ce1990e25ce Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 17 Jul 2026 11:02:57 +0000 Subject: [PATCH 15/26] Make `get_metric_config_bounds()` mimic `dict.get()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that an absent metric-config entry means "explicitly unbounded", forcing users to special-case absence (catching `KeyError` or checking for `None`) is just friction: they almost always want the bounds and an unbounded `Bounds()` is a perfectly usable answer. Return an unbounded `Bounds()` for absent metrics instead of raising, and accept a keyword-only `default` (like `dict.get()`) for callers that do need to detect absence — `default=None` recovers the old `get_metric_config_bounds_or_none()` behavior exactly, so that accessor is removed. Signed-off-by: Leandro Lucarella --- .../electrical_components/__init__.py | 3 +- .../_electrical_component.py | 70 ++++++++++--------- .../test_electrical_component_base.py | 59 +++++++++------- 3 files changed, 73 insertions(+), 59 deletions(-) diff --git a/src/frequenz/client/common/microgrid/electrical_components/__init__.py b/src/frequenz/client/common/microgrid/electrical_components/__init__.py index 380d67d0..5f4484bc 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/__init__.py +++ b/src/frequenz/client/common/microgrid/electrical_components/__init__.py @@ -18,7 +18,7 @@ from ._converter import Converter from ._crypto_miner import CryptoMiner from ._diagnostic_code import ElectricalComponentDiagnosticCode -from ._electrical_component import ElectricalComponent +from ._electrical_component import DefaultT, ElectricalComponent from ._electrical_component_connection import ( BaseElectricalComponentConnection, ElectricalComponentConnection, @@ -85,6 +85,7 @@ "Converter", "CryptoMiner", "DcEvCharger", + "DefaultT", "ElectricalComponent", "ElectricalComponentCategory", "ElectricalComponentConnection", 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 700008cd..2b225cbe 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py @@ -6,7 +6,7 @@ import dataclasses from collections.abc import Mapping from datetime import datetime, timezone -from typing import Any, Self, assert_never +from typing import Any, Self, TypeVar, assert_never, overload from ..._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError from ...metrics import Bounds, InvalidBounds, InvalidBoundsError, Metric @@ -14,6 +14,9 @@ from .._lifetime import InvalidLifetime, InvalidLifetimeError, Lifetime from ._ids import ElectricalComponentId +DefaultT = TypeVar("DefaultT") +"""A type variable for the default value of dict-like getters.""" + @dataclasses.dataclass(frozen=True, kw_only=True) class ElectricalComponent: # pylint: disable=too-many-instance-attributes @@ -96,8 +99,7 @@ class ElectricalComponent: # pylint: disable=too-many-instance-attributes as plain `int` keys for forward-compatibility. Tip: - Prefer [`get_metric_config_bounds()`][..get_metric_config_bounds] or - [`get_metric_config_bounds_or_none()`][..get_metric_config_bounds_or_none] + Prefer [`get_metric_config_bounds()`][..get_metric_config_bounds] when a valid [`Bounds`][.....metrics.Bounds] is required. """ @@ -203,54 +205,56 @@ def accepts_control(self) -> bool: case unknown: assert_never(unknown) - def get_metric_config_bounds(self, metric: Metric) -> Bounds: # noqa: DOC502,DOC503 + @overload + def get_metric_config_bounds(self, metric: Metric) -> Bounds: ... + + @overload + def get_metric_config_bounds( + self, metric: Metric, *, default: DefaultT + ) -> Bounds | DefaultT: ... + + def get_metric_config_bounds( + self, metric: Metric, *, default: object = Bounds() + ) -> object: """Return the configured bounds for a metric as a valid `Bounds`. - Args: - metric: The metric whose bounds to retrieve. + An absent entry returns an unbounded metric, so when no bounds are + configured for `metric` this returns an unbounded + [`Bounds`][frequenz.client.common.metrics.Bounds] by default. Pass + `default` to return a different value for absent entries instead, + mimicking [`dict.get()`][dict.get]. - Returns: - The valid [`Bounds`][.....metrics.Bounds] configured for `metric`. + Example: + To check if a `metric` has **valid** configured bounds, you can use: - Raises: - KeyError: If no bounds are configured for `metric`. - InvalidBoundsError: If the bounds configured for `metric` are - malformed. The offending instance is available on the - exception's `bounds` attribute. - """ - bounds = self.metric_config_bounds[metric] - match bounds: - case InvalidBounds() as invalid: - raise InvalidBoundsError( - self, - "metric_config_bounds", - invalid, - f"invalid bounds {invalid!r} for metric {metric} in {self}", - ) - case Bounds() as valid: - return valid - case unknown: - assert_never(unknown) + ```py + component: ElectricalComponent + metric: Metric + if component.get_metric_config_bounds(metric, default=None) is not None: + print(f"{metric} has valid configured bounds") + ``` - def get_metric_config_bounds_or_none(self, metric: Metric) -> Bounds | None: - """Return the configured bounds for a metric, or `None` when absent. + This is similar to accessing + [`metric_config_bounds`][...ElectricalComponent.metric_config_bounds] + directly, but avoid the special handling of invalid bounds. Args: metric: The metric whose bounds to retrieve. + default: The value to return when no bounds are configured for + `metric`. Returns: The valid [`Bounds`][.....metrics.Bounds] configured for `metric`, - or `None` when no bounds are configured for it. + or `default` when there is no entry for `metric`. Raises: InvalidBoundsError: If the bounds configured for `metric` are malformed. The offending instance is available on the exception's `bounds` attribute. """ - bounds = self.metric_config_bounds.get(metric) - match bounds: + match self.metric_config_bounds.get(metric): case None: - return None + return default case InvalidBounds() as invalid: raise InvalidBoundsError( self, diff --git a/tests/microgrid/electrical_components/test_electrical_component_base.py b/tests/microgrid/electrical_components/test_electrical_component_base.py index 39014847..b04ad6e5 100644 --- a/tests/microgrid/electrical_components/test_electrical_component_base.py +++ b/tests/microgrid/electrical_components/test_electrical_component_base.py @@ -237,53 +237,62 @@ def test_get_metric_config_bounds_returns_valid_bounds() -> None: assert result is bounds -def test_get_metric_config_bounds_absent_raises_key_error() -> None: - """`get_metric_config_bounds` raises `KeyError` when no bounds are configured.""" +def test_get_metric_config_bounds_absent_returns_unbounded() -> None: + """`get_metric_config_bounds` returns an unbounded `Bounds` for absent metrics.""" component = _make_component(metric_config_bounds={}) - with pytest.raises(KeyError): - component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + result = component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) + assert result == Bounds() + assert isinstance(result, Bounds) -def test_get_metric_config_bounds_invalid_raises_error() -> None: - """`get_metric_config_bounds` raises `InvalidBoundsError` for malformed entries.""" - invalid = InvalidBounds(lower=10.0, upper=-10.0) - component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: invalid}) - with pytest.raises(InvalidBoundsError) as exc_info: - component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) +def test_get_metric_config_bounds_absent_returns_default() -> None: + """`get_metric_config_bounds` returns `default` for absent metrics.""" + component = _make_component(metric_config_bounds={}) + sentinel = object() - assert exc_info.value.bounds is invalid - assert "AC_POWER_ACTIVE" in str(exc_info.value) + assert ( + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE, default=None) is None + ) + assert ( + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE, default=sentinel) + is sentinel + ) -def test_get_metric_config_bounds_or_none_returns_valid_bounds() -> None: - """`get_metric_config_bounds_or_none` returns the configured `Bounds`.""" +def test_get_metric_config_bounds_present_ignores_default() -> None: + """`get_metric_config_bounds` ignores `default` when the metric has bounds.""" bounds = Bounds(lower=-10.0, upper=10.0) component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: bounds}) - assert component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) is bounds - - -def test_get_metric_config_bounds_or_none_absent_returns_none() -> None: - """`get_metric_config_bounds_or_none` returns `None` when no bounds are configured.""" - component = _make_component(metric_config_bounds={}) - - assert component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) is None + assert ( + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE, default=None) + is bounds + ) -def test_get_metric_config_bounds_or_none_invalid_raises_error() -> None: - """`get_metric_config_bounds_or_none` still raises for malformed entries.""" +def test_get_metric_config_bounds_invalid_raises_error() -> None: + """`get_metric_config_bounds` raises `InvalidBoundsError` for malformed entries.""" invalid = InvalidBounds(lower=10.0, upper=-10.0) component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: invalid}) with pytest.raises(InvalidBoundsError) as exc_info: - component.get_metric_config_bounds_or_none(Metric.AC_POWER_ACTIVE) + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE) assert exc_info.value.bounds is invalid assert "AC_POWER_ACTIVE" in str(exc_info.value) +def test_get_metric_config_bounds_invalid_raises_despite_default() -> None: + """`default` only applies to absent metrics, not malformed ones.""" + invalid = InvalidBounds(lower=10.0, upper=-10.0) + component = _make_component(metric_config_bounds={Metric.AC_POWER_ACTIVE: invalid}) + + with pytest.raises(InvalidBoundsError): + component.get_metric_config_bounds(Metric.AC_POWER_ACTIVE, default=None) + + @pytest.mark.parametrize( "name,expected_str", [ From c2179910eadd5f55b312d8d65c3d4400a92c77a6 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 16 Jul 2026 12:30:40 +0000 Subject: [PATCH 16/26] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index b7b191eb..5564d68e 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -52,6 +52,16 @@ A well-formed `DeliveryArea` has a non-empty `code` and a specified (non-`UNSPECIFIED`) `code_type`. Constructing one with invalid data currently emits a `DeprecationWarning`; a future release will replace the warning with a hard `ValueError`. To opt into the upcoming behavior right now, pass `_raise_on_invalid=True` to the constructor. Prefer `delivery_area_from_proto2` to load delivery areas from the wire — malformed messages become `InvalidDeliveryArea` instances instead. +* `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto` is now deprecated; use `bounds_from_proto2` instead. + + The new converter returns `Bounds | InvalidBounds` and surfaces malformed wire data at the type level rather than raising a `ValueError` when `lower > upper`. The old converter continues to work but emits a `DeprecationWarning`. + +* `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto_with_issues` is now deprecated with no direct replacement. + + Validity is now encoded in the return type of `bounds_from_proto2` (`Bounds | InvalidBounds`), so callers should inspect the returned type instead of collecting issue strings via a side channel. The old converter continues to work but emits a `DeprecationWarning`. + +* `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`. + ## New Features * Added 4 new electrical component classes for categories that previously collapsed into `UnrecognizedElectricalComponent`: @@ -81,6 +91,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.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()` * Added new delivery-area class hierarchy: @@ -92,6 +103,8 @@ * Added a new `frequenz.client.common.microgrid.Lifetime` type together with the `frequenz.client.common.microgrid.proto.v1alpha8.lifetime_from_proto` conversion function. +* Added `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto2` returning `Bounds | InvalidBounds`. This is the replacement for the now-deprecated `bounds_from_proto`. + * 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. @@ -100,6 +113,8 @@ The class of a component is its identity; components don't carry category or type attributes. The only exceptions are the error-recovery classes `UnrecognizedElectricalComponent` and `MismatchedCategoryElectricalComponent` (with a raw protobuf `category` value) and `UnrecognizedBattery`, `UnrecognizedInverter` and `UnrecognizedEvCharger` (with a raw protobuf `type` value), which preserve the raw protobuf values received from the protocol version used to load them. + `ElectricalComponent.metric_config_bounds` is typed `Mapping[Metric | int, Bounds | InvalidBounds]`: malformed wire entries are preserved as `InvalidBounds` instead of being silently dropped, and entries that named a metric but carried no (or an empty) `config_bounds` submessage load as an unbounded `Bounds()` (a `Bounds` with neither bound set imposes no limit in either direction). Use `get_metric_config_bounds()` to resolve an entry to a valid `Bounds` or a clear `InvalidBoundsError`; it mimics `dict.get()`, returning an unbounded `Bounds()` (or a caller-supplied `default`) for absent metrics. + * 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. ## Bug Fixes From 3c71a2d05b47d336d1d4518314f0db41bb1bc234 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:39:27 +0000 Subject: [PATCH 17/26] 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 26a871e8d0d7bcdf7e4fd3f4b01126fa064ab3c4 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:41:04 +0000 Subject: [PATCH 18/26] 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 6257a908ee11b088f71902a8897e2ba66860a79e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:53:53 +0000 Subject: [PATCH 19/26] 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 529a2349719215d0e98197da6f6ae2aee7d0506c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 10:56:42 +0000 Subject: [PATCH 20/26] 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 38ba21906cc9328634e5e7065c01cabde323a5d4 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:09:48 +0000 Subject: [PATCH 21/26] 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 8d4b8f2357bd82f4ba31c7ac44e6ec7ad91bda0a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:12:02 +0000 Subject: [PATCH 22/26] 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 8b09979a6af22fc83f342a69db41859765e3c8b9 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:15:35 +0000 Subject: [PATCH 23/26] 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 af6a08a011b6febf689ee4b308d55edced9d1320 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:29:18 +0000 Subject: [PATCH 24/26] 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 1c13dbb0bc7b4fc3001e80f2f2b814bc2151da4d Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:31:21 +0000 Subject: [PATCH 25/26] 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 9f4580565ab32e7467c483505e6ea766aa931fb0 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Sun, 19 Jul 2026 11:18:43 +0000 Subject: [PATCH 26/26] 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.