Update the timeout type in power Request

The power Request is at least exposed through power
Result in BatteryPool. Using timedelta offers improved
code readability, time unit handling, making it a more
reliable choice for working with time intervals.

Signed-off-by: Daniel Zullo <[email protected]>

Author: daniel-zullo-frequenz

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

@@ -98,7 +98,7 @@ class PowerDistributingActor(Actor):
 
     It is recommended to wait for PowerDistributingActor output with timeout. Otherwise if
     the processing function fails then the response will never come.
-    The timeout should be Result:request_timeout_sec + time for processing the request.
+    The timeout should be Result:request_timeout + time for processing the request.
 
     Edge cases:
     * If there are 2 requests to be processed for the same subset of batteries, then
@@ -285,7 +285,7 @@ async def _run(self) -> None:
             )
 
             failed_power, failed_batteries = await self._set_distributed_power(
-                api, distribution, request.request_timeout_sec
+                api, distribution, request.request_timeout
             )
 
             response: Success | PartialFailure
@@ -321,14 +321,14 @@ async def _set_distributed_power(
         self,
         api: MicrogridApiClient,
         distribution: DistributionResult,
-        timeout_sec: float,
+        timeout: timedelta,
     ) -> Tuple[float, Set[int]]:
         """Send distributed power to the inverters.
 
         Args:
             api: Microgrid api client
             distribution: Distribution result
-            timeout_sec: How long wait for the response
+            timeout: How long wait for the response
 
         Returns:
             Tuple where first element is total failed power, and the second element
@@ -341,13 +341,13 @@ async def _set_distributed_power(
 
         _, pending = await asyncio.wait(
             tasks.values(),
-            timeout=timeout_sec,
+            timeout=timeout.total_seconds(),
             return_when=ALL_COMPLETED,
         )
 
         await self._cancel_tasks(pending)
 
-        return self._parse_result(tasks, distribution.distribution, timeout_sec)
+        return self._parse_result(tasks, distribution.distribution, timeout)
 
     def _get_power_distribution(
         self, request: Request, inv_bat_pairs: List[InvBatPair]
@@ -632,7 +632,7 @@ def _parse_result(
         self,
         tasks: Dict[int, asyncio.Task[None]],
         distribution: Dict[int, float],
-        request_timeout_sec: float,
+        request_timeout: timedelta,
     ) -> Tuple[float, Set[int]]:
         """Parse the results of `set_power` requests.
 
@@ -644,7 +644,7 @@ def _parse_result(
                 set the power for this inverter. Each task should be finished or cancelled.
             distribution: A dictionary where the key is the inverter ID and the value is how much
                 power was set to the corresponding inverter.
-            request_timeout_sec: The timeout that was used for the request.
+            request_timeout: The timeout that was used for the request.
 
         Returns:
             A tuple where the first element is the total failed power, and the second element is
@@ -678,7 +678,7 @@ def _parse_result(
                 _logger.warning(
                     "Battery %d didn't respond in %f sec. Mark it as broken.",
                     battery_id,
-                    request_timeout_sec,
+                    request_timeout.total_seconds(),
                 )
 
         return failed_power, failed_batteries

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

@@ -6,6 +6,7 @@
 
 import dataclasses
 from collections import abc
+from datetime import timedelta
 
 
 @dataclasses.dataclass
@@ -25,7 +26,7 @@ class Request:
     batteries: abc.Set[int]
     """The component ids of the batteries to be used for this request."""
 
-    request_timeout_sec: float = 5.0
+    request_timeout: timedelta = timedelta(seconds=5.0)
     """The maximum amount of time to wait for the request to be fulfilled."""
 
     adjust_power: bool = True

src/frequenz/sdk/timeseries/battery_pool/battery_pool.py

@@ -146,7 +146,7 @@ async def set_power(
                 power=power.as_watts(),
                 batteries=self._batteries,
                 adjust_power=adjust_power,
-                request_timeout_sec=request_timeout.total_seconds(),
+                request_timeout=request_timeout,
                 include_broken_batteries=include_broken_batteries,
             )
         )
@@ -193,7 +193,7 @@ async def charge(
                 power=as_watts,
                 batteries=self._batteries,
                 adjust_power=adjust_power,
-                request_timeout_sec=request_timeout.total_seconds(),
+                request_timeout=request_timeout,
                 include_broken_batteries=include_broken_batteries,
             )
         )
@@ -240,7 +240,7 @@ async def discharge(
                 power=-as_watts,
                 batteries=self._batteries,
                 adjust_power=adjust_power,
-                request_timeout_sec=request_timeout.total_seconds(),
+                request_timeout=request_timeout,
                 include_broken_batteries=include_broken_batteries,
             )
         )

tests/actor/power_distributing/test_power_distributing.py

@@ -114,7 +114,7 @@ async def test_power_distributor_one_user(self, mocker: MockerFixture) -> None:
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
         )
 
         attrs = {"get_working_batteries.return_value": request.batteries}
