From bea94497b755a7498010d79ca5949de4bbd9aa83 Mon Sep 17 00:00:00 2001 From: Elzbieta Kotulska Date: Wed, 31 Jul 2024 21:15:51 +0200 Subject: [PATCH 1/6] Fix PV power distribution To exclude PV inverters that didn't send any data. The power distributor crashes because it tried to get data from a component that didn't send any data since the application startup. Signed-off-by: Elzbieta Kotulska --- RELEASE_NOTES.md | 2 +- .../_pv_inverter_manager.py | 22 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 1b7dcc491..a5d1f3a7f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -2,4 +2,4 @@ ## Bug Fixes -- Fix getting reactive power from meters, inverters and EV chargers. +- Fix PV power distribution excluding inverters that haven't sent any data since the application started. diff --git a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py index 2a2f77852..987491d36 100644 --- a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py +++ b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py @@ -113,11 +113,19 @@ async def distribute_power(self, request: Request) -> None: raise ValueError( "Cannot distribute power to PV inverters without any inverters" ) - working_components = list( - self._component_pool_status_tracker.get_working_components( - request.component_ids - ) - ) + + working_components: list[int] = [] + for inv_id in self._component_pool_status_tracker.get_working_components( + request.component_ids + ): + if self._component_data_caches[inv_id].has_value(): + working_components.append(inv_id) + else: + _logger.warning( + "Exclude inverter %s from distribution, because it didn't " + "send any data since the application startup.", + inv_id, + ) # When sorting by lower bounds, which are negative for PV inverters, we have to # reverse the order, so that the inverters with the higher bounds i.e., the @@ -130,6 +138,10 @@ async def distribute_power(self, request: Request) -> None: ) num_components = len(working_components) + if num_components == 0: + _logger.error("No inverters available for power distribution. Aborting.") + return + for idx, inv_id in enumerate(working_components): # Request powers are negative for PV inverters. When remaining power is # greater than 0.0, we can stop allocating further. From 894fcf21bbbce086c72bffb2c8e35d3a7c31d86c Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Thu, 2 May 2024 14:45:06 +0200 Subject: [PATCH 2/6] Set zero power for PV inverters not neccessary to reach target power If desired power is already reached without the need to use some inverters, their powers need to be set to zero. If not, their original powers would remain and the target powers would be wrong. In case the requested power is zero, this was also leading to crashes because there were no allocations. This change also fixes that issue. Signed-off-by: Sahas Subramanian --- .../_pv_inverter_manager.py | 3 ++- .../_pv_pool/test_pv_pool_control_methods.py | 23 ++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py index 987491d36..b616d3369 100644 --- a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py +++ b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py @@ -148,7 +148,8 @@ async def distribute_power(self, request: Request) -> None: if remaining_power > Power.zero() or is_close_to_zero( remaining_power.as_watts() ): - break + allocations[inv_id] = Power.zero() + continue distribution = remaining_power / float(num_components - idx) inv_data = self._component_data_caches[inv_id] if not inv_data.has_value(): diff --git a/tests/timeseries/_pv_pool/test_pv_pool_control_methods.py b/tests/timeseries/_pv_pool/test_pv_pool_control_methods.py index 492a01ead..1a6cea279 100644 --- a/tests/timeseries/_pv_pool/test_pv_pool_control_methods.py +++ b/tests/timeseries/_pv_pool/test_pv_pool_control_methods.py @@ -131,7 +131,7 @@ async def _recv_reports_until( if check(report): break - async def test_setting_power( + async def test_setting_power( # pylint: disable=too-many-statements self, mocks: _Mocks, mocker: MockerFixture, @@ -260,3 +260,24 @@ async def test_setting_power( mocker.call(inv_ids[2], -30000.0), mocker.call(inv_ids[3], -30000.0), ] + + # Setting 0 power should set all inverters to 0 + set_power.reset_mock() + await pv_pool.propose_power(Power.zero()) + await self._recv_reports_until( + bounds_rx, + lambda x: x.target_power is not None and x.target_power.as_watts() == 0.0, + ) + self._assert_report( + await bounds_rx.receive(), power=0.0, lower=-100000.0, upper=0.0 + ) + await asyncio.sleep(0.0) + + assert set_power.call_count == 4 + inv_ids = mocks.microgrid.pv_inverter_ids + assert sorted(set_power.call_args_list, key=lambda x: x.args[0]) == [ + mocker.call(inv_ids[0], 0.0), + mocker.call(inv_ids[1], 0.0), + mocker.call(inv_ids[2], 0.0), + mocker.call(inv_ids[3], 0.0), + ] From 841fea8eee53fdba1f5817c77e0e52a0c795fbbd Mon Sep 17 00:00:00 2001 From: cwasicki <126617870+cwasicki@users.noreply.github.com> Date: Fri, 9 Aug 2024 15:49:34 +0200 Subject: [PATCH 3/6] Clear release notes Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com> --- RELEASE_NOTES.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index a5d1f3a7f..3d521e2e3 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1,5 +1 @@ # Frequenz Python SDK Release Notes - -## Bug Fixes - -- Fix PV power distribution excluding inverters that haven't sent any data since the application started. From f8d65029a1543c02ad4635eea7c64b4475eebc04 Mon Sep 17 00:00:00 2001 From: cwasicki <126617870+cwasicki@users.noreply.github.com> Date: Fri, 9 Aug 2024 15:02:18 +0200 Subject: [PATCH 4/6] Fix gap in ring buffer when updating a missing value If there are no gaps in the ring buffer and the new value is missing and creates a gap in time, we need to start the new gap after the latest value from before. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com> --- RELEASE_NOTES.md | 4 ++++ src/frequenz/sdk/timeseries/_ringbuffer/buffer.py | 5 ++++- tests/timeseries/test_ringbuffer.py | 10 +++++----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 3d521e2e3..445a7b984 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -1 +1,5 @@ # Frequenz Python SDK Release Notes + +## Bug Fixes + +- Fixes a bug in the ring buffer in case the updated value is missing and creates a gap in time. diff --git a/src/frequenz/sdk/timeseries/_ringbuffer/buffer.py b/src/frequenz/sdk/timeseries/_ringbuffer/buffer.py index f431ab039..bac39a46d 100644 --- a/src/frequenz/sdk/timeseries/_ringbuffer/buffer.py +++ b/src/frequenz/sdk/timeseries/_ringbuffer/buffer.py @@ -491,8 +491,11 @@ def _update_gaps( # New missing entry that is not already in a gap? if record_as_missing: if not found_in_gaps: + # If there are no gaps and the new value is not subsequent to the + # newest value, we need to start the new gap after the newest value + start_gap = min(newest + self._sampling_period, timestamp) self._gaps.append( - Gap(start=timestamp, end=timestamp + self._sampling_period) + Gap(start=start_gap, end=timestamp + self._sampling_period) ) elif len(self._gaps) > 0: if found_in_gaps: diff --git a/tests/timeseries/test_ringbuffer.py b/tests/timeseries/test_ringbuffer.py index b3da1292d..ffdb5cd96 100644 --- a/tests/timeseries/test_ringbuffer.py +++ b/tests/timeseries/test_ringbuffer.py @@ -277,12 +277,12 @@ def test_gaps() -> None: # pylint: disable=too-many-statements assert buffer.count_covered() == 5 assert len(buffer.gaps) == 0 - # whole range gap suffers from sdk#646 + # whole range gap buffer.update(Sample(dt(99), None)) - assert buffer.oldest_timestamp == dt(95) # bug: should be None - assert buffer.newest_timestamp == dt(99) # bug: should be None - assert buffer.count_valid() == 4 # bug: should be 0 (whole range gap) - assert buffer.count_covered() == 5 # bug: should be 0 + assert buffer.oldest_timestamp is None + assert buffer.newest_timestamp is None + assert buffer.count_valid() == 0 + assert buffer.count_covered() == 0 assert len(buffer.gaps) == 1 From dbc16db0db83d1eee57bd2e729479e3d8d16ba98 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 2 Aug 2024 15:36:03 +0200 Subject: [PATCH 5/6] Replace `Task.exception()` with `Task.result()` `Task.exception()` returns raised exceptions as values, unless tasks raised `CancelledError`, in which case, `Task.exception()` raises `CancelledError` as well: https://docs.python.org/3/library/asyncio-task.html#asyncio.Task.exception `Task.result()` provides a more consistent interface, by always re-raising all caught exceptions. Signed-off-by: Sahas Subramanian --- .../_ev_charger_manager.py | 40 +++++++++--------- .../_pv_inverter_manager.py | 41 ++++++++++--------- 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/src/frequenz/sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py b/src/frequenz/sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py index 2ac8b8254..9cfd163ec 100644 --- a/src/frequenz/sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py +++ b/src/frequenz/sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py @@ -323,29 +323,29 @@ async def _set_api_power( succeeded_components: set[int] = set() failed_power = Power.zero() for component_id, task in tasks.items(): - exc = task.exception() - if exc is not None: - failed_components.add(component_id) - failed_power += target_power_changes[component_id] + try: + task.result() + except asyncio.CancelledError: + _logger.warning( + "Timeout while setting power to EV charger %s", component_id + ) + except grpc.aio.AioRpcError as exc: + _logger.warning( + "Error while setting power to EV charger %s: %s", + component_id, + exc, + ) + except Exception: # pylint: disable=broad-except + _logger.exception( + "Unknown error while setting power to EV charger: %s", component_id + ) else: succeeded_components.add(component_id) + continue + + failed_components.add(component_id) + failed_power += target_power_changes[component_id] - match task.exception(): - case asyncio.CancelledError(): - _logger.warning( - "Timeout while setting power to EV charger %s", component_id - ) - case grpc.aio.AioRpcError() as err: - _logger.warning( - "Error while setting power to EV charger %s: %s", - component_id, - err, - ) - case Exception(): - _logger.exception( - "Unknown error while setting power to EV charger: %s", - component_id, - ) if failed_components: return PartialFailure( failed_components=failed_components, diff --git a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py index b616d3369..38876f1e9 100644 --- a/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py +++ b/src/frequenz/sdk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py @@ -196,29 +196,30 @@ async def _set_api_power( # pylint: disable=too-many-locals succeeded_components: set[int] = set() failed_power = Power.zero() for component_id, task in tasks.items(): - exc = task.exception() - if exc is not None: - failed_components.add(component_id) - failed_power += allocations[component_id] + try: + task.result() + except asyncio.CancelledError: + _logger.warning( + "Timeout while setting power to PV inverter %s", component_id + ) + except grpc.aio.AioRpcError as exc: + _logger.warning( + "Error while setting power to PV inverter %s: %s", + component_id, + exc, + ) + except Exception: # pylint: disable=broad-except + _logger.exception( + "Unknown error while setting power to PV inverter: %s", + component_id, + ) else: succeeded_components.add(component_id) + continue + + failed_components.add(component_id) + failed_power += allocations[component_id] - match task.exception(): - case asyncio.CancelledError(): - _logger.warning( - "Timeout while setting power to PV inverter %s", component_id - ) - case grpc.aio.AioRpcError() as err: - _logger.warning( - "Error while setting power to PV inverter %s: %s", - component_id, - err, - ) - case Exception(): - _logger.exception( - "Unknown error while setting power to PV inverter: %s", - component_id, - ) if failed_components: await self._results_sender.send( PartialFailure( From c8cf967640e12e534255076fd90dff2a0136c9e1 Mon Sep 17 00:00:00 2001 From: Sahas Subramanian Date: Fri, 9 Aug 2024 17:15:10 +0200 Subject: [PATCH 6/6] Update release notes Signed-off-by: Sahas Subramanian --- RELEASE_NOTES.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 445a7b984..73f8ac0a5 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -3,3 +3,5 @@ ## Bug Fixes - Fixes a bug in the ring buffer in case the updated value is missing and creates a gap in time. + +- Fixed a bug that was causing the `PowerDistributor` to exit if power requests to PV inverters or EV chargers timeout.