From 4596dbb82fd38a77520c1edfc9af594752452ace Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 12 Mar 2025 11:51:29 +0100 Subject: [PATCH 1/8] Make `ComponentMetricsData` a `dataclass` This is mainly to get the nice `__str__` and `__repr__` methods for better debugging. Signed-off-by: Leandro Lucarella --- .../battery_pool/_component_metrics.py | 46 ++++--------------- 1 file changed, 10 insertions(+), 36 deletions(-) diff --git a/src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py b/src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py index 5155b1b46..05f9ffe3c 100644 --- a/src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py +++ b/src/frequenz/sdk/timeseries/battery_pool/_component_metrics.py @@ -5,48 +5,24 @@ from collections.abc import Mapping +from dataclasses import dataclass from datetime import datetime from frequenz.client.microgrid import ComponentMetricId +@dataclass(frozen=True, eq=False) class ComponentMetricsData: """Store values of the component metrics.""" - def __init__( - self, - component_id: int, - timestamp: datetime, - metrics: Mapping[ComponentMetricId, float], - ) -> None: - """Create class instance. + component_id: int + """The component ID the data is for.""" - Args: - component_id: component id - timestamp: timestamp the same for all metrics - metrics: map between metrics and its values. - """ - self._component_id = component_id - self._timestamp = timestamp - self._metrics: Mapping[ComponentMetricId, float] = metrics - - @property - def component_id(self) -> int: - """Get component id of the given metrics. + timestamp: datetime + """The timestamp for all the metrics.""" - Returns: - Component id - """ - return self._component_id - - @property - def timestamp(self) -> datetime: - """Get timestamp of the given metrics. - - Returns: - Timestamp (one for all metrics). - """ - return self._timestamp + metrics: Mapping[ComponentMetricId, float] + """The values for each metric.""" def get(self, metric: ComponentMetricId) -> float | None: """Get metric value. @@ -57,7 +33,7 @@ def get(self, metric: ComponentMetricId) -> float | None: Returns: Value of the metric. """ - return self._metrics.get(metric, None) + return self.metrics.get(metric, None) def __eq__(self, other: object) -> bool: """Compare two objects of this class. @@ -74,6 +50,4 @@ def __eq__(self, other: object) -> bool: if not isinstance(other, ComponentMetricsData): return False - return ( - self.component_id == other.component_id and self._metrics == other._metrics - ) + return self.component_id == other.component_id and self.metrics == other.metrics From 1dd4cb22c8a901b27142fdd19abdab9df44fc1db Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 14 Feb 2025 15:20:24 +0100 Subject: [PATCH 2/8] Move microgrid actors tests to tests/microgrid Signed-off-by: Leandro Lucarella --- tests/{actor => microgrid}/power_distributing/__init__.py | 0 .../power_distributing/_component_status/__init__.py | 0 .../_component_status/test_battery_pool_status.py | 0 .../power_distributing/_component_status/test_battery_status.py | 0 .../_component_status/test_ev_charger_status.py | 0 .../_component_status/test_pv_inverter_status.py | 0 .../power_distributing/test_battery_distribution_algorithm.py | 0 .../power_distributing/test_power_distributing.py | 0 tests/{actor => microgrid}/test_data_sourcing.py | 0 9 files changed, 0 insertions(+), 0 deletions(-) rename tests/{actor => microgrid}/power_distributing/__init__.py (100%) rename tests/{actor => microgrid}/power_distributing/_component_status/__init__.py (100%) rename tests/{actor => microgrid}/power_distributing/_component_status/test_battery_pool_status.py (100%) rename tests/{actor => microgrid}/power_distributing/_component_status/test_battery_status.py (100%) rename tests/{actor => microgrid}/power_distributing/_component_status/test_ev_charger_status.py (100%) rename tests/{actor => microgrid}/power_distributing/_component_status/test_pv_inverter_status.py (100%) rename tests/{actor => microgrid}/power_distributing/test_battery_distribution_algorithm.py (100%) rename tests/{actor => microgrid}/power_distributing/test_power_distributing.py (100%) rename tests/{actor => microgrid}/test_data_sourcing.py (100%) diff --git a/tests/actor/power_distributing/__init__.py b/tests/microgrid/power_distributing/__init__.py similarity index 100% rename from tests/actor/power_distributing/__init__.py rename to tests/microgrid/power_distributing/__init__.py diff --git a/tests/actor/power_distributing/_component_status/__init__.py b/tests/microgrid/power_distributing/_component_status/__init__.py similarity index 100% rename from tests/actor/power_distributing/_component_status/__init__.py rename to tests/microgrid/power_distributing/_component_status/__init__.py diff --git a/tests/actor/power_distributing/_component_status/test_battery_pool_status.py b/tests/microgrid/power_distributing/_component_status/test_battery_pool_status.py similarity index 100% rename from tests/actor/power_distributing/_component_status/test_battery_pool_status.py rename to tests/microgrid/power_distributing/_component_status/test_battery_pool_status.py diff --git a/tests/actor/power_distributing/_component_status/test_battery_status.py b/tests/microgrid/power_distributing/_component_status/test_battery_status.py similarity index 100% rename from tests/actor/power_distributing/_component_status/test_battery_status.py rename to tests/microgrid/power_distributing/_component_status/test_battery_status.py diff --git a/tests/actor/power_distributing/_component_status/test_ev_charger_status.py b/tests/microgrid/power_distributing/_component_status/test_ev_charger_status.py similarity index 100% rename from tests/actor/power_distributing/_component_status/test_ev_charger_status.py rename to tests/microgrid/power_distributing/_component_status/test_ev_charger_status.py diff --git a/tests/actor/power_distributing/_component_status/test_pv_inverter_status.py b/tests/microgrid/power_distributing/_component_status/test_pv_inverter_status.py similarity index 100% rename from tests/actor/power_distributing/_component_status/test_pv_inverter_status.py rename to tests/microgrid/power_distributing/_component_status/test_pv_inverter_status.py diff --git a/tests/actor/power_distributing/test_battery_distribution_algorithm.py b/tests/microgrid/power_distributing/test_battery_distribution_algorithm.py similarity index 100% rename from tests/actor/power_distributing/test_battery_distribution_algorithm.py rename to tests/microgrid/power_distributing/test_battery_distribution_algorithm.py diff --git a/tests/actor/power_distributing/test_power_distributing.py b/tests/microgrid/power_distributing/test_power_distributing.py similarity index 100% rename from tests/actor/power_distributing/test_power_distributing.py rename to tests/microgrid/power_distributing/test_power_distributing.py diff --git a/tests/actor/test_data_sourcing.py b/tests/microgrid/test_data_sourcing.py similarity index 100% rename from tests/actor/test_data_sourcing.py rename to tests/microgrid/test_data_sourcing.py From 495bddc26b078b6462436ddaab6db2014628923b Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 30 Jan 2025 10:52:57 +0100 Subject: [PATCH 3/8] Improve component graph's documentation Signed-off-by: Leandro Lucarella --- src/frequenz/sdk/microgrid/component_graph.py | 234 +++++++++--------- 1 file changed, 111 insertions(+), 123 deletions(-) diff --git a/src/frequenz/sdk/microgrid/component_graph.py b/src/frequenz/sdk/microgrid/component_graph.py index 5c01304bf..1b0be619e 100644 --- a/src/frequenz/sdk/microgrid/component_graph.py +++ b/src/frequenz/sdk/microgrid/component_graph.py @@ -5,18 +5,18 @@ The component graph is an approximate representation of the microgrid circuit, abstracted to a level appropriate for higher-level monitoring and control. -Examples of use-cases would be: +Common use cases include: - * using the graph structure to infer which component measurements - need to be combined to obtain grid power or onsite load +* Combining component measurements to compute grid power or onsite load by using + the graph structure to determine which measurements to aggregate - * identifying which inverter(s) need to be engaged to (dis)charge - a particular battery +* Identifying which inverter(s) need to be engaged to charge or discharge + a particular battery based on their connectivity in the graph - * understanding which power flows in the microgrid are derived from - green and grey sources +* Understanding which power flows in the microgrid are derived from green vs + grey sources based on the component connectivity -It deliberately does not include all pieces of hardware placed in the microgrid, +The graph deliberately does not include all pieces of hardware placed in the microgrid, instead limiting itself to just those that are needed to monitor and control the flow of power. """ @@ -40,7 +40,7 @@ # pylint: disable=too-many-lines -# Constant to store the actual obejcts as data attached to the graph nodes and edges +# Constant to store the actual objects as data attached to the graph nodes and edges _DATA_KEY = "data" @@ -60,12 +60,11 @@ def components( """Fetch the components of the microgrid. Args: - component_ids: filter out any components not matching one of the provided IDs - component_categories: filter out any components not matching one of the - provided types + component_ids: The component IDs that the components must match. + component_categories: The component categories that the components must match. Returns: - Set of the components currently connected to the microgrid, filtered by + The set of components currently connected to the microgrid, filtered by the provided `component_ids` and `component_categories` values. """ @@ -78,13 +77,11 @@ def connections( """Fetch the connections between microgrid components. Args: - start: filter out any connections whose `start` does not match one of these - component IDs - end: filter out any connections whose `end` does not match one of these - component IDs + start: The component IDs that the connections' start must match. + end: The component IDs that the connections' end must match. Returns: - Set of the connections between components in the microgrid, filtered by + The set of connections between components in the microgrid, filtered by the provided `start`/`end` choices. """ @@ -93,16 +90,16 @@ def predecessors(self, component_id: int) -> set[Component]: """Fetch the graph predecessors of the specified component. Args: - component_id: numerical ID of the component whose predecessors should be - fetched + component_id: The IDs of the components whose predecessors should be + fetched. Returns: - Set of IDs of the components that are predecessors of `component_id`, - i.e. for which there is a connection from each of these components to + The set of components that are predecessors of `component_id`, i.e. for + which there is a connection from each of these components to `component_id`. Raises: - KeyError: if the specified `component_id` is not in the graph + KeyError: If the specified `component_id` is not in the graph. """ @abstractmethod @@ -110,16 +107,15 @@ def successors(self, component_id: int) -> set[Component]: """Fetch the graph successors of the specified component. Args: - component_id: numerical ID of the component whose successors should be - fetched + component_id: The IDs of the components whose successors should be fetched. Returns: - Set of IDs of the components that are successors of `component_id`, - i.e. for which there is a connection from `component_id` to each of - these components. + The set of components that are successors of `component_id`, i.e. for + which there is a connection from `component_id` to each of these + components. Raises: - KeyError: if the specified `component_id` is not in the graph + KeyError: If the specified `component_id` is not in the graph """ @abstractmethod @@ -130,7 +126,7 @@ def is_grid_meter(self, component: Component) -> bool: component. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a grid meter. @@ -141,7 +137,7 @@ def is_pv_inverter(self, component: Component) -> bool: """Check if the specified component is a PV inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a PV inverter. @@ -155,7 +151,7 @@ def is_pv_meter(self, component: Component) -> bool: successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a PV meter. @@ -168,7 +164,7 @@ def is_pv_chain(self, component: Component) -> bool: A component is part of a PV chain if it is a PV meter or a PV inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a PV chain. @@ -179,7 +175,7 @@ def is_battery_inverter(self, component: Component) -> bool: """Check if the specified component is a battery inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a battery inverter. @@ -193,7 +189,7 @@ def is_battery_meter(self, component: Component) -> bool: predecessors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a battery meter. @@ -207,7 +203,7 @@ def is_battery_chain(self, component: Component) -> bool: inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a battery chain. @@ -218,7 +214,7 @@ def is_ev_charger(self, component: Component) -> bool: """Check if the specified component is an EV charger. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is an EV charger. @@ -232,7 +228,7 @@ def is_ev_charger_meter(self, component: Component) -> bool: successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is an EV charger meter. @@ -246,7 +242,7 @@ def is_ev_charger_chain(self, component: Component) -> bool: EV charger. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of an EV charger chain. @@ -257,7 +253,7 @@ def is_chp(self, component: Component) -> bool: """Check if the specified component is a CHP. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a CHP. @@ -270,7 +266,7 @@ def is_chp_meter(self, component: Component) -> bool: This is done by checking if the component has only CHPs as its successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a CHP meter. @@ -283,7 +279,7 @@ def is_chp_chain(self, component: Component) -> bool: A component is part of a CHP chain if it is a CHP meter or a CHP. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a CHP chain. @@ -296,8 +292,7 @@ def dfs( visited: set[Component], condition: Callable[[Component], bool], ) -> set[Component]: - """ - Search for components that fulfill the condition in the Graph. + """Search for components that fulfill the condition in the Graph. DFS is used for searching the graph. The graph traversal is stopped once a component fulfills the condition. @@ -308,8 +303,8 @@ def dfs( condition: The condition function to check for. Returns: - A set of component ids where the corresponding components fulfill - the condition function. + A set of component IDs where the corresponding components fulfill + the `condition` function. """ @abstractmethod @@ -333,13 +328,13 @@ def find_first_descendant_component( highest priority. Args: - root_category: The category of the root component to search for. - descendant_categories: The descendant categories to search for the - first descendant component in. + root_category: The class of the root component to search for. + descendant_categories: The descendant classes to search for the first + descendant component in. Returns: The first descendant component found in the component graph, - considering the specified root and descendant categories. + considering the specified `root` and `descendants` categories. """ @@ -359,16 +354,14 @@ def __init__( """Initialize the component graph. Args: - components: components with which to first initialize the graph, - provided as pairs of the form `(component_id, - component_category)`; if set, must provide `connections` as well - connections: connections with which to initialize the graph, - provided as pairs of component IDs describing the start and end - of the connection; if set, must provide `components` as well + components: The components to initialize the graph with. If set, must + provide `connections` as well. + connections: The connections to initialize the graph with. If set, must + provide `components` as well. Raises: - InvalidGraphError: if `components` and `connections` are not both `None` - and either of them is either `None` or empty + InvalidGraphError: If `components` and `connections` are not both `None` + and either of them is either `None` or empty. """ self._graph: nx.DiGraph = nx.DiGraph() @@ -392,12 +385,11 @@ def components( """Fetch the components of the microgrid. Args: - component_ids: filter out any components not matching one of the provided IDs - component_categories: filter out any components not matching one of the - provided types + component_ids: The component IDs that the components must match. + component_categories: The component categories that the components must match. Returns: - Set of the components currently connected to the microgrid, filtered by + The set of components currently connected to the microgrid, filtered by the provided `component_ids` and `component_categories` values. """ selection_ids = ( @@ -422,13 +414,11 @@ def connections( """Fetch the connections between microgrid components. Args: - start: filter out any connections whose `start` does not match one of these - component IDs - end: filter out any connections whose `end` does not match one of these - component IDs + start: The component IDs that the connections' start must match. + end: The component IDs that the connections' end must match. Returns: - Set of the connections between components in the microgrid, filtered by + The set of connections between components in the microgrid, filtered by the provided `start`/`end` choices. """ match (start, end): @@ -449,16 +439,16 @@ def predecessors(self, component_id: int) -> set[Component]: """Fetch the graph predecessors of the specified component. Args: - component_id: numerical ID of the component whose predecessors should be - fetched + component_id: The IDs of the components whose predecessors should be + fetched. Returns: - Set of IDs of the components that are predecessors of `component_id`, - i.e. for which there is a connection from each of these components to + The set of components that are predecessors of `component_id`, i.e. for + which there is a connection from each of these components to `component_id`. Raises: - KeyError: if the specified `component_id` is not in the graph + KeyError: If the specified `component_id` is not in the graph. """ if component_id not in self._graph: raise KeyError( @@ -473,16 +463,15 @@ def successors(self, component_id: int) -> set[Component]: """Fetch the graph successors of the specified component. Args: - component_id: numerical ID of the component whose successors should be - fetched + component_id: The IDs of the components whose successors should be fetched. Returns: - Set of IDs of the components that are successors of `component_id`, - i.e. for which there is a connection from `component_id` to each of - these components. + The set of components that are successors of `component_id`, i.e. for + which there is a connection from `component_id` to each of these + components. Raises: - KeyError: if the specified `component_id` is not in the graph + KeyError: If the specified `component_id` is not in the graph """ if component_id not in self._graph: raise KeyError( @@ -505,16 +494,14 @@ def refresh_from( components and connections. Args: - components: components to include in the graph, provided as pairs of - the form `(component_id, component_category)` - connections: connections to include in the graph, provided as pairs - of component IDs describing the start and end of the connection - correct_errors: callback that, if set, will be invoked if the + components: The components to include in the graph. + connections: The connections to include in the graph. + correct_errors: The callback that, if set, will be invoked if the provided graph data is in any way invalid (it will attempt to - correct the errors by inferring what the correct data should be) + correct the errors by inferring what the correct data should be). Raises: - InvalidGraphError: if the provided `components` and `connections` + InvalidGraphError: If the provided `components` and `connections` do not form a valid component graph and `correct_errors` does not fix it. """ @@ -565,10 +552,10 @@ async def refresh_from_api( """Refresh the contents of a component graph from the remote API. Args: - api: API client from which to fetch graph data - correct_errors: callback that, if set, will be invoked if the + api: The API client from which to fetch graph data. + correct_errors: The callback that, if set, will be invoked if the provided graph data is in any way invalid (it will attempt to - correct the errors by inferring what the correct data should be) + correct the errors by inferring what the correct data should be). """ components, connections = await asyncio.gather( api.components(), @@ -592,7 +579,7 @@ def is_grid_meter(self, component: Component) -> bool: component. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a grid meter. @@ -615,7 +602,7 @@ def is_pv_inverter(self, component: Component) -> bool: """Check if the specified component is a PV inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a PV inverter. @@ -632,7 +619,7 @@ def is_pv_meter(self, component: Component) -> bool: successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a PV meter. @@ -655,7 +642,7 @@ def is_pv_chain(self, component: Component) -> bool: meter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a PV chain. @@ -666,7 +653,7 @@ def is_ev_charger(self, component: Component) -> bool: """Check if the specified component is an EV charger. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is an EV charger. @@ -680,7 +667,7 @@ def is_ev_charger_meter(self, component: Component) -> bool: successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is an EV charger meter. @@ -700,7 +687,7 @@ def is_ev_charger_chain(self, component: Component) -> bool: EV charger meter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of an EV charger chain. @@ -711,7 +698,7 @@ def is_battery_inverter(self, component: Component) -> bool: """Check if the specified component is a battery inverter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a battery inverter. @@ -728,7 +715,7 @@ def is_battery_meter(self, component: Component) -> bool: its successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a battery meter. @@ -748,7 +735,7 @@ def is_battery_chain(self, component: Component) -> bool: battery meter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a battery chain. @@ -759,7 +746,7 @@ def is_chp(self, component: Component) -> bool: """Check if the specified component is a CHP. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a CHP. @@ -773,7 +760,7 @@ def is_chp_meter(self, component: Component) -> bool: successors. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is a CHP meter. @@ -792,7 +779,7 @@ def is_chp_chain(self, component: Component) -> bool: A component is part of a CHP chain if it is either a CHP or a CHP meter. Args: - component: component to check. + component: The component to check. Returns: Whether the specified component is part of a CHP chain. @@ -805,8 +792,7 @@ def dfs( visited: set[Component], condition: Callable[[Component], bool], ) -> set[Component]: - """ - Search for components that fulfill the condition in the Graph. + """Search for components that fulfill the condition in the Graph. DFS is used for searching the graph. The graph traversal is stopped once a component fulfills the condition. @@ -817,7 +803,7 @@ def dfs( condition: The condition function to check for. Returns: - A set of component ids where the corresponding components fulfill + A set of component IDs where the corresponding components fulfill the condition function. """ if current_node in visited: @@ -855,17 +841,17 @@ def find_first_descendant_component( highest priority. Args: - root_category: The category of the root component to search for. - descendant_categories: The descendant categories to search for the - first descendant component in. - - Raises: - ValueError: when the root component is not found in the component - graph or when no component is found in the given categories. + root_category: The class of the root component to search for. + descendant_categories: The descendant classes to search for the first + descendant component in. Returns: The first descendant component found in the component graph, - considering the specified root and descendant categories. + considering the specified `root` and `descendants` categories. + + Raises: + ValueError: When the root component is not found in the component + graph or when no component is found in the given categories. """ root_component = next( (comp for comp in self.components(component_categories={root_category})), @@ -900,8 +886,11 @@ def _validate_graph(self) -> None: """Check that the underlying graph data is valid. Raises: - InvalidGraphError: if there are no components, or no connections, or - the graph is not a tree, or if any component lacks type data + InvalidGraphError: If: + - There are no components. + - There are no connections. + - The graph is not a tree. + - Any node lacks its associated component data. """ if self._graph.number_of_nodes() == 0: raise InvalidGraphError("No components in graph!") @@ -943,9 +932,8 @@ def _validate_graph_root(self) -> None: """Check that there is exactly one node without predecessors, of valid type. Raises: - InvalidGraphError: if there is more than one node without predecessors, - or if there is a single such node that is not one of NONE, GRID, or - JUNCTION + InvalidGraphError: If there is more than one node without predecessors, + or if there is a single such node that is not one of NONE or GRID. """ no_predecessors = filter( lambda c: self._graph.in_degree(c.component_id) == 0, @@ -975,11 +963,11 @@ def _validate_grid_endpoint(self) -> None: """Check that the grid endpoint is configured correctly in the graph. Raises: - InvalidGraphError: if there is more than one grid endpoint in the + InvalidGraphError: If there is more than one grid endpoint in the graph, or if the grid endpoint has predecessors (if it exists, then it should be the root of the component-graph tree), or if it has no successors in the graph (i.e. it is not connected to - anything) + anything). """ grid = list(self.components(component_categories={ComponentCategory.GRID})) @@ -1010,8 +998,8 @@ def _validate_intermediary_components(self) -> None: successors in the component graph, such as METER, or INVERTER. Raises: - InvalidGraphError: if any intermediary component has zero predecessors - or zero successors + InvalidGraphError: If any intermediary component has zero predecessors + or zero successors. """ intermediary_components = list( self.components(component_categories={ComponentCategory.INVERTER}) @@ -1037,8 +1025,8 @@ def _validate_leaf_components(self) -> None: connections and no outgoing connections. Raises: - InvalidGraphError: if any leaf component in the graph has 0 predecessors, - or has > 0 successors + InvalidGraphError: If any leaf component in the graph has 0 predecessors, + or has > 0 successors. """ leaf_components = list( self.components( From 1f06955675e3071cce2fcf8e226982eba8384ff4 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 20 Feb 2025 11:58:14 +0100 Subject: [PATCH 4/8] Use `str()` instead of the private `_name` Signed-off-by: Leandro Lucarella --- .../sdk/timeseries/formula_engine/_formula_formatter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py b/src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py index 8a246fba5..f5637b5c6 100644 --- a/src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py +++ b/src/frequenz/sdk/timeseries/formula_engine/_formula_formatter.py @@ -215,7 +215,7 @@ def format(self, postfix_expr: list[FormulaStep]) -> str: self._stack.append(StackItem(value, OperatorPrecedence.PRIMARY, 1)) case MetricFetcher(): metric_fetcher = step - value = metric_fetcher._name # pylint: disable=protected-access + value = str(metric_fetcher) if engine_reference := getattr( metric_fetcher.stream, "_engine_reference", None ): From c6f098633012e2fe8d68c3c0caa762092a30f66a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 6 Mar 2025 13:07:58 +0100 Subject: [PATCH 5/8] Improve `test_producer` output Change the name from "test case" to "test step" in the fallback test, as they are not really isolated test cases, but steps in one long test (as next steps depend on the previous step to have happened). This makes it more clear that steps are connected and need to run in sequence. Also the index was made part of the test steps definition, so we can show more clearly which (sub-)stop failed, and we also add a print line indicating which step is being ran. Signed-off-by: Leandro Lucarella --- tests/timeseries/test_producer.py | 76 ++++++++++++++++++------------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/tests/timeseries/test_producer.py b/tests/timeseries/test_producer.py index 75028ec3e..747082e2a 100644 --- a/tests/timeseries/test_producer.py +++ b/tests/timeseries/test_producer.py @@ -112,55 +112,67 @@ async def test_producer_fallback_formula(self, mocker: MockerFixture) -> None: # * if the meter value is None, it should be treated as None. # * for other components None is treated as 0. - # fmt: off expected_input_output: list[ - tuple[list[float | None], list[float | None], list[float | None], Power | None] - ] = [ - # ([pv_meter_power], [pv_inverter_power], [chp_power], expected_power) + tuple[ + float, + list[float | None], + list[float | None], + list[float | None], + Power | None, + ] + ] + # fmt: off + expected_input_output = [ + # (test number, [pv_meter_power], [pv_inverter_power], [chp_power], expected_power) + # Step 1: All components are available # Add power from meters and chp - ([-1.0, -2.0], [None, -200.0], [300], Power.from_watts(297.0)), - ([-1.0, -10], [-100.0, -200.0], [400], Power.from_watts(389.0)), - # Case 2: The first meter is unavailable (None). + (1.1, [-1.0, -2.0], [None, -200.0], [300], Power.from_watts(297.0)), + (1.2, [-1.0, -10], [-100.0, -200.0], [400], Power.from_watts(389.0)), + # Step 2: The first meter is unavailable (None). # Subscribe to the fallback inverter, but return None as the result, # according to the "nones-are-zero" rule - ([None, -2.0], [-100, -200.0], [400], None), - # Case 3: First meter is unavailable (None). Fallback inverter provides + (2.1, [None, -2.0], [-100, -200.0], [400], None), + # Step 3: First meter is unavailable (None). Fallback inverter provides # a value. # Add second meter, first inverter and chp power - ([None, -2.0], [-100, -200.0], [400], Power.from_watts(298.0)), - ([None, -2.0], [-50, -200.0], [300], Power.from_watts(248.0)), - # Case 4: Both first meter and its fallback inverter are unavailable + (3.1, [None, -2.0], [-100, -200.0], [400], Power.from_watts(298.0)), + (3.2, [None, -2.0], [-50, -200.0], [300], Power.from_watts(248.0)), + # Step 4: Both first meter and its fallback inverter are unavailable # (None). Return 0 from failing component according to the # "nones-are-zero" rule. - ([None, -2.0], [None, -200.0], [300], Power.from_watts(298.0)), - ([None, -10.0], [-20.0, -200.0], [300], Power.from_watts(270.0)), - # Case 5: CHP is unavailable. Return 0 from failing component + (4.1, [None, -2.0], [None, -200.0], [300], Power.from_watts(298.0)), + (4.2, [None, -10.0], [-20.0, -200.0], [300], Power.from_watts(270.0)), + # Step 5: CHP is unavailable. Return 0 from failing component # according to the "nones-are-zero" rule. - ([None, -10.0], [-20.0, -200.0], [None], Power.from_watts(-30.0)), - # Case 6: Both meters are unavailable (None). Subscribe for fallback inverter - ([None, None], [-20.0, -200.0], [None], None), - ([None, None], [-20.0, -200.0], [None], Power.from_watts(-220.0)), - ([None, None], [None, -200.0], [None], Power.from_watts(-200.0)), - # Case 7: All components are unavailable (None). Return 0 according to the + (5.1, [None, -10.0], [-20.0, -200.0], [None], Power.from_watts(-30.0)), + # Step 6: Both meters are unavailable (None). Subscribe for fallback inverter + (6.1, [None, None], [-20.0, -200.0], [None], None), + (6.2, [None, None], [-20.0, -200.0], [None], Power.from_watts(-220.0)), + (6.3, [None, None], [None, -200.0], [None], Power.from_watts(-200.0)), + # Step 7: All components are unavailable (None). Return 0 according to the # "nones-are-zero" rule. - ([None, None], [None, None], [None], Power.from_watts(0)), - ([None, None], [None, None], [None], Power.from_watts(0)), - ([None, None], [None, None], [300.0], Power.from_watts(300.0)), - ([-200.0, None], [None, -100.0], [50.0], Power.from_watts(-250.0)), - ([-200.0, -200.0], [-10.0, -20.0], [50.0], Power.from_watts(-350.0)), - # Case 8: Meter is unavailable, start fallback formula. - ([None, -200.0], [-10.0, -100.0], [50.0], None), - ([None, -200.0], [-10.0, -100.0], [50.0], Power.from_watts(-160)), + (7.1, [None, None], [None, None], [None], Power.from_watts(0)), + (7.2, [None, None], [None, None], [None], Power.from_watts(0)), + (7.3, [None, None], [None, None], [300.0], Power.from_watts(300.0)), + (7.4, [-200.0, None], [None, -100.0], [50.0], Power.from_watts(-250.0)), + (7.5, [-200.0, -200.0], [-10.0, -20.0], [50.0], Power.from_watts(-350.0)), + # Step 8: Meter is unavailable, start fallback formula. + (8.1, [None, -200.0], [-10.0, -100.0], [50.0], None), + (8.2, [None, -200.0], [-10.0, -100.0], [50.0], Power.from_watts(-160)), ] # fmt: on - for idx, ( + for ( + idx, meter_power, pv_inverter_power, chp_power, expected_power, - ) in enumerate(expected_input_output): + ) in expected_input_output: + print("----------------------------------------------------") + print(f" Test step {idx}") + print("----------------------------------------------------") await mockgrid.mock_resampler.send_chp_power(chp_power) await mockgrid.mock_resampler.send_meter_power(meter_power) await mockgrid.mock_resampler.send_pv_inverter_power(pv_inverter_power) @@ -168,7 +180,7 @@ async def test_producer_fallback_formula(self, mocker: MockerFixture) -> None: result = await producer_power_receiver.receive() assert result.value == expected_power, ( - f"Test case {idx} failed:" + f"Test step {idx} failed:" + f" meter_power: {meter_power}" + f" pv_inverter_power {pv_inverter_power}" + f" chp_power {chp_power}" From ae4159a5017a47e99088172f36df342cd9338af0 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 7 Mar 2025 15:02:21 +0100 Subject: [PATCH 6/8] Improve BatteryManager error reporting A `KeyError` was raised to indicate errors while getting inverters for batteries, but this have a couple of issues. First, the `KeyError` was converted to a string to get an error message, but `KeyError` is a very special error, and the argument is interpreted as a *key*, not as an error message, so when converting the error to a `str`, the result is the `repr()` of the *key*, so the strings are quoted and escaped, which is not what we want to report errors downstream. Second, one of the reported errors isn't really a `KeyError`, it is just a mismatch between what is requested and the current microgrid topology, so using a `KeyError` was completely wrong. Since these errors were only used to report errors from a direct call, we change `_get_components_data()` to return `list[InvBatPair] | str`, where if there is an error, a `str` with the error description is returned directly. As an extra, we sort the IDs when used in error messages, so they are easier to read for users, and more deterministic for tests. Tests are also simplified to remove the use of regex, which were probably only used because string were quoted. Signed-off-by: Leandro Lucarella --- .../_component_managers/_battery_manager.py | 42 ++++++++++--------- .../test_power_distributing.py | 15 +++---- 2 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py b/src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py index bb75b9bf1..330c64340 100644 --- a/src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py +++ b/src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py @@ -224,17 +224,18 @@ async def _get_distribution(self, request: Request) -> DistributionResult | Resu Returns: Distribution of the batteries. """ - try: - pairs_data: list[InvBatPair] = self._get_components_data( - request.component_ids - ) - except KeyError as err: - return Error(request=request, msg=str(err)) + match self._get_components_data(request.component_ids): + case str() as err: + return Error(request=request, msg=err) + case list() as pairs_data: + pass + case unexpected: + typing.assert_never(unexpected) if not pairs_data: error_msg = ( - "No data for at least one of the given " - f"batteries {str(request.component_ids)}" + "No data for at least one of the given batteries: " + + self._str_ids(request.component_ids) ) return Error(request=request, msg=str(error_msg)) @@ -510,18 +511,17 @@ def nan_metric_in_list(data: list[DataType], metrics: list[str]) -> bool: def _get_components_data( self, batteries: collections.abc.Set[int] - ) -> list[InvBatPair]: + ) -> list[InvBatPair] | str: """Get data for the given batteries and adjacent inverters. Args: batteries: Batteries that needs data. - Raises: - KeyError: If any battery in the given list doesn't exists in microgrid. - Returns: - Pairs of battery and adjacent inverter data. + Pairs of battery and adjacent inverter data or an error message if there was + an error while getting the data. """ + inverter_ids: collections.abc.Set[int] pairs_data: list[InvBatPair] = [] working_batteries = self._component_pool_status_tracker.get_working_components( @@ -530,9 +530,9 @@ def _get_components_data( for battery_id in working_batteries: if battery_id not in self._battery_caches: - raise KeyError( + return ( f"No battery {battery_id}, " - f"available batteries: {list(self._battery_caches.keys())}" + f"available batteries: {self._str_ids(self._battery_caches.keys())}" ) connected_inverters = _get_all_from_map(self._bat_invs_map, batteries) @@ -545,9 +545,10 @@ def _get_components_data( if batteries_from_inverters != batteries: extra_batteries = batteries_from_inverters - batteries - raise KeyError( - f"Inverters {_get_all_from_map(self._bat_invs_map, extra_batteries)} " - f"are connected to batteries that were not requested: {extra_batteries}" + inverter_ids = _get_all_from_map(self._bat_invs_map, extra_batteries) + return ( + f"Inverter(s) ({self._str_ids(inverter_ids)}) are connected to " + f"battery(ies) ({self._str_ids(extra_batteries)}) that were not requested" ) # set of set of batteries one for each working_battery @@ -556,7 +557,7 @@ def _get_components_data( ) for battery_ids in battery_sets: - inverter_ids: frozenset[int] = self._bat_invs_map[next(iter(battery_ids))] + inverter_ids = self._bat_invs_map[next(iter(battery_ids))] data = self._get_battery_inverter_data(battery_ids, inverter_ids) if data is None: @@ -570,6 +571,9 @@ def _get_components_data( pairs_data.append(data) return pairs_data + def _str_ids(self, ids: collections.abc.Set[int]) -> str: + return ", ".join(str(cid) for cid in sorted(ids)) + def _get_power_distribution( self, request: Request, inv_bat_pairs: list[InvBatPair] ) -> DistributionResult: diff --git a/tests/microgrid/power_distributing/test_power_distributing.py b/tests/microgrid/power_distributing/test_power_distributing.py index cf357d2dc..f0e7d1fd4 100644 --- a/tests/microgrid/power_distributing/test_power_distributing.py +++ b/tests/microgrid/power_distributing/test_power_distributing.py @@ -7,7 +7,6 @@ import asyncio import math -import re from collections import abc from typing import TypeVar from unittest.mock import MagicMock @@ -474,7 +473,7 @@ async def test_two_batteries_one_broken_one_inverters( assert result.request == request assert ( result.msg - == "No data for at least one of the given batteries {9, 19}" + == "No data for at least one of the given batteries: 9, 19" ) async def test_battery_two_inverters(self, mocker: MockerFixture) -> None: @@ -821,13 +820,10 @@ async def test_connected_but_not_requested_batteries( result: Result = done.pop().result() assert isinstance(result, Error) assert result.request == request - - err_msg = re.search( - r"'Inverters \{48\} are connected to batteries that were not " - r"requested: \{19\}'", - result.msg, + assert ( + result.msg + == "Inverter(s) (48) are connected to battery(ies) (19) that were not requested" ) - assert err_msg is not None async def test_battery_soc_nan(self, mocker: MockerFixture) -> None: """Test if battery with SoC==NaN is not used.""" @@ -1054,8 +1050,7 @@ async def test_power_distributor_invalid_battery_id( result: Result = done.pop().result() assert isinstance(result, Error) assert result.request == request - err_msg = re.search(r"No battery 100, available batteries:", result.msg) - assert err_msg is not None + assert result.msg == "No battery 100, available batteries: 9, 19, 29" async def test_power_distributor_one_user_adjust_power_consume( self, mocker: MockerFixture From f38b2f8af48eed05c5e36e5859756916b5928e7c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 7 Mar 2025 15:09:38 +0100 Subject: [PATCH 7/8] Simplify code using a safety timeout Use `async with asyncio.timeout()` instead of using `asyncio.wait()` with extra tasks. Signed-off-by: Leandro Lucarella --- .../test_power_distributing.py | 188 ++++-------------- 1 file changed, 39 insertions(+), 149 deletions(-) diff --git a/tests/microgrid/power_distributing/test_power_distributing.py b/tests/microgrid/power_distributing/test_power_distributing.py index f0e7d1fd4..1b8ed96c6 100644 --- a/tests/microgrid/power_distributing/test_power_distributing.py +++ b/tests/microgrid/power_distributing/test_power_distributing.py @@ -215,15 +215,9 @@ async def test_power_distributor_one_user(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_power.isclose(Power.from_kilowatts(1.0)) assert result.excess_power.isclose(Power.from_watts(200.0)) @@ -284,15 +278,9 @@ async def test_power_distributor_exclusion_bounds( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - - result: Result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_power.isclose(Power.zero(), abs_tol=1e-9) assert result.excess_power.isclose(Power.zero(), abs_tol=1e-9) @@ -308,15 +296,9 @@ async def test_power_distributor_exclusion_bounds( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result = done.pop().result() assert isinstance( result, OutOfBounds ), f"Expected OutOfBounds, got {result}" @@ -380,15 +362,9 @@ async def test_two_batteries_one_inverters(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Success) # Inverter bounded at 500 assert result.succeeded_power.isclose(Power.from_watts(500.0)) @@ -459,15 +435,8 @@ async def test_two_batteries_one_broken_one_inverters( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 - - result: Result = done.pop().result() + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() assert isinstance(result, Error) assert result.request == request @@ -516,15 +485,9 @@ async def test_battery_two_inverters(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Success) # Inverters each bounded at 500, together 1000 assert result.succeeded_power.isclose(Power.from_watts(1000.0)) @@ -569,15 +532,9 @@ async def test_two_batteries_three_inverters(self, mocker: MockerFixture) -> Non await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Success) # each inverter is bounded at 500 and we have 3 inverters assert result.succeeded_power.isclose(Power.from_watts(1500.0)) @@ -656,15 +613,9 @@ async def test_two_batteries_one_inverter_different_exclusion_bounds_2( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, OutOfBounds) assert result.request == request assert result.bounds == PowerBounds(-1000, -500, 500, 1000) @@ -744,15 +695,9 @@ async def test_two_batteries_one_inverter_different_exclusion_bounds( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, OutOfBounds) assert result.request == request # each inverter is bounded at 500 @@ -809,15 +754,9 @@ async def test_connected_but_not_requested_batteries( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Error) assert result.request == request assert ( @@ -865,15 +804,9 @@ async def test_battery_soc_nan(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - - result: Result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_components == {19} assert result.succeeded_power.isclose(Power.from_watts(500.0)) @@ -921,15 +854,9 @@ async def test_battery_capacity_nan(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - - result: Result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_components == {19} assert result.succeeded_power.isclose(Power.from_watts(500.0)) @@ -996,15 +923,9 @@ async def test_battery_power_bounds_nan(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result: Result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_components == {19} assert result.succeeded_power.isclose(Power.from_kilowatts(1.0)) @@ -1041,13 +962,9 @@ async def test_power_distributor_invalid_battery_id( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, _ = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(done) == 1 - result: Result = done.pop().result() assert isinstance(result, Error) assert result.request == request assert result.msg == "No battery 100, available batteries: 9, 19, 29" @@ -1086,15 +1003,9 @@ async def test_power_distributor_one_user_adjust_power_consume( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - - assert len(pending) == 0 - assert len(done) == 1 + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - result = done.pop().result() assert isinstance(result, OutOfBounds) assert result is not None assert result.request == request @@ -1134,15 +1045,9 @@ async def test_power_distributor_one_user_adjust_power_supply( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - - result = done.pop().result() assert isinstance(result, OutOfBounds) assert result is not None assert result.request == request @@ -1182,15 +1087,9 @@ async def test_power_distributor_one_user_adjust_power_success( await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - - result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_power.isclose(Power.from_kilowatts(1.0)) assert result.excess_power.isclose(Power.zero(), abs_tol=1e-9) @@ -1229,14 +1128,9 @@ async def test_not_all_batteries_are_working(self, mocker: MockerFixture) -> Non await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() - assert len(pending) == 0 - assert len(done) == 1 - result = done.pop().result() assert isinstance(result, Success) assert result.succeeded_components == {19} assert result.excess_power.isclose(Power.from_watts(700.0)) @@ -1284,13 +1178,9 @@ async def test_partial_failure_result(self, mocker: MockerFixture) -> None: await requests_channel.new_sender().send(request) result_rx = results_channel.new_receiver() - done, pending = await asyncio.wait( - [asyncio.create_task(result_rx.receive())], - timeout=SAFETY_TIMEOUT.total_seconds(), - ) - assert len(pending) == 0 - assert len(done) == 1 - result = done.pop().result() + async with asyncio.timeout(SAFETY_TIMEOUT.total_seconds()): + result = await result_rx.receive() + assert isinstance(result, PartialFailure) assert result.succeeded_components == batteries - failed_batteries assert result.failed_components == failed_batteries From 668969096d8c81d1d605958c7a6d6e99f116fd7a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 7 Mar 2025 15:09:59 +0100 Subject: [PATCH 8/8] Improve tests descriptions Signed-off-by: Leandro Lucarella --- .../power_distributing/test_battery_distribution_algorithm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/microgrid/power_distributing/test_battery_distribution_algorithm.py b/tests/microgrid/power_distributing/test_battery_distribution_algorithm.py index 4397fc050..3ea42ff75 100644 --- a/tests/microgrid/power_distributing/test_battery_distribution_algorithm.py +++ b/tests/microgrid/power_distributing/test_battery_distribution_algorithm.py @@ -562,7 +562,7 @@ def test_supply_two_batteries_2(self) -> None: # Test consumption power distribution def test_consumption_three_batteries_1(self) -> None: - """Distribute consume power.""" + """Distribute consume power with three batteries with the same capacity.""" capacity: list[Metric] = [Metric(49000), Metric(49000), Metric(49000)] soc: list[Metric] = [ Metric(80.0, Bound(0, 100)), @@ -587,7 +587,7 @@ def test_consumption_three_batteries_1(self) -> None: assert result.remaining_power == approx(0.0) def test_consumption_three_batteries_2(self) -> None: - """Distribute consume power.""" + """Distribute consume power with three batteries but one with double the capacity.""" capacity: list[Metric] = [Metric(98000), Metric(49000), Metric(49000)] soc: list[Metric] = [ Metric(80.0, Bound(0, 100)),