@@ -135,7 +135,7 @@ async def test_power_distributor_one_user(self, mocker: MockerFixture) -> None:
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -196,15 +196,15 @@ async def test_power_distributor_exclusion_bounds(
                 namespace=self._namespace,
                 power=0.0,
                 batteries={9, 19},
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
             )
 
             await channel.new_sender().send(request)
             result_rx = channel_registry.new_receiver(self._namespace)
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -222,15 +222,15 @@ async def test_power_distributor_exclusion_bounds(
                 namespace=self._namespace,
                 power=300.0,
                 batteries={9, 19},
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
             )
 
             await channel.new_sender().send(request)
             result_rx = channel_registry.new_receiver(self._namespace)
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -264,7 +264,7 @@ async def test_battery_soc_nan(self, mocker: MockerFixture) -> None:
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
         )
 
         attrs = {"get_working_batteries.return_value": request.batteries}
@@ -291,7 +291,7 @@ async def test_battery_soc_nan(self, mocker: MockerFixture) -> None:
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -327,7 +327,7 @@ async def test_battery_capacity_nan(self, mocker: MockerFixture) -> None:
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
         )
         attrs = {"get_working_batteries.return_value": request.batteries}
         mocker.patch(
@@ -347,7 +347,7 @@ async def test_battery_capacity_nan(self, mocker: MockerFixture) -> None:
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -399,7 +399,7 @@ async def test_battery_power_bounds_nan(self, mocker: MockerFixture) -> None:
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
         )
         attrs = {"get_working_batteries.return_value": request.batteries}
         mocker.patch(
@@ -419,7 +419,7 @@ async def test_battery_power_bounds_nan(self, mocker: MockerFixture) -> None:
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -447,7 +447,7 @@ async def test_power_distributor_invalid_battery_id(
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 100},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
         )
 
         attrs = {"get_working_batteries.return_value": request.batteries}
@@ -468,7 +468,7 @@ async def test_power_distributor_invalid_battery_id(
 
             done, _ = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(done) == 1
@@ -494,7 +494,7 @@ async def test_power_distributor_one_user_adjust_power_consume(
             namespace=self._namespace,
             power=1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
             adjust_power=False,
         )
 
@@ -517,7 +517,7 @@ async def test_power_distributor_one_user_adjust_power_consume(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -545,7 +545,7 @@ async def test_power_distributor_one_user_adjust_power_supply(
             namespace=self._namespace,
             power=-1200.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
             adjust_power=False,
         )
 
@@ -568,7 +568,7 @@ async def test_power_distributor_one_user_adjust_power_supply(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -596,7 +596,7 @@ async def test_power_distributor_one_user_adjust_power_success(
             namespace=self._namespace,
             power=1000.0,
             batteries={9, 19},
-            request_timeout_sec=SAFETY_TIMEOUT,
+            request_timeout=SAFETY_TIMEOUT,
             adjust_power=False,
         )
 
@@ -619,7 +619,7 @@ async def test_power_distributor_one_user_adjust_power_success(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
         assert len(pending) == 0
@@ -660,15 +660,15 @@ async def test_not_all_batteries_are_working(self, mocker: MockerFixture) -> Non
                 namespace=self._namespace,
                 power=1200.0,
                 batteries=batteries,
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
             )
 
             await channel.new_sender().send(request)
             result_rx = channel_registry.new_receiver(self._namespace)
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -709,7 +709,7 @@ async def test_use_all_batteries_none_is_working(
                 namespace=self._namespace,
                 power=1200.0,
                 batteries={9, 19},
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
                 include_broken_batteries=True,
             )
 
@@ -718,7 +718,7 @@ async def test_use_all_batteries_none_is_working(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -761,7 +761,7 @@ async def test_force_request_a_battery_is_not_working(
                 namespace=self._namespace,
                 power=1200.0,
                 batteries=batteries,
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
                 include_broken_batteries=True,
             )
 
@@ -770,7 +770,7 @@ async def test_force_request_a_battery_is_not_working(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -814,7 +814,7 @@ async def test_force_request_battery_nan_value_non_cached(
                 namespace=self._namespace,
                 power=1200.0,
                 batteries=batteries,
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
                 include_broken_batteries=True,
             )
 
@@ -841,7 +841,7 @@ async def test_force_request_battery_nan_value_non_cached(
 
             done, pending = await asyncio.wait(
                 [asyncio.create_task(result_rx.receive())],
-                timeout=SAFETY_TIMEOUT,
+                timeout=SAFETY_TIMEOUT.total_seconds(),
             )
 
             assert len(pending) == 0
@@ -884,7 +884,7 @@ async def test_force_request_batteries_nan_values_cached(
                 namespace=self._namespace,
                 power=1200.0,
                 batteries=batteries,
-                request_timeout_sec=SAFETY_TIMEOUT,
+                request_timeout=SAFETY_TIMEOUT,
                 include_broken_batteries=True,
             )
 
@@ -893,7 +893,7 @@ async def test_force_request_batteries_nan_values_cached(
             async def test_result() -> None:
                 done, pending = await asyncio.wait(
                     [asyncio.create_task(result_rx.receive())],
-                    timeout=SAFETY_TIMEOUT,
+                    timeout=SAFETY_TIMEOUT.total_seconds(),
                 )
                 assert len(pending) == 0
                 assert len(done) == 1