Fix ordering issues with sets of ints (frequenz-floss#1177)

Sets of `int` have a special behavior in Python (at least CPython), as
it seems the hashing function is the `int` itself, resulting in `set`
being ordered when iterated. But this is not guaranteed and could change
at any time.

The code is unfortunately sometimes relaying in this `set` order, and
this PR fixes it by using `sorted` when needed or making tests more
robust and work reliable when the order is not guaranteed.

This is necessary to transition to using the new microgrid client
`ComponentId` to represent component IDs.

Author: llucax

src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py

@@ -609,8 +609,12 @@ def _distribute_multi_inverter_pairs(
             else:
                 remaining_power = power.power
 
-                # Inverters are sorted by largest excl bound first
-                for inverter_id in inverter_ids:
+                # Sort inverters to have the largest exclusion bounds first
+                sorted_inverter_ids = sorted(
+                    inverter_ids, key=lambda inv_id: excl_bounds[inv_id], reverse=True
+                )
+
+                for inverter_id in sorted_inverter_ids:
                     if (
                         not is_close_to_zero(remaining_power)
                         and excl_bounds[inverter_id] <= remaining_power

src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py

@@ -144,8 +144,9 @@ def _validate_component_ids(
 
             for bucket in self._component_buckets:
                 if any(component_id in bucket for component_id in component_ids):
+                    comp_ids = ", ".join(map(str, sorted(component_ids)))
                     raise NotImplementedError(
-                        f"PowerManagingActor: component IDs {component_ids} are already"
+                        f"PowerManagingActor: component IDs {comp_ids} are already"
                         " part of another bucket.  Overlapping buckets are not"
                         " yet supported."
                     )

src/frequenz/sdk/microgrid/component_graph.py

@@ -915,14 +915,14 @@ def _validate_graph(self) -> None:
         # This check doesn't seem to have much sense, it only search for nodes without
         # data associated with them. We leave it here for now, but we should consider
         # removing it in the future.
-        undefined: list[int] = [
+        if undefined := [
             node[0] for node in self._graph.nodes(data=True) if len(node[1]) == 0
-        ]
-
-        if len(undefined) > 0:
+        ]:
+            undefined_str = ", ".join(map(str, sorted(undefined)))
             raise InvalidGraphError(
-                f"Missing definition for graph components: {undefined}"
+                f"Missing definition for graph components: {undefined_str}"
             )
+
         # should be true as a consequence of checks above
         if sum(1 for _ in self.components()) <= 0:
             raise InvalidGraphError("Graph must have a least one component!")

tests/actor/_power_managing/test_matryoshka.py

@@ -435,8 +435,8 @@ async def test_matryoshka_drop_old_proposals() -> None:
     with pytest.raises(
         NotImplementedError,
         match=re.escape(
-            "PowerManagingActor: component IDs frozenset({8, 5}) are already "
-            + "part of another bucket.  Overlapping buckets are not yet supported."
+            "PowerManagingActor: component IDs 5, 8 are already part of "
+            "another bucket.  Overlapping buckets are not yet supported."
         ),
     ):
         tester.tgt_power(
@@ -506,8 +506,8 @@ def ensure_overlapping_bucket_request_fails() -> None:
         with pytest.raises(
             NotImplementedError,
             match=re.escape(
-                "PowerManagingActor: component IDs frozenset({8, 5}) are already "
-                + "part of another bucket.  Overlapping buckets are not yet supported."
+                "PowerManagingActor: component IDs 5, 8 are already part of "
+                "another bucket.  Overlapping buckets are not yet supported."
             ),
         ):
             tester.tgt_power(

tests/actor/power_distributing/test_battery_distribution_algorithm.py

@@ -4,6 +4,7 @@
 # pylint: disable=too-many-lines
 """Tests for distribution algorithm."""
 import math
+from collections.abc import Sequence, Set
 from dataclasses import dataclass
 from datetime import datetime, timezone
 
@@ -1023,6 +1024,20 @@ def assert_result(result: DistributionResult, expected: DistributionResult) -> N
         assert result.distribution == approx(expected.distribution, abs=0.01)
         assert result.remaining_power == approx(expected.remaining_power, abs=0.01)
 
+    @staticmethod
+    def assert_any_result(
+        result: DistributionResult,
+        expected_distribution_ids: Set[int],
+        expected_distribution_powers: Sequence[float],
+        expected_remaining_power: float,
+    ) -> None:
+        """Assert the result is as expected, disregarding which power goes to which component."""
+        assert result.remaining_power == expected_remaining_power
+        assert {*result.distribution.keys()} == expected_distribution_ids
+        assert list(sorted(result.distribution.values())) == list(
+            sorted(expected_distribution_powers)
+        )
+
     def test_scenario_1(self) -> None:
         """Test scenario 1.
 
@@ -1349,19 +1364,27 @@ def test_scenario_4(self) -> None:
 
         algorithm = BatteryDistributionAlgorithm()
 
-        self.assert_result(
+        # The assignment of power to each inverter can be swapped, as they both have the
+        # same bounds, none will be preferred
+        self.assert_any_result(
             algorithm.distribute_power(-300, components),
-            DistributionResult({2: -300, 3: 0}, remaining_power=0.0),
+            expected_distribution_ids={2, 3},
+            expected_distribution_powers=[-300, 0],
+            expected_remaining_power=0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(300, components),
-            DistributionResult({2: 300, 3: 0}, remaining_power=0.0),
+            expected_distribution_ids={2, 3},
+            expected_distribution_powers=[300, 0],
+            expected_remaining_power=0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(-1800, components),
-            DistributionResult({2: -1000, 3: -500}, remaining_power=-300.0),
+            expected_distribution_ids={2, 3},
+            expected_distribution_powers=[-1000, -500],
+            expected_remaining_power=-300,
         )
 
     def test_scenario_5(self) -> None:
@@ -1431,33 +1454,37 @@ def test_scenario_5(self) -> None:
 
         algorithm = BatteryDistributionAlgorithm()
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(-300, components),
-            DistributionResult({10: -200, 11: 0, 20: -100, 21: 0}, remaining_power=0.0),
+            expected_distribution_ids={10, 11, 20, 21},
+            expected_distribution_powers=[-200, 0, -100, 0],
+            expected_remaining_power=0.0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(300, components),
-            DistributionResult({10: 200, 11: 0, 20: 100, 21: 0}, remaining_power=0.0),
+            expected_distribution_ids={10, 11, 20, 21},
+            expected_distribution_powers=[200, 0, 100, 0],
+            expected_remaining_power=0.0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(-1800, components),
-            DistributionResult(
-                {10: -300, 11: 0, 20: -1000, 21: -500}, remaining_power=0.0
-            ),
+            expected_distribution_ids={10, 11, 20, 21},
+            expected_distribution_powers=[-300, 0, -1000, -500],
+            expected_remaining_power=0.0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(3000, components),
-            DistributionResult(
-                {10: 1000, 11: 500, 20: 1000, 21: 500}, remaining_power=0.0
-            ),
+            expected_distribution_ids={10, 11, 20, 21},
+            expected_distribution_powers=[1000, 500, 1000, 500],
+            expected_remaining_power=0.0,
         )
 
-        self.assert_result(
+        self.assert_any_result(
             algorithm.distribute_power(3500, components),
-            DistributionResult(
-                {10: 1000, 11: 500, 20: 1000, 21: 500}, remaining_power=500.0
-            ),
+            expected_distribution_ids={10, 11, 20, 21},
+            expected_distribution_powers=[1000, 500, 1000, 500],
+            expected_remaining_power=500.0,
         )

tests/timeseries/_battery_pool/test_battery_pool.py

@@ -198,11 +198,21 @@ async def setup_batteries_pool(mocker: MockerFixture) -> AsyncIterator[SetupArgs
     # the scope of this tests. This tests should cover BatteryPool only.
     # We use our own battery status channel, where we easily control set of working
     # batteries.
-    all_batteries = list(get_components(mock_microgrid, ComponentCategory.BATTERY))
+    all_batteries = get_components(mock_microgrid, ComponentCategory.BATTERY)
 
-    battery_pool = microgrid.new_battery_pool(
-        priority=5, component_ids=set(all_batteries[:2])
-    )
+    # This is a hack because these tests used to rely on the order in which components
+    # are returned from the component graph using `all_batteries[:2]` to get the first 2
+    # batteries. But the component graph returns a set, which doesn't have any ordering
+    # guarantees, so if the Python implementation changes, the hashing can change and
+    # this resulting order could be different, breaking the tests. This is why we are
+    # now specifying IDs to use explicitly (these are the IDs that were used when the
+    # tests were written). This can also change in the future if generator of mock
+    # components changes, as it might produce different IDs, so this solution is also
+    # not bullet-proof.
+    assert 8 in all_batteries
+    assert 11 in all_batteries
+
+    battery_pool = microgrid.new_battery_pool(priority=5, component_ids=set([8, 11]))
 
     dp = microgrid._data_pipeline._DATA_PIPELINE
     assert dp is not None
@@ -1125,7 +1135,7 @@ async def run_power_bounds_test(  # pylint: disable=too-many-locals
     )
     compare_messages(msg, expected)
 
-    batteries_in_pool = list(battery_pool.component_ids)
+    batteries_in_pool = list(sorted(battery_pool.component_ids))
     scenarios: list[Scenario[SystemBounds]] = [
         Scenario(
             next(iter(bat_invs_map[batteries_in_pool[0]])),

tests/timeseries/test_formula_formatter.py

@@ -4,15 +4,13 @@
 """Tests for the FormulaFormatter."""
 
 
-from contextlib import AsyncExitStack
-
 from frequenz.channels import Broadcast
 from frequenz.quantities import Quantity
-from pytest_mock import MockerFixture
 
-from frequenz.sdk import microgrid
 from frequenz.sdk.timeseries import Sample
-from frequenz.sdk.timeseries.formula_engine._formula_engine import FormulaBuilder
+from frequenz.sdk.timeseries.formula_engine._formula_engine import (
+    FormulaBuilder,
+)
 from frequenz.sdk.timeseries.formula_engine._formula_formatter import format_formula
 from frequenz.sdk.timeseries.formula_engine._formula_steps import (
     Clipper,
@@ -22,7 +20,6 @@
     Minimizer,
 )
 from frequenz.sdk.timeseries.formula_engine._tokenizer import Tokenizer, TokenType
-from tests.timeseries.mock_microgrid import MockMicrogrid
 
 
 def build_formula(formula: str) -> list[FormulaStep]:
@@ -112,27 +109,34 @@ def test_functions(self) -> None:
         # flake8: enable
         # fmt: on
 
-    async def test_higher_order_formula(self, mocker: MockerFixture) -> None:
-        """Test that the formula is formatted correctly for a higher-order formula."""
-        mockgrid = MockMicrogrid(grid_meter=False, mocker=mocker)
-        mockgrid.add_batteries(3)
-        mockgrid.add_ev_chargers(1)
-        mockgrid.add_solar_inverters(2)
-
-        async with mockgrid, AsyncExitStack() as stack:
-            logical_meter = microgrid.logical_meter()
-            stack.push_async_callback(logical_meter.stop)
-
-            pv_pool = microgrid.new_pv_pool(priority=5)
-            stack.push_async_callback(pv_pool.stop)
-
-            grid = microgrid.grid()
-            stack.push_async_callback(grid.stop)
-
-            assert str(grid.power) == "#36 + #7 + #47 + #17 + #57 + #27"
-
-            composed_formula = (grid.power - pv_pool.power).build("grid_minus_pv")
-            assert (
-                str(composed_formula)
-                == "[grid-power](#36 + #7 + #47 + #17 + #57 + #27) - [pv-power](#47 + #57)"
-            )
+    async def test_higher_order_formula(self) -> None:
+        """Test that higher-order formulas (formulas combining other formulas) are formatted correctly."""
+        # Create two base formulas
+        builder1 = FormulaBuilder("test_formula1", Quantity)
+        builder2 = FormulaBuilder("test_formula2", Quantity)
+
+        # Push metrics directly to the builders
+        channel1 = Broadcast[Sample[Quantity]](name="channel1")
+        channel2 = Broadcast[Sample[Quantity]](name="channel2")
+        builder1.push_metric("#1", channel1.new_receiver(), nones_are_zeros=True)
+        builder1.push_oper("+")
+        builder1.push_metric("#2", channel2.new_receiver(), nones_are_zeros=True)
+
+        channel3 = Broadcast[Sample[Quantity]](name="channel3")
+        channel4 = Broadcast[Sample[Quantity]](name="channel4")
+        builder2.push_metric("#3", channel3.new_receiver(), nones_are_zeros=True)
+        builder2.push_oper("+")
+        builder2.push_metric("#4", channel4.new_receiver(), nones_are_zeros=True)
+
+        # Build individual formula engines first
+        engine1 = builder1.build()
+        engine2 = builder2.build()
+
+        # Combine them into a higher-order formula
+        composed_formula = (engine1 - engine2).build("higher_order_formula")
+
+        # Check the string representation
+        assert (
+            str(composed_formula)
+            == "[test_formula1](#1 + #2) - [test_formula2](#3 + #4)"
+        )