Improve `Bounds` validation and error handling by llucax · Pull Request #254 · frequenz-floss/frequenz-client-common-python

llucax · 2026-07-17T13:02:26Z

Like with Lifetime, Location, etc. we use the type system to encode invalid data.

Replaces #232 and part of #239 and #251.

daniel-zullo-frequenz

I reported a wrong path in the release notes and have a question. LGTM otherwise.

Just one thing to keep in mind, there was a back and forth about adding and removing MissingBounds which I found it confusing and it could have saved me some time if there was a clarification note about it for the reviewer.

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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

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 `<invalid:...>` marker, and any drift between the two would leak into that composed form. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

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 `<invalid:...>` 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 <luca-frequenz@llucax.com>

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 `<invalid:missing>` marker. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Copilot

Pull request overview

This PR updates the metrics Bounds conversion and electrical-component handling to encode malformed bounds in the type system (via InvalidBounds) instead of relying on log messages or side-channel “issues” lists, aligning with the project’s broader “invalid types + raising accessors” pattern.

Changes:

Introduce BaseBounds, InvalidBounds, and InvalidBoundsError, plus a new converter bounds_from_proto2() returning Bounds | InvalidBounds, and deprecate the older bounds_from_proto*() converters.
Preserve malformed metric config bounds on electrical components as InvalidBounds and add ElectricalComponent.get_metric_config_bounds() to provide a safe, raising accessor.
Remove logging-based “issues” reporting for Microgrid conversion and update tests accordingly.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/frequenz/client/common/metrics/_bounds.py`	Adds `BaseBounds`/`InvalidBounds` hierarchy and `InvalidBoundsError`; updates `Bounds.__str__` format.
`src/frequenz/client/common/metrics/__init__.py`	Exports new bounds types and error.
`src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py`	Adds `bounds_from_proto2()` and deprecates old bounds converters.
`src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py`	Re-exports `bounds_from_proto2()`.
`src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py`	Switches bounds parsing to `bounds_from_proto2()` and reports invalid bounds using `<invalid:...>`.
`src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py`	Preserves invalid metric config bounds as `InvalidBounds` and drops side-channel issue collection.
`src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py`	Types `metric_config_bounds` as `Bounds
`src/frequenz/client/common/microgrid/electrical_components/__init__.py`	Updates package exports (currently also exports `DefaultT`).
`src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py`	Removes conversion-time warning logging; relies on higher-level raising accessors instead.
`tests/metrics/_bounds/__init__.py`	Adds tests package scaffold for new bounds types.
`tests/metrics/_bounds/test_base_bounds.py`	Tests `BaseBounds` cannot be instantiated.
`tests/metrics/_bounds/test_bounds.py`	Updates bounds tests for new base class and `__str__` formatting.
`tests/metrics/_bounds/test_invalid_bounds.py`	Adds tests for `InvalidBounds` behavior and `<invalid:...>` rendering.
`tests/metrics/_bounds/test_invalid_bounds_error.py`	Adds tests for `InvalidBoundsError`.
`tests/metrics/proto/v1alpha8/test_bounds.py`	Adds tests for `bounds_from_proto2()` and deprecation warnings for old converters.
`tests/metrics/proto/v1alpha8/test_sample_metric_sample.py`	Updates expected major-issue string formatting for invalid bounds.
`tests/microgrid/electrical_components/test_electrical_component_base.py`	Adds tests for `get_metric_config_bounds()` behavior and invalid-bound error raising.
`tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py`	Updates metric-config-bounds proto tests for preserved invalid bounds and unbounded semantics.
`tests/microgrid/proto/v1alpha8/test_microgrid.py`	Removes logging assertions now that conversion warnings are dropped.
`RELEASE_NOTES.md`	Documents deprecations, new converter, new accessor, and `Bounds.__str__` change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

daniel-zullo-frequenz

Just a comment about a duplicated line in the release notes, LGTM otherwise

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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

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 <metric> is invalid (<detail>), ignoring these bounds`; the parenthetical now renders the `InvalidBounds.__str__` marker (`<invalid:[lower,upper]>`) 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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

`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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

`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_<thing>.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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

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 <luca-frequenz@llucax.com>

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax requested a review from a team as a code owner July 17, 2026 13:02

llucax requested review from ela-kotulska-frequenz and removed request for a team July 17, 2026 13:02

github-actions Bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:metrics Affects the metrics protobuf definitions part:microgrid Affects the microgrid protobuf definitions labels Jul 17, 2026

llucax force-pushed the err-bounds branch from d993072 to a655316 Compare July 17, 2026 13:28

llucax requested review from daniel-zullo-frequenz, shsms and tiyash-basu-frequenz July 17, 2026 13:28

llucax self-assigned this Jul 17, 2026

llucax added this to the v0.4.1 milestone Jul 17, 2026

llucax force-pushed the err-bounds branch from a655316 to cc95478 Compare July 20, 2026 16:38

llucax enabled auto-merge July 20, 2026 16:39

llucax mentioned this pull request Jul 20, 2026

Add BoundsSet and use it for MetricSample.bounds_set #255

Draft

daniel-zullo-frequenz reviewed Jul 21, 2026

View reviewed changes

Comment thread RELEASE_NOTES.md Outdated

daniel-zullo-frequenz reviewed Jul 21, 2026

View reviewed changes

Comment thread src/frequenz/client/common/microgrid/electrical_components/__init__.py

daniel-zullo-frequenz reviewed Jul 21, 2026

View reviewed changes

Comment thread src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py

llucax added 6 commits July 21, 2026 14:44

Copilot AI review requested due to automatic review settings July 21, 2026 12:45

llucax force-pushed the err-bounds branch from cc95478 to ef7e5cc Compare July 21, 2026 12:45

Copilot started reviewing on behalf of llucax July 21, 2026 12:45 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Comment thread src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py

Comment thread src/frequenz/client/common/microgrid/electrical_components/__init__.py

Comment thread src/frequenz/client/common/microgrid/electrical_components/__init__.py

llucax force-pushed the err-bounds branch from ef7e5cc to f77670c Compare July 21, 2026 13:56

llucax requested a review from daniel-zullo-frequenz July 21, 2026 13:57

daniel-zullo-frequenz reviewed Jul 21, 2026

View reviewed changes

Comment thread RELEASE_NOTES.md Outdated

daniel-zullo-frequenz reviewed Jul 21, 2026

View reviewed changes

llucax added 10 commits July 21, 2026 16:45

Update release notes

86536ee

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax force-pushed the err-bounds branch from f77670c to 86536ee Compare July 21, 2026 14:46

llucax requested a review from daniel-zullo-frequenz July 21, 2026 14:46

daniel-zullo-frequenz approved these changes Jul 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve `Bounds` validation and error handling#254

Improve `Bounds` validation and error handling#254
llucax wants to merge 16 commits into
frequenz-floss:v0.x.xfrom
llucax:err-bounds

llucax commented Jul 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

llucax commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llucax commented Jul 17, 2026 •

edited

Loading