Add gRPC calls to control components #158

llucax · 2025-06-13T09:13:53Z

Functionality for managing component operational limits has been implemented with AddComponentBounds, supported by a new Bounds wrapper.

The client now supports setting active and reactive power for components through the SetComponentPowerActive and SetComponentPowerReactive methods. These methods share a common underlying implementation and test suite, with minor adaptations for the specific power type being controlled.

A new base Component class has been added, which will serve as a common ancestor for all component types. This facilitates shared functionality and enables the use of algebraic data types for more robust match statements and exhaustiveness checks. Sub-classes will be added in follow-up pull requests.

The metrics module has been restructured into a package to better organize upcoming additions. A Metric enum has been introduced, and support for loading AggregatedMetricValue and Lifetime data from protobuf messages has been added.

Part of #55.

Copilot

Pull Request Overview

This PR adds gRPC calls for controlling component power and bounds with new methods in the microgrid client, along with reorganized metrics and component definitions. Key changes include:

Implementation of set_component_power_active, set_component_power_reactive, and add_component_bounds methods in the client.
Addition of new component base class and improved metrics package structure.
Comprehensive test cases for various power and bounds scenarios.

Reviewed Changes

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

Show a summary per file

File	Description
tests/client_test_cases/set_component_power_active/*	Test cases for handling invalid power and lifetime parameters
tests/client_test_cases/add_component_bounds/*	Test cases validating bounds addition behavior
src/frequenz/client/microgrid/metrics/_bounds_proto.py	Added bounds conversion from protobuf messages
src/frequenz/client/microgrid/metrics/_bounds.py	Added Bounds dataclass with validation logic
src/frequenz/client/microgrid/_client.py	New gRPC methods for setting power and adding bounds, along with argument validation
src/frequenz/client/microgrid/component/*	New base component and associated categorization and status definitions

src/frequenz/client/microgrid/metrics/_bounds_proto.py

src/frequenz/client/microgrid/metrics/_bounds.py

src/frequenz/client/microgrid/_client.py

src/frequenz/client/microgrid/metrics/_metric.py

shsms · 2025-06-30T12:57:28Z

tests/metrics/test_bounds.py

+def test_creation() -> None:
+    """Test creation of Bounds with valid values."""
+    bounds = Bounds(lower=-10.0, upper=10.0)
+    assert bounds.lower == -10.0
+    assert bounds.upper == 10.0


maybe also a case where lower and upper are the same values?

tests/component/__init__.py

shsms · 2025-06-30T14:03:49Z

src/frequenz/client/microgrid/_client.py

+def _delta_to_seconds(delta: timedelta | None) -> int | None:
+    """Convert a `timedelta` to seconds (or `None` if `None`)."""
+    return round(delta.total_seconds()) if delta is not None else None


This returns float | None, not int | None

True. Interesting that mypy doesn't catch this 🤔

.... looking 🔍 ......

It is not reported because it does return int. It seems to be undocumented, but it actually returns int if ndigits is not specified:

class _SupportsRound1(Protocol[_T_co]): def __round__(self) -> _T_co: ... class _SupportsRound2(Protocol[_T_co]): def __round__(self, ndigits: int, /) -> _T_co: ... @overload def round(number: _SupportsRound1[_T], ndigits: None = None) -> _T: ... @overload def round(number: _SupportsRound2[_T], ndigits: SupportsIndex) -> _T: ...

and for float:

@overload def __round__(self, ndigits: None = None, /) -> int: ... @overload def __round__(self, ndigits: SupportsIndex, /) -> float: ...

💥

tests/client_test_cases/set_component_power_active/no_lifetime_case.py

tests/client_test_cases/set_component_power_active/success_case.py

shsms · 2025-06-30T14:25:50Z

src/frequenz/client/microgrid/_client.py

+        If multiple inclusion bounds have been provided for a metric, then the
+        overlapping bounds are merged into a single bound, and non-overlapping
+        bounds are kept separate.


The API also specifies that there can't be more than two non-overlapping bounds, right? In that case, it would be nice if the docs for the add_component_bounds method specify that users should be careful not to add more than two non-overlapping bounds.

Not sure what do you mean by this? It says explicitly there that overlapping bounds will be merged.

I have this vague memory that these were supposed to exactly replace inclusion/exclusion bounds, so there will not be more than two non-overlapping bounds in the new scheme, and that that was going to be codified in the docs.

But that doesn't appear to be the case, so nvm.

oh, yeah, I remember that too, not sure what's the current state of this, if that limitation still holds or have been lifted. Since one can add bounds, I guess it is not true anymore. @tiyash-basu-frequenz ?

src/frequenz/client/microgrid/_client.py

Signed-off-by: Leandro Lucarella <[email protected]>

Arguments of `pytest`s are pretty self descriptive, as they come from figures, parametrization or other decorators. Signed-off-by: Leandro Lucarella <[email protected]>

We will need to add more metrics-related wrappers in the future, so it is better to make it a package. Signed-off-by: Leandro Lucarella <[email protected]>

Signed-off-by: Leandro Lucarella <[email protected]>

This class will be used to combine classic inheritance (all components will derive from it), a mix-in to provide common functionality of all components, and also build "algebraic data types", so they can be used in `match` statements and easily do exhaustion checks to make sure all possible types of components are handled when needed. Note it doesn't make sense to make this a `abc.ABC` because there are no abstract methods. Signed-off-by: Leandro Lucarella <[email protected]>

These 2 calls are identically, they just affect active or reactive power respectively, so there are a few utility function that are shared, as well as test cases. For test cases we need to use a couple of hacks to share them. * `*_case.py` files for `set_reactive_power` are symlinks to `set_active_power()` * Both have their own `_config.py` file where we define the classes used for the tests, so we can share them, as only the request/response objects change between both. * Because of the client test framework works, `_config` must be imported as a top-level module. * We need to ignore `pylint` and `mypy` checks when importing, as they can't find the `_config` module at the top-level. Signed-off-by: Leandro Lucarella <[email protected]>

Signed-off-by: Leandro Lucarella <[email protected]>

llucax

Updated to fix all comments of stuff that needed fixing. I added more cases to the bounds creation tests.

llucax · 2025-07-01T10:12:50Z

src/frequenz/client/microgrid/_client.py

+        If multiple inclusion bounds have been provided for a metric, then the
+        overlapping bounds are merged into a single bound, and non-overlapping
+        bounds are kept separate.


Not sure what do you mean by this? It says explicitly there that overlapping bounds will be merged.

llucax · 2025-07-01T10:24:48Z

src/frequenz/client/microgrid/_client.py

+def _delta_to_seconds(delta: timedelta | None) -> int | None:
+    """Convert a `timedelta` to seconds (or `None` if `None`)."""
+    return round(delta.total_seconds()) if delta is not None else None


True. Interesting that mypy doesn't catch this 🤔

.... looking 🔍 ......

It is not reported because it does return int. It seems to be undocumented, but it actually returns int if ndigits is not specified:

class _SupportsRound1(Protocol[_T_co]): def __round__(self) -> _T_co: ... class _SupportsRound2(Protocol[_T_co]): def __round__(self, ndigits: int, /) -> _T_co: ... @overload def round(number: _SupportsRound1[_T], ndigits: None = None) -> _T: ... @overload def round(number: _SupportsRound2[_T], ndigits: SupportsIndex) -> _T: ...

and for float:

@overload def __round__(self, ndigits: None = None, /) -> int: ... @overload def __round__(self, ndigits: SupportsIndex, /) -> float: ...

💥

llucax · 2025-07-01T10:50:55Z

Also added a new commit to fix the Raises: in the already existing rpc calls.

llucax · 2025-07-01T10:51:00Z

Enabling auto-merge.

shsms · 2025-07-01T14:43:04Z

Also added a new commit to fix the Raises: in the already existing rpc calls.

Not sure I see it.

llucax · 2025-07-02T15:05:27Z

1c8a132 (#158)

shsms · 2025-07-02T15:25:31Z

I see, you meant the typo. The description sounded like something more significant. 🤔

Copilot AI review requested due to automatic review settings June 13, 2025 09:13

llucax requested review from a team as code owners June 13, 2025 09:13

llucax requested review from daniel-zullo-frequenz and thea-leake June 13, 2025 09:13

llucax self-assigned this Jun 13, 2025

github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:client Affects the client code labels Jun 13, 2025

llucax requested review from shsms and tiyash-basu-frequenz June 13, 2025 09:14

llucax added the type:enhancement New feature or enhancement visitble to users label Jun 13, 2025

Copilot AI reviewed Jun 13, 2025

View reviewed changes

src/frequenz/client/microgrid/metrics/_bounds_proto.py Outdated Show resolved Hide resolved

src/frequenz/client/microgrid/metrics/_bounds.py Outdated Show resolved Hide resolved

src/frequenz/client/microgrid/_client.py Outdated Show resolved Hide resolved

llucax force-pushed the component-control branch from 93ee4c4 to d069f10 Compare June 13, 2025 09:19

llucax added this to the v0.17.0 milestone Jun 13, 2025

llucax added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jun 13, 2025

llucax force-pushed the component-control branch from d069f10 to 19932f5 Compare June 13, 2025 09:23

shsms reviewed Jun 30, 2025

View reviewed changes

llucax mentioned this pull request Jul 1, 2025

Improve Metric doc frequenz-floss/frequenz-api-common#382

Open

llucax added 10 commits July 1, 2025 12:39

Fix typo in docstring

1c8a132

Signed-off-by: Leandro Lucarella <[email protected]>

Add loading of Lifetime from protobuf

0590d81

Signed-off-by: Leandro Lucarella <[email protected]>

Use a one-liner docstring for a Lifetime test

ad03c63

Arguments of `pytest`s are pretty self descriptive, as they come from figures, parametrization or other decorators. Signed-off-by: Leandro Lucarella <[email protected]>

Move the metrics module to a package

c305970

We will need to add more metrics-related wrappers in the future, so it is better to make it a package. Signed-off-by: Leandro Lucarella <[email protected]>

Add loading of AggregatedMetricValue from protobuf

e416a80

Signed-off-by: Leandro Lucarella <[email protected]>

Add Metric enum

02be569

Signed-off-by: Leandro Lucarella <[email protected]>

Add Bounds wrapper

8460b64

Signed-off-by: Leandro Lucarella <[email protected]>

Implement AddComponentBounds

3c69ffe

Signed-off-by: Leandro Lucarella <[email protected]>

llucax force-pushed the component-control branch from a5070e1 to 3c69ffe Compare July 1, 2025 10:48

llucax commented Jul 1, 2025

View reviewed changes

llucax enabled auto-merge July 1, 2025 10:51

shsms approved these changes Jul 2, 2025

View reviewed changes

llucax merged commit bd5828f into frequenz-floss:v0.17.x Jul 2, 2025
5 checks passed

llucax deleted the component-control branch July 2, 2025 15:26

llucax mentioned this pull request Aug 27, 2025

Implement ReceiveComponentDataStreamRequest #178

Merged

Add gRPC calls to control components #158

Add gRPC calls to control components #158

Uh oh!

Conversation

llucax commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax commented Jul 1, 2025

Uh oh!

llucax commented Jul 1, 2025

Uh oh!

shsms commented Jul 1, 2025

Uh oh!

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

Uh oh!

Uh oh!

shsms commented Jul 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

llucax commented Jun 13, 2025 •

edited

Loading

llucax commented Jul 2, 2025 •

edited

Loading