Replace Bounds and Lifetime with frequenz.core.math.Interval#232
Closed
llucax wants to merge 8 commits into
Closed
Replace Bounds and Lifetime with frequenz.core.math.Interval#232llucax wants to merge 8 commits into
Bounds and Lifetime with frequenz.core.math.Interval#232llucax wants to merge 8 commits into
Conversation
llucax
requested review from
florian-wagner-frequenz
and removed request for
a team
github-actions
Bot
added
part:docs
Contributor
Author
|
It will be a little simpler after frequenz-floss/frequenz-core-python#178 is merged and released. |
Introduce `bounds_from_proto2` and `bounds_from_proto_with_issues2` which return `frequenz.core.math.Interval[float | None]` instead of the local `Bounds` type. `Bounds` is being retired in favor of the generic `Interval` type from `frequenz-core`; these new symbols provide the migration path for callers. The existing `bounds_from_proto`/`_with_issues` remain unchanged and will be deprecated in a follow-up commit. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Rewrite `Bounds` as a thin, deprecated subclass of `frequenz.core.math.Interval[float | None]`, exposing the historical `lower` / `upper` attribute names as deprecated `@property` aliases for `Interval.start` / `Interval.end`. `Bounds` now inherits Interval's validation, containment, string conversion, and dataclass semantics; two overrides are added: * `__eq__` — cross-compares against any `Interval` instance so `Bounds(lower=1, upper=2) == Interval(1, 2)` is `True`. Without this, dataclass-generated equality would reject mismatched types and break the migration UX. * `__hash__` — restored (Python nullifies auto-hash whenever `__eq__` is overridden) and matches Interval's hashing of `(start, end)`. Because `Bounds` now IS-A `Interval[float | None]`, code accepting `Interval[float | None]` accepts `Bounds` instances. This unlocks migrating field types on `MetricSample`, `ElectricalComponent`, and `ElectricalComponentConnection` from `Bounds` to `Interval` transparently. Two visible behavioural changes on `Bounds`: * Invalid-bounds `ValueError` message text moves from Bounds's old wording (`"Lower bound (X) must be less than or equal to upper bound (Y)"`) to Interval's (`"The start (X) can't be bigger than end (Y)"`). * `str(Bounds(lower=None, upper=None))` now returns `"[∞, ∞]"` instead of `"[None, None]"` (`Interval`'s `__str__`). We ignore the deprecation inside `bounds_from_proto()` to avoid a double deprecation message because the function itself will be deprecated in an upcoming commit. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Change the `MetricSample.bounds` field type from `list[Bounds]` to `Sequence[Interval[float | None]]`. `Sequence` (covariant) allows callers to keep passing `list[Bounds]` now that `Bounds` is a subclass of `Interval[float | None]`; it also accepts pure `list[Interval[float | None]]`, `tuple[Interval[float | None], ...]`, etc.. This is a soft-breaking type-level change: downstream code holding a variable typed `list[Bounds]` and passing it to `MetricSample` still type-checks under Sequence covariance, and code reading `sample.bounds` still supports iteration and indexing but no longer supports `.append()` or other list-mutation methods. Runtime behaviour is unchanged (the class is frozen — mutation was never legal). Allowing mutation of a frozen `dataclass` was a bug anyways, it should have never been allowed. The internal proto converter `_metric_bounds_from_proto` is rewritten in place to build `list[Interval[float | None]]` directly via `bounds_from_proto2`, so freshly-parsed samples no longer carry deprecated `Bounds` instances. Existing `bounds_from_proto` remains unchanged (deprecation lands in a follow-up commit). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Change the `metric_config_bounds` field on `ElectricalComponent` from `Mapping[Metric | int, Bounds]` to `Mapping[Metric | int, Interval[float | None]]`. `ElectricalComponent` is still unreleased, so this is a direct type replacement, no need for backwards-compatibility. The private converter `_metric_config_bounds_from_proto`, now calls `bounds_from_proto2` internally so freshly-parsed components no longer carry deprecated `Bounds` instances. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Change the `operational_lifetime` field on `ElectricalComponent` and `ElectricalComponentConnection` from `Lifetime` to `Interval[datetime | None]`. Both classes are still unreleased, so this is a direct type replacement, no backwards-compatibility needed. The default factory yields an unbounded `Interval[datetime | None](None, None)` matching the historical "always operational" semantics of `Lifetime()`. `lifetime_from_proto` is rewritten to return `Interval[datetime | None]` directly. The invalid-lifetime error message text in the affected tests moves from `"Start (X) must be before or equal to end (Y)"` (from `Lifetime.__post_init__`) to `"The start (X) can't be bigger than end (Y)"` (from `Interval.__post_init__`). The `Lifetime` class itself will be removed in a follow-up commit. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Delete the `Lifetime` dataclass and its tests. `Lifetime` was added in the current unreleased development cycle, and the previous commit migrated every consumer to `frequenz.core.math.Interval`, so there is no remaining user of the class. The `lifetime_from_proto` conversion function stays — it still parses `lifetime_pb2.Lifetime` protobuf messages, only its return type is now `Interval[datetime | None]`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Mark the two `Bounds`-returning proto converters `bounds_from_proto*` and `bounds_from_proto_with_issues` as `@deprecated`, pointing callers at `bounds_from_proto2` and `bounds_from_proto_with_issues2` respectively. Both new variants return `frequenz.core.math.Interval[float | None]` and are already used by every internal call site. `bounds_from_proto_with_issues` delegates to `bounds_from_proto`; the internal call is wrapped in `warnings.catch_warnings()` so callers of `_with_issues` see one `DeprecationWarning` (the outer one), not two. The internal `Bounds(...)` construction inside `bounds_from_proto` remains wrapped in `warnings.catch_warnings()` (added when `Bounds` became `@deprecated`), so the two suppressions compose correctly and each caller entry point emits exactly one warning identifying its own replacement symbol. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Marenz
approved these changes
Jul 6, 2026
Contributor
Author
|
We had an internal discussion and @shsms was concerned about removing the domain-specific language from |
auto-merge was automatically disabled
July 6, 2026 09:43
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wrapping
BoundsandLifetimeis bringing protobuf low-level concepts into the high-level API.These 2 messages provide an interval with a start and an end, exactly what
frequenz.interval.Intervalprovides.We replace both:
Boundsvia a deprecation process, as it was already released.Lifetimevia a direct replacement, as it was not yet released.The deprecated
Boundsclass now inherits fromInterval, so it can be passed in places where anIntervalis expected. Newbounds_from_proto*2()functions are provided to convert from protobuf messages toIntervalobjects, and the oldbounds_from_proto*()functions (without the2suffix) are deprecated.The
Lifetimetype is completely removed, thelifetime_from_proto*()functions now return anIntervalinstead.