Refactor power distributor's Results (#305)

We make `Result` and its subclasses dataclasses so they have a proper
`__str__()` method that shows the details.

We also make some property renaming to make it more consistent and
improve documentation.

Fixes #275.

Author: llucax

RELEASE_NOTES.md

@@ -8,6 +8,10 @@
 
 * Update BatteryStatus to mark battery with unknown capacity as not working (#263)
 * The channels dependency was updated to v0.14.0 (#292)
+* Some properties for `PowerDistributingActor` results were renamed to be more consistent between `Success` and `PartialFailure`:
+  * The `Success.used_batteries` property was renamed to `succeeded_batteries`.
+  * The `PartialFailure.success_batteries` property was renamed to `succeeded_batteries`.
+  * The `succeed_power` property was renamed to `succeeded_power` for both `Success` and `PartialFailure`.
 
 ## New Features
 
@@ -22,6 +26,7 @@
       )
       grid_power = microgrid.logical_meter().grid_power()
   ```
+* The `Result` class (and subclasses) for the `PowerDistributingActor` are now dataclasses, so logging them will produce a more detailed output.
 
 ## Bug Fixes
 

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

@@ -271,12 +271,12 @@ async def run(self) -> None:
                     request.batteries
                 )
             except KeyError as err:
-                await user.channel.send(Error(request, str(err)))
+                await user.channel.send(Error(request=request, msg=str(err)))
                 continue
 
             if len(pairs_data) == 0:
                 error_msg = f"No data for the given batteries {str(request.batteries)}"
-                await user.channel.send(Error(request, str(error_msg)))
+                await user.channel.send(Error(request=request, msg=str(error_msg)))
                 continue
 
             try:
@@ -285,7 +285,7 @@ async def run(self) -> None:
                 )
             except ValueError as err:
                 error_msg = f"Couldn't distribute power, error: {str(err)}"
-                await user.channel.send(Error(request, str(error_msg)))
+                await user.channel.send(Error(request=request, msg=str(error_msg)))
                 continue
 
             distributed_power_value = request.power - distribution.remaining_power
@@ -309,8 +309,8 @@ async def run(self) -> None:
                 succeed_batteries = set(battery_distribution.keys()) - failed_batteries
                 response = PartialFailure(
                     request=request,
-                    succeed_power=distributed_power_value,
-                    succeed_batteries=succeed_batteries,
+                    succeeded_power=distributed_power_value,
+                    succeeded_batteries=succeed_batteries,
                     failed_power=failed_power,
                     failed_batteries=failed_batteries,
                     excess_power=distribution.remaining_power,
@@ -319,8 +319,8 @@ async def run(self) -> None:
                 succeed_batteries = set(battery_distribution.keys())
                 response = Success(
                     request=request,
-                    succeed_power=distributed_power_value,
-                    used_batteries=succeed_batteries,
+                    succeeded_power=distributed_power_value,
+                    succeeded_batteries=succeed_batteries,
                     excess_power=distribution.remaining_power,
                 )
 
@@ -380,17 +380,17 @@ def _check_request(self, request: Request) -> Optional[Result]:
                     f"No battery {battery}, available batteries: "
                     f"{list(self._battery_receivers.keys())}"
                 )
-                return Error(request, msg)
+                return Error(request=request, msg=msg)
 
         if not request.adjust_power:
             if request.power < 0:
                 bound = self._get_lower_bound(request.batteries)
                 if request.power < bound:
-                    return OutOfBound(request, bound)
+                    return OutOfBound(request=request, bound=bound)
             else:
                 bound = self._get_upper_bound(request.batteries)
                 if request.power > bound:
-                    return OutOfBound(request, bound)
+                    return OutOfBound(request=request, bound=bound)
 
         return None
 
@@ -420,7 +420,7 @@ def _remove_duplicated_requests(
             # Generators seems to be the fastest
             if prev_request.batteries == batteries:
                 task = asyncio.create_task(
-                    prev_user.channel.send(Ignored(prev_request))
+                    prev_user.channel.send(Ignored(request=prev_request))
                 )
                 to_ignore.append(task)
             # Use generators as generators seems to be the fastest.
@@ -490,7 +490,7 @@ async def _wait_for_request(self, user: _User) -> None:
                     "Consider increasing size of the queue."
                 )
                 _logger.error(msg)
-                await user.channel.send(Error(request, str(msg)))
+                await user.channel.send(Error(request=request, msg=str(msg)))
             else:
                 self._request_queue.put_nowait((request, user))
                 await asyncio.gather(*tasks)

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

@@ -2,22 +2,30 @@
 # Copyright © 2022 Frequenz Energy-as-a-Service GmbH
 """Definition of the user request."""
 
-from dataclasses import dataclass
-from typing import Set
+from __future__ import annotations
 
+import dataclasses
 
-@dataclass
+
+@dataclasses.dataclass
 class Request:
-    """Request from the user."""
+    """Request to set power to the `PowerDistributingActor`."""
 
-    # How much power to set
     power: int
-    # In which batteries the power should be set
-    batteries: Set[int]
-    # Timeout for the server to respond on the requests.
+    """The requested power in watts."""
+
+    batteries: set[int]
+    """The component ids of the batteries to be used for this request."""
+
     request_timeout_sec: float = 5.0
-    # If True and requested power value is above upper bound, then the power will be
-    # decreased to match the bounds. Only the decreased power will be set.
-    # If False and the requested power is above upper bound, then request won't
-    # be processed. result.OutOfBound message will be send back to the user.
+    """The maximum amount of time to wait for the request to be fulfilled."""
+
     adjust_power: bool = True
+    """Whether to adjust the power to match the bounds.
+
+    If `True`, the power will be adjusted (lowered) to match the bounds, so
+    only the reduced power will be set.
+
+    If `False` and the power is outside the batteries' bounds, the request will
+    fail and be replied to with an `OutOfBound` result.
+    """

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

@@ -3,122 +3,100 @@
 
 """Results from PowerDistributingActor."""
 
-from abc import ABC
-from typing import Set
+from __future__ import annotations
+
+import dataclasses
 
 from .request import Request
 
 
-class Result(ABC):
-    """Base class for the power distributor result."""
-
-    def __init__(self, request: Request) -> None:
-        """Create class instance.
-
-        Args:
-            request: The user's request to which this message responds.
-        """
-        self.request: Request = request
-
-
-class Success(Result):
-    """Send if setting power for all batteries succeed."""
-
-    def __init__(
-        self,
-        request: Request,
-        succeed_power: int,
-        used_batteries: Set[int],
-        excess_power: int,
-    ) -> None:
-        """Create class instance.
-
-        Args:
-            request: The user's request to which this message responds.
-            succeed_power: Part of the requested power that was successfully set.
-            used_batteries: Subset of the requested batteries, that were used to
-                realize the request.
-            excess_power: Part of the requested power that could not be fulfilled,
-                because it was outside available power bounds.
-        """
-        super().__init__(request)
-        self.succeed_power: int = succeed_power
-        self.used_batteries: Set[int] = used_batteries
-        self.excess_power: int = excess_power
-
-
-class PartialFailure(Result):
-    """Send if any battery failed and didn't perform the request."""
-
-    # It is very simple class with only data so it should be ok to disable pylint.
-    # All these results should be dataclass but in python < 3.10 it is risky
-    # to derive after dataclass.
-    def __init__(  # pylint: disable=too-many-arguments
-        self,
-        request: Request,
-        succeed_power: int,
-        succeed_batteries: Set[int],
-        failed_power: int,
-        failed_batteries: Set[int],
-        excess_power: int,
-    ) -> None:
-        """Create class instance.
-
-        Args:
-            request: The user's request to which this message responds.
-            succeed_power: Part of the requested power that was successfully set.
-            succeed_batteries: Subset of the requested batteries for which the request
-                succeed.
-            failed_power: Part of the requested power that failed.
-            failed_batteries: Subset of the requested batteries for which the request
-                failed.
-            excess_power: Part of the requested power that could not be fulfilled,
-                because it was outside available power bounds.
-        """
-        super().__init__(request)
-        self.succeed_power: int = succeed_power
-        self.succeed_batteries: Set[int] = succeed_batteries
-        self.failed_power: int = failed_power
-        self.failed_batteries: Set[int] = failed_batteries
-        self.excess_power: int = excess_power
+@dataclasses.dataclass
+class _BaseResultMixin:
+    """Base mixin class for reporting power distribution results."""
 
+    request: Request
+    """The user's request to which this message responds."""
+
+
+# When moving to Python 3.10+ we should replace this with an union type:
+# Result = Success | PartialFailure | Error | OutOfBound | Ignored
+# For now it can't be done because before 3.10 isinstance(result, Success)
+# doesn't work, so it is hard to figure out what type of result you got in
+# a forward compatible way.
+# When moving we should use the _BaseResultMixin as a base class for all
+# results.
+@dataclasses.dataclass
+class Result(_BaseResultMixin):
+    """Power distribution result."""
+
+
+@dataclasses.dataclass
+class _BaseSuccessMixin:
+    """Result returned when setting the power succeed for all batteries."""
+
+    succeeded_power: int
+    """The part of the requested power that was successfully set."""
+
+    succeeded_batteries: set[int]
+    """The subset of batteries for which power was set successfully."""
+
+    excess_power: int
+    """The part of the requested power that could not be fulfilled.
+
+    This happens when the requested power is outside the available power bounds.
+    """
 
-class Error(Result):
-    """Error occurred and power was not set."""
 
-    def __init__(self, request: Request, msg: str) -> None:
-        """Create class instance.
+# We need to put the _BaseSuccessMixin before Result in the inheritance list to
+# make sure that the Result attributes appear before the _BaseSuccessMixin,
+# otherwise the request attribute will be last in the dataclass constructor
+# because of how MRO works.
 
-        Args:
-            request: The user's request to which this message responds.
-            msg: Error message explaining why error happened.
-        """
-        super().__init__(request)
-        self.msg: str = msg
 
+@dataclasses.dataclass
+class Success(_BaseSuccessMixin, Result):  # Order matters here. See above.
+    """Result returned when setting the power succeeded for all batteries."""
 
+
+@dataclasses.dataclass
+class PartialFailure(_BaseSuccessMixin, Result):
+    """Result returned when any battery failed to perform the request."""
+
+    failed_power: int
+    """The part of the requested power that failed to be set."""
+
+    failed_batteries: set[int]
+    """The subset of batteries for which the request failed."""
+
+
+@dataclasses.dataclass
+class Error(Result):
+    """Result returned when an error occurred and power was not set at all."""
+
+    msg: str
+    """The error message explaining why error happened."""
+
+
+@dataclasses.dataclass
 class OutOfBound(Result):
-    """Send if power was not set because requested power was not within bounds.
+    """Result returned when the power was not set because it was out of bounds.
 
-    This message is send only if Request.adjust_power = False.
+    This result happens when the originating request was done with
+    `adjust_power = False` and the requested power is not within the batteries bounds.
     """
 
-    def __init__(self, request: Request, bound: int) -> None:
-        """Create class instance.
+    bound: int
+    """The total power bound for the requested batteries.
 
-        Args:
-            request: The user's request to which this message responds.
-            bound: Total power bound for the requested batteries.
-                If requested power < 0, then this value is lower bound.
-                Otherwise it is upper bound.
-        """
-        super().__init__(request)
-        self.bound: int = bound
+    If the requested power negative, then this value is the lower bound.
+    Otherwise it is upper bound.
+    """
 
 
+@dataclasses.dataclass
 class Ignored(Result):
-    """Send if request was ignored.
+    """Result returned when the request was ignored.
 
-    Request was ignored because new request for the same subset of batteries
-    was received.
+    The request can be ignored when a new request for the same subset of
+    batteries was received.
     """