Make all `float` annotations `float | int` by llucax · Pull Request #256 · frequenz-floss/frequenz-client-common-python

llucax · 2026-07-21T12:32:53Z

PEP 484's numeric tower makes int assignable to any float-annotated parameter or field, even under mypy --strict, while at runtime isinstance(1, float) is False. Any field annotated float can therefore silently store an int, and code unwrapping it with match … case float(): falls through to assert_never(), calls to float-only methods like hex() crash, and type()-based dispatch misbehaves — despite everything type-checking cleanly.

There is no clean fix in current Python (see the discussion in #250): runtime coercion at ingress was prototyped and measured ~2.3× slower on hot-path types, structural Protocol tricks don't close the widened variable and Sequence covariance holes, and narrowing match arms one by one leaves the annotation lying. So the decision is to stop lying instead: annotate every such value as float | int, which is what PEP 484 actually admits, at zero runtime cost. It is also forward-compatible with the typing-council proposal to make float mean float | int (python/typing-council#46).

This commit replaces all float annotations with FloatInt, a type alias for float | int.

Fixes #250.

llucax · 2026-07-21T12:33:45Z

Draft because it is based on #255, but other than that it should be ready for review.

Copilot

Pull request overview

This PR updates frequenz-client-common’s public surface to be explicit about Python’s PEP 484 numeric-tower behavior by introducing a FloatInt = float | int alias and propagating it through affected dataclasses and accessors, preventing “looks exhaustive” runtime logic (e.g. match … case float():) from silently falling through on int values.

Changes:

Add FloatInt type alias and update relevant float-annotated fields/accessors to use it (e.g. metrics samples, bounds, location, transformer voltages).
Rework bounds handling to preserve malformed wire data (InvalidBounds, InvalidBoundsSet) and add normalization/membership semantics (BoundsSet) plus new semantic accessors (MetricSample.get_bounds_set(), ElectricalComponent.get_metric_config_bounds()).
Deprecate older proto conversion helpers (bounds_from_proto, bounds_from_proto_with_issues) in favor of bounds_from_proto2.

Reviewed changes

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

Show a summary per file

File	Description
tests/types/_location/test_location.py	Extends location tests to accept/reject `int` lat/lon via `FloatInt`.
tests/types/_location/test_invalid_longitude.py	Verifies `InvalidLongitude` preserves `int` values and string formatting.
tests/types/_location/test_invalid_latitude.py	Verifies `InvalidLatitude` preserves `int` values and string formatting.
tests/test_float.py	Adds tests for the new `FloatInt` alias.
tests/microgrid/proto/v1alpha8/test_microgrid.py	Updates microgrid conversion tests (drops log assertions).
tests/microgrid/electrical_components/test_power_transformer.py	Updates transformer voltage typing/tests to `FloatInt`.
tests/microgrid/electrical_components/test_electrical_component_base.py	Adds tests for metric-config bounds accessor behavior.
tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py	Updates proto parsing tests for bounds/invalid-bounds preservation.
tests/metrics/test_sample_metric_sample.py	Updates MetricSample tests for `bounds_set`, invalid bounds sets, and `FloatInt` values.
tests/metrics/test_sample_aggregated_value.py	Updates aggregated metric value tests for `FloatInt` inputs.
tests/metrics/test_bounds.py	Removes old Bounds tests (replaced by new bounds test package).
tests/metrics/proto/v1alpha8/test_sample_metric_sample.py	Updates proto MetricSample tests for `bounds_set` + invalid bounds behavior.
tests/metrics/proto/v1alpha8/test_bounds.py	Adds coverage for `bounds_from_proto2` and deprecations.
tests/metrics/_bounds/test_invalid_bounds.py	New unit tests for `InvalidBounds`.
tests/metrics/_bounds/test_invalid_bounds_set.py	New unit tests for `InvalidBoundsSet`.
tests/metrics/_bounds/test_invalid_bounds_set_error.py	New unit tests for `InvalidBoundsSetError`.
tests/metrics/_bounds/test_invalid_bounds_error.py	New unit tests for `InvalidBoundsError`.
tests/metrics/_bounds/test_bounds.py	New unit tests for `Bounds` (incl. `FloatInt` storage + membership).
tests/metrics/_bounds/test_bounds_set.py	New unit tests for `BoundsSet` normalization and membership.
tests/metrics/_bounds/test_base_bounds.py	New unit tests for non-instantiability of `BaseBounds`.
tests/metrics/_bounds/init.py	Introduces bounds test package.
src/frequenz/client/common/types/_location.py	Updates location types and accessors to use `FloatInt`.
src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py	Simplifies microgrid conversion and changes warning/log behavior.
src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py	Updates parsing of metric bounds to preserve invalid bounds using `bounds_from_proto2`.
src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py	Updates transformer voltages to `FloatInt`.
src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py	Adds semantic accessor `get_metric_config_bounds()` and supports `InvalidBounds`.
src/frequenz/client/common/microgrid/electrical_components/init.py	Exports new typing helper(s) and updated symbols.
src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py	Builds `BoundsSet`/`InvalidBoundsSet` from wire bounds using `bounds_from_proto2`.
src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py	Adds `bounds_from_proto2` and deprecates older converters.
src/frequenz/client/common/metrics/proto/v1alpha8/init.py	Exports `bounds_from_proto2`.
src/frequenz/client/common/metrics/_sample.py	Migrates MetricSample to `bounds_set`, adds deprecations/backcompat, adds `get_bounds_set()`.
src/frequenz/client/common/metrics/_bounds.py	Introduces Base/Valid/Invalid bounds hierarchy plus `BoundsSet` and errors.
src/frequenz/client/common/metrics/init.py	Re-exports new bounds/bounds-set types and errors.
src/frequenz/client/common/_float.py	Adds `FloatInt` type alias and explanatory documentation.
src/frequenz/client/common/init.py	Exports `FloatInt` at package root.
RELEASE_NOTES.md	Adds upgrade notes and documents new APIs/deprecations.

Comments suppressed due to low confidence (1)

src/frequenz/client/common/metrics/_bounds.py:70

Bounds.__contains__ treats NaN items as not contained, but Bounds.__post_init__ currently allows NaN in lower/upper. With NaN bounds, comparisons become non-total (e.g. item < nan is False), which can make containment checks accept values they shouldn’t and can also break sorting/merging in BoundsSet. Consider rejecting NaN bounds at construction time.

    def __post_init__(self) -> None:
        """Validate these bounds."""
        if self.lower is None:
            return
        if self.upper is None:
            return
        if self.lower > self.upper:
            raise ValueError(
                f"Lower bound ({self.lower}) must be less than or equal to upper "
                f"bound ({self.upper})"
            )

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

Copilot

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/frequenz/client/common/metrics/_bounds.py:66

Bounds.__contains__() (and BoundsSet normalization) assumes lower/upper are ordered values. If either bound is NaN, the comparisons are non-transitive (x < nan and x > nan are both False), so membership tests can incorrectly return True for arbitrary values (e.g. 0 in Bounds(lower=math.nan, upper=10.0) would be True). Consider rejecting NaN bounds in __post_init__ so malformed wire data is surfaced as InvalidBounds via bounds_from_proto2.

    def __post_init__(self) -> None:
        """Validate these bounds."""
        if self.lower is None:
            return
        if self.upper is None:
            return
        if self.lower > self.upper:

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

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

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

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

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

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

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

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

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

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

PEP 484's numeric tower makes `int` assignable to any `float`-annotated parameter or field, even under `mypy --strict`, while at runtime `isinstance(1, float)` is `False`. Any field annotated `float` can therefore silently store an `int`, and code unwrapping it with `match … case float():` falls through to `assert_never()`, calls to `float`-only methods like `hex()` crash, and `type()`-based dispatch misbehaves — despite everything type-checking cleanly. There is no clean fix in current Python (see the discussion in frequenz-floss#250): runtime coercion at ingress was prototyped and measured ~2.3× slower on hot-path types, structural `Protocol` tricks don't close the widened variable and `Sequence` covariance holes, and narrowing match arms one by one leaves the annotation lying. So the decision is to stop lying instead: annotate every such value as `float | int`, which is what PEP 484 actually admits, at zero runtime cost. It is also forward-compatible with the typing-council proposal to make `float` mean `float | int` (python/typing-council#46). Add the `FloatInt` alias as the canonical spelling of that union. The alias gives the workaround a single documented home — explaining the trap, the `bool ⊂ int` leak. Follow-up commits migrate the existing `float` annotations to it. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

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

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

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 21, 2026

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

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Comment thread tests/test_float.py

Comment thread src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py

Comment thread RELEASE_NOTES.md

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

llucax force-pushed the float-int branch from 9be1cd8 to 36b6f0f 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/__init__.py

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

llucax mentioned this pull request Jul 21, 2026

Add a new CategorySpecificInfo wrapper #257

Draft

llucax force-pushed the float-int branch 3 times, most recently from 44a5a3b to b36dd75 Compare July 22, 2026 09:12

llucax added 14 commits August 11, 2026 12:20

Update release notes

43ea977

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

Use FloatInt in metric bounds

558e20e

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

Use FloatInt in metric samples

f2c4f36

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

Use FloatInt in Location

f7011dc

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

llucax added 2 commits August 11, 2026 12:21

Use FloatInt in PowerTransformer

380314f

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

Update release notes

c2395af

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

llucax force-pushed the float-int branch from b36dd75 to c2395af Compare August 11, 2026 11:24

Navigation Menu

Uh oh!

Make all `float` annotations `float | int` - #256

Make all `float` annotations `float | int`#256
llucax wants to merge 16 commits into
frequenz-floss:v0.x.xfrom
llucax:float-int

llucax commented Jul 21, 2026

Uh oh!

llucax commented Jul 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

llucax commented Jul 21, 2026

Uh oh!

llucax commented Jul 21, 2026

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants