From ee89173315b3c26fe52cdcf2a9fa223bbebd4ddb Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 13 Mar 2025 11:47:50 +0100 Subject: [PATCH 1/5] Sort component IDs in exceptions These component IDs are stored in sets, which don't have a order guarantee, and looking at component IDs in exceptions with random order makes it hard to identify if a particular component is in the list or not. This also makes it easier to write tests that are not flaky or depend on the component's order. To improve the exceptions also `", ".join()` is used to get less noise in the exception message (like `frozenset({4, 2, 6})` vs `2, 4, 5`). This makes exceptions easier to read because it is easier to look for a component ID Signed-off-by: Leandro Lucarella --- .../sdk/microgrid/_power_managing/_matryoshka.py | 3 ++- src/frequenz/sdk/microgrid/component_graph.py | 10 +++++----- tests/actor/_power_managing/test_matryoshka.py | 8 ++++---- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py b/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py index 6c8fc8751..58b037f93 100644 --- a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py +++ b/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." ) diff --git a/src/frequenz/sdk/microgrid/component_graph.py b/src/frequenz/sdk/microgrid/component_graph.py index f5cdc081e..5c01304bf 100644 --- a/src/frequenz/sdk/microgrid/component_graph.py +++ b/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!") diff --git a/tests/actor/_power_managing/test_matryoshka.py b/tests/actor/_power_managing/test_matryoshka.py index 03aabff36..589e7b139 100644 --- a/tests/actor/_power_managing/test_matryoshka.py +++ b/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( From c5ff25030ded1576e594de16ba78318d1929a73b Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 12 Mar 2025 11:52:45 +0100 Subject: [PATCH 2/5] Don't rely on the order of a battery set in tests These tests 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. Signed-off-by: Leandro Lucarella --- .../_battery_pool/test_battery_pool.py | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/timeseries/_battery_pool/test_battery_pool.py b/tests/timeseries/_battery_pool/test_battery_pool.py index b19a5a08b..5123f759e 100644 --- a/tests/timeseries/_battery_pool/test_battery_pool.py +++ b/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]])), From e2f903db86345cd7eef9ae76508b20123ad22961 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 12 Mar 2025 14:49:14 +0100 Subject: [PATCH 3/5] Fix sorting of inverters The inverters were considered as sorted to have the largest exclusion bounds first, but since they were passed as a set, there is not ordering guarantees. We now sort them to ensure we are allocating the excess power to inverters with more capacity first. Signed-off-by: Leandro Lucarella --- .../_battery_distribution_algorithm.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py b/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py index b4e390e1d..92dcc1e72 100644 --- a/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py +++ b/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 From 2a816b8ec5912bc3eb81eefa178ffad75a3114b0 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 13 Mar 2025 11:03:42 +0100 Subject: [PATCH 4/5] Fix tests depending on the order of inverters When testing the power distribution algorithm, we can't rely on which amount of power goes to each inverter when all inverters have the same bounds, because they are stored in a `set`, which provide no order guarantee, and when distributing power, power is only prioritized using bounds. Signed-off-by: Leandro Lucarella --- .../test_battery_distribution_algorithm.py | 71 +++++++++++++------ 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/tests/actor/power_distributing/test_battery_distribution_algorithm.py b/tests/actor/power_distributing/test_battery_distribution_algorithm.py index 0ecf37a81..4397fc050 100644 --- a/tests/actor/power_distributing/test_battery_distribution_algorithm.py +++ b/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, ) From ec77bad3ba8e4a1239628009d6b28ba5c4fe1f15 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 20 Feb 2025 12:02:12 +0100 Subject: [PATCH 5/5] Use a custom formula in the higher order formula test When testing the `FormulaFormatter`, the `microgrid.pv_pool().power` and `microgrid.grid.power` formulas were used. This pulls a lot of complexity in, as the whole microgrid needs to be mocked. It also makes the test nondeterministic, as the components returned by the component graph, which are used to build the formulas, are returned as a set, which doesn't provide order, so the resulting formula string will vary from run to run. To test higher order formulas, we now build custom formulas instead. Signed-off-by: Leandro Lucarella --- tests/timeseries/test_formula_formatter.py | 64 ++++++++++++---------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/tests/timeseries/test_formula_formatter.py b/tests/timeseries/test_formula_formatter.py index 784c15f8c..f95c4ab3b 100644 --- a/tests/timeseries/test_formula_formatter.py +++ b/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)" + )