Remove circular dependency in BatteriesStatus

And simplify BatteriesStatus code.

Signed-off-by: ela-kotulska-frequenz <[email protected]>

Author: ela-kotulska-frequenz

src/frequenz/sdk/actor/power_distributing/_batteries_status.py

@@ -10,7 +10,6 @@
 
 from ..._internal.asyncio import AsyncConstructible
 from ...microgrid._battery import BatteryStatus, BatteryStatusTracker
-from .result import PartialFailure, Result, Success
 
 _logger = logging.getLogger(__name__)
 
@@ -72,8 +71,6 @@ def get_working_batteries(self, battery_ids: Set[int]) -> Set[int]:
             battery_ids: batteries ids
 
         Raises:
-            RuntimeError: If `async_init` method was not called at the beginning to
-                initialized object.
             KeyError: If any battery in the given batteries is not in the pool.
 
         Returns:
@@ -101,29 +98,23 @@ def get_working_batteries(self, battery_ids: Set[int]) -> Set[int]:
         )
         return uncertain
 
-    def update_last_request_status(self, result: Result):
+    def update_status(self, succeed_batteries: Set[int], failed_batteries: Set[int]):
         """Update batteries in pool based on the last result from the request.
 
         Args:
-            result: Summary of what batteries failed and succeed in last request.
-
-        Raises:
-            RuntimeError: If `async_init` method was not called at the beginning to
-                initialize object.
+            succeed_batteries: Batteries that succeed request
+            failed_batteries: Batteries that failed request.
         """
-        if isinstance(result, Success):
-            for bat_id in result.used_batteries:
-                self._batteries[bat_id].unblock()
-
-        elif isinstance(result, PartialFailure):
-            for bat_id in result.failed_batteries:
-                duration = self._batteries[bat_id].block()
-                if duration > 0:
-                    _logger.warning(
-                        "Battery %d failed last response. Block it for %f sec",
-                        bat_id,
-                        duration,
-                    )
-
-            for bat_id in result.succeed_batteries:
-                self._batteries[bat_id].unblock()
+        for battery_id in succeed_batteries:
+            self._batteries[battery_id].unblock()
+
+        for battery_id in failed_batteries:
+            duration = self._batteries[battery_id].block()
+            if duration > 0:
+                _logger.warning(
+                    "Battery %d failed last response. Block it for %f sec",
+                    battery_id,
+                    duration,
+                )
+
+            self._batteries[battery_id].block()

src/frequenz/sdk/actor/power_distributing/power_distributing.py

@@ -308,14 +308,15 @@ async def run(self) -> None:
                     excess_power=distribution.remaining_power,
                 )
             else:
+                succeed_batteries = set(battery_distribution.keys())
                 response = Success(
                     request=request,
                     succeed_power=distributed_power_value,
-                    used_batteries=set(battery_distribution.keys()),
+                    used_batteries=succeed_batteries,
                     excess_power=distribution.remaining_power,
                 )
 
