Validate component IDs when creating component pools

RELEASE_NOTES.md

@@ -1,5 +1,17 @@
 # Frequenz Python SDK Release Notes
 
-## Bug fixes
+## Summary
 
-- `FormulaEngine` and `FormulaEngine3Phase` are now type aliases to `Formula` and `Formula3Phase`, fixing a typing issue introduced in `v1.0.0-rc2202`.
+<!-- Here goes a general summary of what this release is about -->
+
+## Upgrading
+
+<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
+
+## New Features
+
+<!-- Here goes the main new features and examples or instructions on how to use them -->
+
+## Bug Fixes
+
+- Component IDs are validated during creation of battery, pv and ev charger pools, so that errors are caught early and we don't end up getting cryptic failures from somewhere else.

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

@@ -81,12 +81,24 @@ def __init__(  # pylint: disable=too-many-arguments
             batteries_id: Subset of the batteries that should be included in the
                 battery pool. If None or empty, then all batteries from the microgrid
                 will be used.
+
+        Raises:
+            ValueError: If any of the specified batteries is not present in the
+                microgrid.
         """
         self._batteries: frozenset[ComponentId]
+        all_batteries = self._get_all_batteries()
         if batteries_id:
             self._batteries = frozenset(batteries_id)
+            if not self._batteries.issubset(all_batteries):
+                unknown_ids = self._batteries - all_batteries
+                raise ValueError(
+                    "Unable to create a BatteryPool. These component IDs are either "
+                    + "not batteries or are unknown: "
+                    + f"{unknown_ids}"
+                )
         else:
-            self._batteries = self._get_all_batteries()
+            self._batteries = all_batteries
 
         self._working_batteries: set[ComponentId] = set()
 

src/frequenz/sdk/timeseries/ev_charger_pool/_ev_charger_pool_reference_store.py

@@ -71,13 +71,21 @@ def __init__(  # pylint: disable=too-many-arguments
         self.power_manager_bounds_subs_sender = power_manager_bounds_subs_sender
         self.power_distribution_results_fetcher = power_distribution_results_fetcher
 
+        graph = connection_manager.get().component_graph
+        all_ev_chargers = frozenset(
+            {evc.id for evc in graph.components(matching_types=EvCharger)}
+        )
+
         if component_ids is not None:
             self.component_ids: frozenset[ComponentId] = frozenset(component_ids)
+            if not self.component_ids.issubset(all_ev_chargers):
+                unknown_ids = self.component_ids - all_ev_chargers
+                raise ValueError(
+                    "Unable to create an EVChargerPool. These component IDs are either "
+                    + f"not EV chargers or are unknown: {unknown_ids}"
+                )
         else:
-            graph = connection_manager.get().component_graph
-            self.component_ids = frozenset(
-                {evc.id for evc in graph.components(matching_types=EvCharger)}
-            )
+            self.component_ids = all_ev_chargers
 
         self.power_bounds_subs: dict[str, asyncio.Task[None]] = {}
 

src/frequenz/sdk/timeseries/pv_pool/_pv_pool_reference_store.py

@@ -72,13 +72,22 @@ def __init__(  # pylint: disable=too-many-arguments
         self.power_manager_bounds_subs_sender = power_manager_bounds_subs_sender
         self.power_distribution_results_fetcher = power_distribution_results_fetcher
 
+        graph = connection_manager.get().component_graph
+        all_solar_inverters = frozenset(
+            {inv.id for inv in graph.components(matching_types=SolarInverter)}
+        )
+
         if component_ids is not None:
             self.component_ids: frozenset[ComponentId] = frozenset(component_ids)
+            if not self.component_ids.issubset(all_solar_inverters):
+                unknown_ids = self.component_ids - all_solar_inverters
+                raise ValueError(
+                    "Unable to create a PVPool. These component IDs are either "
+                    + "not PV inverters or are unknown: "
+                    + f"{unknown_ids}"
+                )
         else:
-            graph = connection_manager.get().component_graph
-            self.component_ids = frozenset(
-                {inv.id for inv in graph.components(matching_types=SolarInverter)}
-            )
+            self.component_ids = all_solar_inverters
 
         self.power_bounds_subs: dict[str, asyncio.Task[None]] = {}
 

tests/microgrid/test_datapipeline.py

@@ -4,6 +4,7 @@
 """Basic tests for the DataPipeline."""
 
 import asyncio
+import re
 from datetime import timedelta
 
 import async_solipsism
@@ -12,13 +13,16 @@
 from frequenz.client.common.microgrid import MicrogridId
 from frequenz.client.common.microgrid.components import ComponentId
 from frequenz.client.microgrid.component import (
+    AcEvCharger,
     BatteryInverter,
     ComponentConnection,
     GridConnectionPoint,
     LiIonBattery,
+    SolarInverter,
 )
 from pytest_mock import MockerFixture
 
+from frequenz.sdk.microgrid import _data_pipeline
 from frequenz.sdk.microgrid._data_pipeline import _DataPipeline
 from frequenz.sdk.timeseries import ResamplerConfig2
 
@@ -68,16 +72,56 @@ async def test_actors_started(
     )
     bat_inverter_4 = BatteryInverter(id=ComponentId(4), microgrid_id=_MICROGRID_ID)
     battery_15 = LiIonBattery(id=ComponentId(15), microgrid_id=_MICROGRID_ID)
+    pv_inverter_7 = SolarInverter(id=ComponentId(7), microgrid_id=_MICROGRID_ID)
+    ev_charger_9 = AcEvCharger(id=ComponentId(9), microgrid_id=_MICROGRID_ID)
+
     mock_client = MockMicrogridClient(
-        components={grid_1, bat_inverter_4, battery_15},
+        components={grid_1, bat_inverter_4, battery_15, pv_inverter_7, ev_charger_9},
         connections={
             ComponentConnection(source=grid_1.id, destination=bat_inverter_4.id),
             ComponentConnection(source=bat_inverter_4.id, destination=battery_15.id),
+            ComponentConnection(source=grid_1.id, destination=pv_inverter_7.id),
+            ComponentConnection(source=grid_1.id, destination=ev_charger_9.id),
         },
     )
     mock_client.initialize(mocker)
 
+    _data_pipeline._DATA_PIPELINE = datapipeline
+
     datapipeline.new_battery_pool(priority=5)
+    datapipeline.new_pv_pool(priority=3)
+    datapipeline.new_ev_charger_pool(priority=4)
+
+    datapipeline.new_battery_pool(priority=1, component_ids={ComponentId(15)})
+    datapipeline.new_pv_pool(priority=2, component_ids={ComponentId(7)})
+    datapipeline.new_ev_charger_pool(priority=2, component_ids={ComponentId(9)})
+
+    with pytest.raises(
+        ValueError,
+        match=re.escape(
+            "Unable to create a BatteryPool. These component IDs are either not "
+            + "batteries or are unknown: frozenset({ComponentId(4)})"
+        ),
+    ):
+        datapipeline.new_battery_pool(priority=2, component_ids={ComponentId(4)})
+
+    with pytest.raises(
+        ValueError,
+        match=re.escape(
+            "Unable to create a PVPool. These component IDs are either not PV "
+            + "inverters or are unknown: frozenset({ComponentId(1)})"
+        ),
+    ):
+        datapipeline.new_pv_pool(priority=4, component_ids={ComponentId(1)})
+
+    with pytest.raises(
+        ValueError,
+        match=re.escape(
+            "Unable to create an EVChargerPool. These component IDs are either "
+            + "not EV chargers or are unknown: frozenset({ComponentId(4)})"
+        ),
+    ):
+        datapipeline.new_ev_charger_pool(priority=5, component_ids={ComponentId(4)})
 
     assert datapipeline._battery_power_wrapper._power_distributing_actor is not None
     await asyncio.sleep(1)