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.

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

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>

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 · 2026-07-22T09:14:00Z

Pushed some docs fixes.

llucax · 2026-07-22T09:50:09Z

Doing a self-review I realized Bounds is accepting nan, but I will do the bug fix in a separate PR later, otherwise there will be too much rebasing and conflict resolution for the branches I already have ready

llucax · 2026-07-22T10:13:21Z

I will wait for @shsms review (as he was the main critic for #232, which resulted in this PR instead) before merging.

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

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 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 previously approved these changes Jul 21, 2026

View reviewed changes

llucax added 16 commits July 22, 2026 10:52

Update release notes

c217991

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

llucax dismissed daniel-zullo-frequenz’s stale review via c217991 July 22, 2026 08:53

llucax force-pushed the err-bounds branch from 86536ee to c217991 Compare July 22, 2026 08:53

llucax disabled auto-merge July 22, 2026 09:14

daniel-zullo-frequenz approved these changes Jul 22, 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!

llucax commented Jul 22, 2026

Uh oh!

llucax commented Jul 22, 2026

Uh oh!

llucax commented Jul 22, 2026

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!

llucax commented Jul 22, 2026

Uh oh!

llucax commented Jul 22, 2026

Uh oh!

llucax commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llucax commented Jul 17, 2026 •

edited

Loading