diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 61ee6f2ad..2926cc45e 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -14,4 +14,4 @@ ## Bug Fixes - +- Fixed a bug that was preventing power proposals to go through if there once existed some proposals with overlapping component IDs, even if the old proposals have expired. diff --git a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py b/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py index af3f0af3b..6c8fc8751 100644 --- a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py +++ b/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py @@ -199,7 +199,15 @@ def calculate_target_power( bucket = self._component_buckets.setdefault(component_ids, set()) if proposal in bucket: bucket.remove(proposal) - bucket.add(proposal) + if ( + proposal.preferred_power is not None + or proposal.bounds.lower is not None + or proposal.bounds.upper is not None + ): + bucket.add(proposal) + elif not bucket: + del self._component_buckets[component_ids] + _ = self._target_power.pop(component_ids, None) # If there has not been any proposal for the given components, don't calculate a # target power and just return `None`. @@ -292,10 +300,17 @@ def drop_old_proposals(self, loop_time: float) -> None: Args: loop_time: The current loop time. """ - for bucket in self._component_buckets.values(): - to_delete = [] - for proposal in bucket: + buckets_to_delete: list[frozenset[int]] = [] + for component_ids, proposals in self._component_buckets.items(): + to_delete: list[Proposal] = [] + for proposal in proposals: if (loop_time - proposal.creation_time) > self._max_proposal_age_sec: to_delete.append(proposal) for proposal in to_delete: - bucket.remove(proposal) + proposals.remove(proposal) + if not proposals: + buckets_to_delete.append(component_ids) + + for component_ids in buckets_to_delete: + del self._component_buckets[component_ids] + _ = self._target_power.pop(component_ids, None) diff --git a/tests/actor/_power_managing/test_matryoshka.py b/tests/actor/_power_managing/test_matryoshka.py index 6828f98a4..03aabff36 100644 --- a/tests/actor/_power_managing/test_matryoshka.py +++ b/tests/actor/_power_managing/test_matryoshka.py @@ -4,8 +4,10 @@ """Tests for the Matryoshka power manager algorithm.""" import asyncio +import re from datetime import datetime, timedelta, timezone +import pytest from frequenz.quantities import Power from frequenz.sdk import timeseries @@ -36,13 +38,14 @@ def tgt_power( # pylint: disable=too-many-arguments,too-many-positional-argumen expected: float | None, creation_time: float | None = None, must_send: bool = False, + batteries: frozenset[int] | None = None, ) -> None: """Test the target power calculation.""" self._call_count += 1 tgt_power = self.algorithm.calculate_target_power( - self._batteries, + self._batteries if batteries is None else batteries, Proposal( - component_ids=self._batteries, + component_ids=self._batteries if batteries is None else batteries, source_id=f"actor-{priority}", preferred_power=None if power is None else Power.from_watts(power), bounds=timeseries.Bounds( @@ -368,6 +371,7 @@ async def test_matryoshka_drop_old_proposals() -> None: With inclusion bounds, and exclusion bounds -30.0 to 30.0. """ batteries = frozenset({2, 5}) + overlapping_batteries = frozenset({5, 8}) system_bounds = _base_types.SystemBounds( timestamp=datetime.now(tz=timezone.utc), @@ -424,3 +428,115 @@ async def test_matryoshka_drop_old_proposals() -> None: tester.tgt_power( priority=1, power=20.0, bounds=(20.0, 50.0), expected=25.0, must_send=True ) + + # When all proposals are too old, they are dropped, and the buckets are dropped as + # well. After that, sending a request for a different but overlapping bucket will + # succeed. And it will fail until then. + 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." + ), + ): + tester.tgt_power( + priority=1, + power=25.0, + bounds=(25.0, 50.0), + expected=25.0, + must_send=True, + batteries=overlapping_batteries, + ) + + tester.tgt_power( + priority=1, + power=25.0, + bounds=(25.0, 50.0), + creation_time=now - 70.0, + expected=25.0, + must_send=True, + ) + tester.tgt_power( + priority=2, + power=25.0, + bounds=(25.0, 50.0), + creation_time=now - 70.0, + expected=25.0, + must_send=True, + ) + tester.tgt_power( + priority=3, + power=25.0, + bounds=(25.0, 50.0), + creation_time=now - 70.0, + expected=25.0, + must_send=True, + ) + + tester.algorithm.drop_old_proposals(now) + + tester.tgt_power( + priority=1, + power=25.0, + bounds=(25.0, 50.0), + expected=25.0, + must_send=True, + batteries=overlapping_batteries, + ) + + +async def test_matryoshka_none_proposals() -> None: + """Tests for the power managing actor. + + When a `None` proposal is received, is source id should be dropped from the bucket. + Then if the bucket becomes empty, it should be dropped as well. + """ + batteries = frozenset({2, 5}) + overlapping_batteries = frozenset({5, 8}) + + system_bounds = _base_types.SystemBounds( + timestamp=datetime.now(tz=timezone.utc), + inclusion_bounds=timeseries.Bounds( + lower=Power.from_watts(-200.0), upper=Power.from_watts(200.0) + ), + exclusion_bounds=timeseries.Bounds(lower=Power.zero(), upper=Power.zero()), + ) + + 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." + ), + ): + tester.tgt_power( + priority=1, + power=None, + bounds=(20.0, 50.0), + expected=None, + must_send=True, + batteries=overlapping_batteries, + ) + + tester = StatefulTester(batteries, system_bounds) + + tester.tgt_power(priority=3, power=22.0, bounds=(22.0, 30.0), expected=22.0) + tester.tgt_power(priority=2, power=25.0, bounds=(25.0, 50.0), expected=25.0) + tester.tgt_power(priority=1, power=20.0, bounds=(20.0, 50.0), expected=None) + + ensure_overlapping_bucket_request_fails() + tester.tgt_power(priority=1, power=None, bounds=(None, None), expected=None) + ensure_overlapping_bucket_request_fails() + tester.tgt_power(priority=3, power=None, bounds=(None, None), expected=None) + ensure_overlapping_bucket_request_fails() + tester.tgt_power(priority=2, power=None, bounds=(None, None), expected=None) + + # Overlapping battery bucket is dropped. + tester.tgt_power( + priority=1, + power=20.0, + bounds=(20.0, 50.0), + expected=20.0, + batteries=overlapping_batteries, + )