Add a new CategorySpecificInfo wrapper - #257
Conversation
996ca26 to
a9bce0f
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands the client’s forward-compatibility surface by introducing a typed home for “leftover” protobuf fields (via CategorySpecificInfo) and by making numeric and bounds handling more robust/explicit across the library. In addition to the electrical-component changes described in the PR metadata, it also introduces a new FloatInt alias and a new bounds/bounds-set hierarchy with deprecations in the metrics proto converters and MetricSample.
Changes:
- Add
CategorySpecificInfoand plumbcategory_specific_info: CategorySpecificInfo | Nonethrough all electrical components (including mismatched/unrecognized cases) during proto conversion. - Introduce
FloatInt = float | intand update several “float” fields/accessors to reflect the runtime numeric tower. - Add
InvalidBounds/BoundsSet/InvalidBoundsSet+ new accessors (MetricSample.get_bounds_set(),ElectricalComponent.get_metric_config_bounds()), and deprecate older bounds conversion/fields.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/types/_location/test_location.py | Extends Location tests to cover int latitude/longitude via FloatInt. |
| tests/types/_location/test_invalid_longitude.py | Adds coverage that InvalidLongitude preserves int values. |
| tests/types/_location/test_invalid_latitude.py | Adds coverage that InvalidLatitude preserves int values. |
| tests/test_float.py | New tests for the FloatInt type alias behavior at runtime. |
| tests/microgrid/proto/v1alpha8/test_microgrid.py | Removes assertions about warning logging during microgrid conversion. |
| tests/microgrid/electrical_components/test_problematic.py | Adds string formatting tests for unrecognized/mismatched components and new CategorySpecificInfo usage. |
| tests/microgrid/electrical_components/test_power_transformer.py | Updates transformer voltage tests to accept FloatInt. |
| tests/microgrid/electrical_components/test_inverter.py | Adds __str__ coverage for UnrecognizedInverter. |
| tests/microgrid/electrical_components/test_ev_charger.py | Adds __str__ coverage for UnrecognizedEvCharger. |
| tests/microgrid/electrical_components/test_electrical_component_base.py | Updates base component tests for category_specific_info and adds bounds accessor tests. |
| tests/microgrid/electrical_components/test_category_specific_info.py | New unit tests for CategorySpecificInfo construction/equality/hashing. |
| tests/microgrid/electrical_components/test_battery.py | Adds __str__ coverage for UnrecognizedBattery. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py | Ensures proto conversion preserves category-specific info kind/leftovers. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py | Verifies category mismatch preserves CategorySpecificInfo and adds category_name. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py | Updates base-data expectations and adds metric-config-bounds behavior tests. |
| tests/microgrid/electrical_components/proto/v1alpha8/conftest.py | Adjusts fixtures for category_specific_info=None and removes old metadata assertion. |
| tests/metrics/test_sample_metric_sample.py | Migrates sample tests from bounds to bounds_set, adds deprecation-path tests and new bounds-set error handling. |
| tests/metrics/test_sample_metric_connection.py | Updates MetricConnection.name semantics to empty-string default. |
| tests/metrics/test_sample_aggregated_value.py | Updates aggregated metric value tests to accept FloatInt. |
| tests/metrics/test_bounds.py | Removes old bounds tests (replaced by new tests/metrics/_bounds/*). |
| tests/metrics/proto/v1alpha8/test_sample_metric_sample.py | Updates proto sample conversion tests to use bounds_set and invalid-bounds preservation. |
| tests/metrics/proto/v1alpha8/test_sample_metric_connection.py | Updates proto connection test for empty-string name semantics. |
| tests/metrics/proto/v1alpha8/test_bounds.py | Adds coverage for new bounds_from_proto2 and deprecation warnings on old converters. |
| tests/metrics/_bounds/test_invalid_bounds.py | New tests for InvalidBounds. |
| tests/metrics/_bounds/test_invalid_bounds_set.py | New tests for InvalidBoundsSet. |
| tests/metrics/_bounds/test_invalid_bounds_set_error.py | New tests for InvalidBoundsSetError. |
| tests/metrics/_bounds/test_invalid_bounds_error.py | New tests for InvalidBoundsError. |
| tests/metrics/_bounds/test_bounds.py | New/updated tests for Bounds behavior (contains/bool/str, FloatInt handling). |
| tests/metrics/_bounds/test_bounds_set.py | New tests for BoundsSet normalization/containment/str/hash behavior. |
| tests/metrics/_bounds/test_base_bounds.py | New tests ensuring BaseBounds is abstract/non-instantiable. |
| tests/metrics/_bounds/init.py | New package marker for bounds tests. |
| src/frequenz/client/common/types/_location.py | Updates location types/accessors/errors to use FloatInt and handle int in pattern matching. |
| src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py | Removes warning aggregation/logging from microgrid_from_proto. |
| src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py | Adds category-name lookup, introduces CategorySpecificInfo parsing/leftover handling, preserves invalid bounds, and switches to bounds_from_proto2. |
| src/frequenz/client/common/microgrid/electrical_components/_problematic.py | Adds/updates __str__ for problematic components and adds category_name for mismatched cases. |
| src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py | Updates transformer voltage fields to FloatInt. |
| src/frequenz/client/common/microgrid/electrical_components/_inverter.py | Adds __str__ for UnrecognizedInverter. |
| src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py | Adds __str__ for UnrecognizedEvCharger. |
| src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py | Replaces category_specific_metadata with category_specific_info, preserves invalid bounds types, adds get_metric_config_bounds(), and changes __str__ formatting. |
| src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py | New CategorySpecificInfo dataclass with custom hashing. |
| src/frequenz/client/common/microgrid/electrical_components/_battery.py | Adds __str__ for UnrecognizedBattery. |
| src/frequenz/client/common/microgrid/electrical_components/init.py | Exports CategorySpecificInfo (and currently also exports DefaultT). |
| src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py | Switches proto sample conversion to produce bounds_set and preserves invalid bounds via type. |
| src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py | Adds bounds_from_proto2, and deprecates older bounds converters. |
| src/frequenz/client/common/metrics/proto/v1alpha8/init.py | Re-exports bounds_from_proto2. |
| src/frequenz/client/common/metrics/_sample.py | Introduces bounds_set, deprecates bounds, adds get_bounds_set(), and updates numeric types to FloatInt. |
| src/frequenz/client/common/metrics/_bounds.py | Adds BaseBounds, InvalidBounds, BoundsSet, InvalidBoundsSet, and related errors + membership/normalization logic. |
| src/frequenz/client/common/metrics/init.py | Exports the new bounds/bounds-set types and errors. |
| src/frequenz/client/common/_float.py | New FloatInt type alias module with rationale and examples. |
| src/frequenz/client/common/init.py | Re-exports FloatInt. |
| RELEASE_NOTES.md | Adds release notes for bounds deprecations, bounds-set APIs, and FloatInt (but currently omits the new category-specific info API). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py:42
CategorySpecificInfo.__hash__()hashes field values viarepr(value). For nested mappings/sets,repr()can depend on insertion order / hash randomization, so two instances that compare equal (e.g. equal nested dicts with different insertion order) can end up with different hashes, violating the hash/equality contract.
frozen_fields = tuple(
sorted((key, repr(value)) for key, value in self.fields.items())
)
return hash((self.kind, frozen_fields))
src/frequenz/client/common/microgrid/electrical_components/init.py:23
DefaultTis a typing-onlyTypeVarused internally for overloads; re-exporting it from the package surface (__init__.py) needlessly expands the public API and makes it harder to change internal typing details later.
from ._crypto_miner import CryptoMiner
from ._diagnostic_code import ElectricalComponentDiagnosticCode
from ._electrical_component import DefaultT, ElectricalComponent
from ._electrical_component_connection import (
src/frequenz/client/common/microgrid/electrical_components/init.py:92
DefaultT(a privateTypeVar) is included in__all__, effectively making it part of the public API. It should be kept internal to avoid locking in an implementation detail of type annotations.
"CryptoMiner",
"DcEvCharger",
"DefaultT",
"ElectricalComponent",
"ElectricalComponentCategory",
|
This is a draft because it is based on #256, but it should be ready for review. |
4b0aad6 to
14739e2
Compare
baa37c5 to
acc553a
Compare
The protobuf message field is required, so the string will always come. Adding `None` as a possibility in the type system forces user code to deal with `None`, and adds an ambiguity between `None` and `""`. So, as we did with other `name` or similar string fields, it is better to just leave it as a pure `str` and let users deal with the empty string more easily, without the need to special-case `None`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Introduce a small frozen dataclass pairing the protobuf `category_specific_info` variant name (`kind`) with the leftover fields that this client version did not translate into typed attributes. An electrical component may carry category-specific info on the wire. Known fields are decoded into typed attributes on the concrete component, but anything left over — because the category is unrecognized, or because a newer API version added fields this client doesn't know yet — was either dropped or kept as a bare `dict` with no indication of which variant it came from. This type gives that leftover data a documented home together with the variant name needed to interpret it. It is hashable like the components that will carry it. The leftover `fields` mapping may hold arbitrary, possibly unhashable values, and folding them into the hash — even through `repr()` — is unsafe: values that compare equal can hash or repr differently (e.g. `1 == 1.0 == True`), which would break the invariant that equal objects hash equally. So `fields` is excluded from the hash and instances hash on `kind` alone, mirroring `metric_config_bounds` on the component. Collisions between instances that share a `kind` but differ in `fields` are then possible, but harmless and highly unlikely in practice. A follow-up commit adopts it as the `category_specific_info` field on every electrical component. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Adopt `CategorySpecificInfo | None` as the `category_specific_info` field on `ElectricalComponent`, replacing the bare `Mapping[str, Any]` that only the mismatched component ever populated. The protobuf `category_specific_info` oneof is now decoded once into its `kind` plus a dict of its fields, then each concrete component drops the fields it translated into typed attributes (e.g. a battery's `type`), keeping the `kind` and any leftover. The result: * recognized components keep the variant `kind` with an empty leftover; * `UnrecognizedElectricalComponent` now preserves the carried info instead of dropping it — previously the escape hatch was empty for the very class that needs it most; * `MismatchedCategoryElectricalComponent` keeps the full info (nothing was translated), recording exactly which variant disagreed with the category; * components carrying no variant get `None`. Because `CategorySpecificInfo` is hashable, the field no longer needs `hash=False` and now participates in the component hash like the other fields. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Replace the `{id}<{Class}>{name}` rendering with `{id}:{name}:{Class}`, and
override `__str__` on the problematic subclasses so the raw wire value
that makes them problematic stays visible in the marker:
* recognized components render as `CID42:bat1:LiIonBattery` (the name is
always literal, so an empty name reads as `CID42::LiIonBattery`)
* `UnrecognizedBattery`/`UnrecognizedEvCharger`/`UnrecognizedInverter`
render as `CID42:bat1:Battery:type=99` — the hardcoded base label plus
the raw type
* `UnrecognizedElectricalComponent` renders as
`CID42:comp1:category=999` — the raw, unrecognized category
* `MismatchedCategoryElectricalComponent` renders as
`CID42:comp1:mismatched:category=5:kind=inverter` — the
declared category (as its enum name) against the carried variant kind.
Each subclass overrides `__str__` in full rather than sharing a
`__str__` hook on the base: the format is a single short f-string, the
tests pin it down, and the flat overrides read better than the
indirection a hook would need.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`MismatchedCategoryElectricalComponent.__str__` could only show the declared category as its raw int (`category=5`), because resolving the name would mean using the deprecated `ElectricalComponentCategory` wrapper enum. Store a `category_name` alongside the raw `category`, resolved by the converter from the protobuf enum descriptor (which is not deprecated) with the long `ELECTRICAL_COMPONENT_CATEGORY_` prefix stripped, so `__str__` renders the short, readable name: `CID42:comp1:mismatched:category=BATTERY:kind=inverter` Only the mismatched component needs it: its declared category is a recognized value that its class name hides. Unrecognized components have no name to show (that is what makes them unrecognized), and every other component encodes its category in its class name. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
acc553a to
1ac23b9
Compare
|
Ready for review. |
7364dc7
An electrical component may carry category-specific info on the wire. Known fields are decoded into typed attributes on the concrete component, but anything left over — because the category is unrecognized, or because a newer API version added fields this client doesn't know yet, was either dropped or kept as a bare
dictwith no indication of which variant it came from. This type gives that leftover data a documented home together with the variant name needed to interpret it.Some of this info was saved in the
category_specific_infofield for some component classes, but it was not saved consistently for all, and thekindof the variant was not preserved, making it hard to inspect unrecognized components or to debug mismatched categories.This PR introduces a small frozen dataclass pairing the protobuf
category_specific_infovariant name (kind) with the leftover fields that this client version did not translate into typed attributes and stores the carried category specific info on every component by making the atttributecategory_specific_infoof typeCategorySpecificInfo | None.The protobuf
category_specific_infooneof is now decoded once into itskindplus a dict of its fields, then each concrete component drops the fields it translated into typed attributes (e.g. a battery'stype), keeping thekindand any leftover. The result:kindwith an empty leftoverUnrecognizedElectricalComponentnow preserves the carried info instead of dropping it — previously the escape hatch was empty for the very class that needs it mostMismatchedCategoryElectricalComponentkeeps the full info (nothing was translated), recording exactly which variant disagreed with the categoryNoneIt also stores the declared category name on mismatched components so that
MismatchedCategoryElectricalComponent.__str__can show the declared category name instead of just the raw int.To do so
MismatchedCategoryElectricalComponentnow has a newcategory_nameattribute alongside the rawcategory, resolved by the converter from the protobuf enum descriptor with the longELECTRICAL_COMPONENT_CATEGORY_prefix stripped, so__str__renders the short, readable name:CID42:comp1:mismatched:category=BATTERY:kind=inverterOnly the mismatched component needs it: its declared category is a recognized value that its class name hides. Unrecognized components have no name to show (that is what makes them unrecognized), and every other component encodes its category in its class name.