-            battery_pool.update_last_request_status(response)
+            battery_pool.update_status(succeed_batteries, failed_batteries)
             await user.channel.send(response)
 
     async def _set_distributed_power(

tests/actor/test_battery_pool.py

@@ -689,12 +689,8 @@ def scenario_block_component(
 
             failed_batteries = {pairs[2].bat_id, pairs[4].bat_id}
             # Notify that batteries 4 and 6 failed in last request.
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed_batteries,
-                succeed_batteries=batteries - failed_batteries,
-            )
-            battery_pool.update_last_request_status(resp)
+            succeed_batteries = batteries - failed_batteries
+            battery_pool.update_status(succeed_batteries, failed_batteries)
             expected = expected - failed_batteries
             working_batteries = battery_pool.get_working_batteries(batteries)
             assert working_batteries == expected
@@ -707,12 +703,8 @@ def scenario_block_component(
             failed_batteries = {pairs[0].bat_id}
             # Notify that another battery failed in last request. Now we have three
             # batteries blocked.
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed_batteries,
-                succeed_batteries=working_batteries - failed_batteries,
-            )
-            battery_pool.update_last_request_status(resp)
+            succeed_batteries = working_batteries - failed_batteries
+            battery_pool.update_status(succeed_batteries, failed_batteries)
             expected = expected - failed_batteries
             working_batteries = battery_pool.get_working_batteries(batteries)
             assert working_batteries == expected
@@ -729,10 +721,7 @@ def scenario_block_component(
             working_batteries = battery_pool.get_working_batteries(batteries)
             assert working_batteries == expected
 
-            success_resp: Success = self.fake_Success(
-                requested_batteries=batteries, used_batteries=batteries
-            )
-            battery_pool.update_last_request_status(success_resp)
+            battery_pool.update_status(batteries, set())
             assert battery_pool.get_working_batteries(batteries) == batteries
 
     def scenario_blocking_timeout_increases(
@@ -757,13 +746,8 @@ def scenario_blocking_timeout_increases(
             for timeout in expected_blocking_timeout:
                 # Notify that batteries failed in last request.
                 failed_batteries = {pairs[idx].bat_id for idx in [2, 4, 0]}
-                resp = self.fake_PartialFailure(
-                    requested_batteries=batteries,
-                    failed_batteries=failed_batteries,
-                    succeed_batteries=expected - failed_batteries,
-                )
-
-                battery_pool.update_last_request_status(resp)
+                succeed_batteries = expected - failed_batteries
+                battery_pool.update_status(succeed_batteries, failed_batteries)
                 expected = expected - failed_batteries
                 assert battery_pool.get_working_batteries(batteries) == expected
 
@@ -784,12 +768,8 @@ def scenario_blocking_timeout_increases(
             # Now the next timeout should be 30 (Tested in loop above).
             # Notify that battery all batteries, except 4 succeed.
             failed_batteries = {pairs[4].bat_id}
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed_batteries,
-                succeed_batteries=batteries - failed_batteries,
-            )
-            battery_pool.update_last_request_status(resp)
+            succeed_batteries = batteries - failed_batteries
+            battery_pool.update_status(succeed_batteries, failed_batteries)
             expected = batteries - failed_batteries
             assert battery_pool.get_working_batteries(batteries) == expected
 
@@ -804,12 +784,8 @@ def scenario_blocking_timeout_increases(
             # Battery 1 should has timeout 2 sec (it reset)
             # Battery 4 should not reset its timeout (30 sec since last fail response.)
             failed_batteries = {pairs[4].bat_id, pairs[1].bat_id}
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed_batteries,
-                succeed_batteries=batteries - failed_batteries,
-            )
-            battery_pool.update_last_request_status(resp)
+            succeed_batteries = batteries - failed_batteries
+            battery_pool.update_status(succeed_batteries, failed_batteries)
             working_batteries = battery_pool.get_working_batteries(batteries)
             # Battery 4 should be blocked since last call
             expected = expected - {pairs[1].bat_id}
@@ -829,11 +805,7 @@ def scenario_blocking_timeout_increases(
             self.set_inverter_message(battery_pool, pairs, inverter_data)
             expected = expected | {pairs[4].bat_id}
             assert battery_pool.get_working_batteries(batteries) == expected
-
-            success_resp: Success = self.fake_Success(
-                requested_batteries=batteries, used_batteries=batteries
-            )
-            battery_pool.update_last_request_status(success_resp)
+            battery_pool.update_status(batteries, set())
             assert battery_pool.get_working_batteries(batteries) == batteries
 
     def scenario_state_changes_for_blocked_component(
@@ -851,14 +823,10 @@ def scenario_state_changes_for_blocked_component(
             batteries = {pair.bat_id for pair in pairs}
             expected = batteries
 
-            failed = {pairs[idx].bat_id for idx in range(3)}
             # Notify that batteries batteries: 0..2 failed in last request.
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed,
-                succeed_batteries=batteries - failed,
-            )
-            battery_pool.update_last_request_status(resp)
+            failed = {pairs[idx].bat_id for idx in range(3)}
+            succeed_batteries = batteries - failed
+            battery_pool.update_status(succeed_batteries, failed)
             expected = batteries - failed
             assert battery_pool.get_working_batteries(batteries) == expected
 
@@ -888,10 +856,7 @@ def scenario_state_changes_for_blocked_component(
             expected = expected = expected | {pairs[3].bat_id}
             assert battery_pool.get_working_batteries(batteries) == expected
 
-            success_resp: Success = self.fake_Success(
-                requested_batteries=batteries, used_batteries=batteries
-            )
-            battery_pool.update_last_request_status(success_resp)
+            battery_pool.update_status(batteries, set())
             assert battery_pool.get_working_batteries(batteries) == batteries
 
     def scenario_unknown_batteries_are_used(
@@ -912,12 +877,8 @@ def scenario_unknown_batteries_are_used(
 
             # Notify that battery 0 failed in last request.
             failed = {pairs[0].bat_id}
-            resp = self.fake_PartialFailure(
-                requested_batteries=batteries,
-                failed_batteries=failed,
-                succeed_batteries=batteries - failed,
-            )
-            battery_pool.update_last_request_status(resp)
+            succeed_batteries = batteries - failed
+            battery_pool.update_status(succeed_batteries, failed)
             expected = expected - failed
             assert battery_pool.get_working_batteries(batteries) == expected