From 3dc80247b3ad438f75050a9f55c8e48a78bda67c Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:29:01 +0100 Subject: [PATCH 1/6] Revert "Add a new predicate to skip messages that are equal to the previous one" With the upcoming name change of `OnlyIfPrevious` to `WithPrevious`, it doesn't look like `filter(WithPrevious(lambda prev, new: prev != new))` is so ugly and verbose to justify having `ChangedOnly` as a special case. Specially since the name is a bit confusing, and not clear enough about what it does. The verbose alternative make it more clear that it's comparing to the previous message. This reverts commit 2b9541ceba62e5e5928ad0905f61c3495bc15280. Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 1 - .../channels/experimental/__init__.py | 3 +- .../channels/experimental/_predicates.py | 59 --------- .../test_predicates_changed_only.py | 121 ------------------ 4 files changed, 1 insertion(+), 183 deletions(-) delete mode 100644 tests/experimental/test_predicates_changed_only.py diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index fd3ec90f..80bd24e0 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -5,4 +5,3 @@ ### Experimental - A new predicate, `OnlyIfPrevious`, to `filter()` messages based on the previous message. -- A new special case of `OnlyIfPrevious`, `ChangedOnly`, to skip messages if they are equal to the previous message. diff --git a/src/frequenz/channels/experimental/__init__.py b/src/frequenz/channels/experimental/__init__.py index 6fe6b0d0..99c52d8f 100644 --- a/src/frequenz/channels/experimental/__init__.py +++ b/src/frequenz/channels/experimental/__init__.py @@ -10,11 +10,10 @@ """ from ._pipe import Pipe -from ._predicates import ChangedOnly, OnlyIfPrevious +from ._predicates import OnlyIfPrevious from ._relay_sender import RelaySender __all__ = [ - "ChangedOnly", "OnlyIfPrevious", "Pipe", "RelaySender", diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_predicates.py index bf5e45f2..7eb44bd9 100644 --- a/src/frequenz/channels/experimental/_predicates.py +++ b/src/frequenz/channels/experimental/_predicates.py @@ -28,10 +28,6 @@ class OnlyIfPrevious(Generic[ChannelMessageT]): process messages only if they satisfy a particular condition with respect to the previous message. - Tip: - If you want to use `==` as predicate, you can use the - [`ChangedOnly`][frequenz.channels.experimental.ChangedOnly] predicate. - Example: Receiving only messages that are not the same instance as the previous one. ```python from frequenz.channels import Broadcast @@ -127,58 +123,3 @@ def __str__(self) -> str: def __repr__(self) -> str: """Return a string representation of this instance.""" return f"<{type(self).__name__}: {self._predicate!r} first_is_true={self._first_is_true!r}>" - - -class ChangedOnly(OnlyIfPrevious[object]): - """A predicate to check if a message is different from the previous one. - - This predicate can be used to filter out messages that are the same as the previous - one. This can be useful in cases where you want to avoid processing duplicate - messages. - - Warning: - This predicate uses the `!=` operator to compare messages, which includes all - the weirdnesses of Python's equality comparison (e.g., `1 == 1.0`, `True == 1`, - `True == 1.0`, `False == 0` are all `True`). - - If you need to use a different comparison, you can create a custom predicate - using [`OnlyIfPrevious`][frequenz.channels.experimental.OnlyIfPrevious]. - - Example: - ```python - from frequenz.channels import Broadcast - from frequenz.channels.experimental import ChangedOnly - - channel = Broadcast[int](name="skip_duplicates_test") - receiver = channel.new_receiver().filter(ChangedOnly()) - sender = channel.new_sender() - - # This message will be received as it is the first message. - await sender.send(1) - assert await receiver.receive() == 1 - - # This message will be skipped as it is the same as the previous one. - await sender.send(1) - - # This message will be received as it is different from the previous one. - await sender.send(2) - assert await receiver.receive() == 2 - ``` - """ - - def __init__(self, *, first_is_true: bool = True) -> None: - """Initialize this instance. - - Args: - first_is_true: Whether the first message should be considered as different - from the previous one. Defaults to `True`. - """ - super().__init__(lambda old, new: old != new, first_is_true=first_is_true) - - def __str__(self) -> str: - """Return a string representation of this instance.""" - return f"{type(self).__name__}" - - def __repr__(self) -> str: - """Return a string representation of this instance.""" - return f"{type(self).__name__}(first_is_true={self._first_is_true!r})" diff --git a/tests/experimental/test_predicates_changed_only.py b/tests/experimental/test_predicates_changed_only.py deleted file mode 100644 index 4d735d85..00000000 --- a/tests/experimental/test_predicates_changed_only.py +++ /dev/null @@ -1,121 +0,0 @@ -# License: MIT -# Copyright © 2024 Frequenz Energy-as-a-Service GmbH - -"""Tests for the ChangedOnly implementation. - -Most testing is done in the OnlyIfPrevious tests, these tests are limited to the -specifics of the ChangedOnly implementation. -""" - -from dataclasses import dataclass -from unittest.mock import MagicMock - -import pytest - -from frequenz.channels.experimental import ChangedOnly, OnlyIfPrevious - - -@dataclass(frozen=True, kw_only=True) -class EqualityTestCase: - """Test case for testing ChangedOnly behavior with tricky equality cases.""" - - title: str - first_value: object - second_value: object - expected_second_result: bool - - -EQUALITY_TEST_CASES = [ - # Python's equality weirdness cases - EqualityTestCase( - title="Integer equals float", - first_value=1, - second_value=1.0, - expected_second_result=False, - ), - EqualityTestCase( - title="Boolean equals integer", - first_value=True, - second_value=1, - expected_second_result=False, - ), - EqualityTestCase( - title="Boolean equals float", - first_value=True, - second_value=1.0, - expected_second_result=False, - ), - EqualityTestCase( - title="False equals zero", - first_value=False, - second_value=0, - expected_second_result=False, - ), - EqualityTestCase( - title="Zero equals False", - first_value=0, - second_value=False, - expected_second_result=False, - ), - # Edge cases that should be different - EqualityTestCase( - title="NaN is never equal to NaN", - first_value=float("nan"), - second_value=float("nan"), - expected_second_result=True, - ), - EqualityTestCase( - title="Different list instances with same content", - first_value=[1], - second_value=[1], - expected_second_result=False, - ), -] - - -def test_changed_only_inheritance() -> None: - """Test that ChangedOnly is properly inheriting from OnlyIfPrevious.""" - changed_only = ChangedOnly() - assert isinstance(changed_only, OnlyIfPrevious) - - -def test_changed_only_predicate_implementation() -> None: - """Test that ChangedOnly properly implements the inequality predicate.""" - # Create mock objects that we can control the equality comparison for - old = MagicMock() - new = MagicMock() - - # Set up the inequality comparison - # mypy doesn't understand mocking __ne__ very well - old.__ne__.return_value = True # type: ignore[attr-defined] - - changed_only = ChangedOnly() - # Skip the first message as it's handled by first_is_true - changed_only(old) - changed_only(new) - - # Verify that __ne__ was called with the correct argument - old.__ne__.assert_called_once_with(new) # type: ignore[attr-defined] - - -@pytest.mark.parametrize( - "test_case", - EQUALITY_TEST_CASES, - ids=lambda test_case: test_case.title, -) -def test_changed_only_equality_cases(test_case: EqualityTestCase) -> None: - """Test ChangedOnly behavior with Python's tricky equality cases. - - Args: - test_case: The test case containing the input values and expected result. - """ - changed_only = ChangedOnly() - assert changed_only(test_case.first_value) is True # First is always True - assert changed_only(test_case.second_value) is test_case.expected_second_result - - -def test_changed_only_representation() -> None: - """Test the string representation of ChangedOnly.""" - changed_only = ChangedOnly() - assert str(changed_only) == "ChangedOnly" - assert repr(changed_only) == "ChangedOnly(first_is_true=True)" From 23912e9f4bee28a87cf63c37d1ef63fe8d4da175 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:43:30 +0100 Subject: [PATCH 2/6] Rename `OnlyIfPrevious` to `WithPrevious` The new name makes is more clear that this predicate is adding an extra condition to the message filtering based on the previous message. Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 2 +- .../channels/experimental/__init__.py | 4 +-- .../channels/experimental/_predicates.py | 10 +++---- .../test_predicates_only_if_previous.py | 26 +++++++++---------- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 80bd24e0..2c09c46c 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -4,4 +4,4 @@ ### Experimental -- A new predicate, `OnlyIfPrevious`, to `filter()` messages based on the previous message. +- A new composable predicate, `WithPrevious`, to filter messages based on the previous message. diff --git a/src/frequenz/channels/experimental/__init__.py b/src/frequenz/channels/experimental/__init__.py index 99c52d8f..2cf30110 100644 --- a/src/frequenz/channels/experimental/__init__.py +++ b/src/frequenz/channels/experimental/__init__.py @@ -10,11 +10,11 @@ """ from ._pipe import Pipe -from ._predicates import OnlyIfPrevious +from ._predicates import WithPrevious from ._relay_sender import RelaySender __all__ = [ - "OnlyIfPrevious", + "WithPrevious", "Pipe", "RelaySender", ] diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_predicates.py index 7eb44bd9..8082967a 100644 --- a/src/frequenz/channels/experimental/_predicates.py +++ b/src/frequenz/channels/experimental/_predicates.py @@ -20,7 +20,7 @@ def __str__(self) -> str: _SENTINEL: Final[_Sentinel] = _Sentinel() -class OnlyIfPrevious(Generic[ChannelMessageT]): +class WithPrevious(Generic[ChannelMessageT]): """A predicate to check if a message has a particular relationship with the previous one. This predicate can be used to filter out messages based on a custom condition on the @@ -31,10 +31,10 @@ class OnlyIfPrevious(Generic[ChannelMessageT]): Example: Receiving only messages that are not the same instance as the previous one. ```python from frequenz.channels import Broadcast - from frequenz.channels.experimental import OnlyIfPrevious + from frequenz.channels.experimental import WithPrevious channel = Broadcast[int | bool](name="example") - receiver = channel.new_receiver().filter(OnlyIfPrevious(lambda old, new: old is not new)) + receiver = channel.new_receiver().filter(WithPrevious(lambda old, new: old is not new)) sender = channel.new_sender() # This message will be received as it is the first message. @@ -53,11 +53,11 @@ class OnlyIfPrevious(Generic[ChannelMessageT]): Example: Receiving only messages if they are bigger than the previous one. ```python from frequenz.channels import Broadcast - from frequenz.channels.experimental import OnlyIfPrevious + from frequenz.channels.experimental import WithPrevious channel = Broadcast[int](name="example") receiver = channel.new_receiver().filter( - OnlyIfPrevious(lambda old, new: new > old, first_is_true=False) + WithPrevious(lambda old, new: new > old, first_is_true=False) ) sender = channel.new_sender() diff --git a/tests/experimental/test_predicates_only_if_previous.py b/tests/experimental/test_predicates_only_if_previous.py index b76bb96d..6c720759 100644 --- a/tests/experimental/test_predicates_only_if_previous.py +++ b/tests/experimental/test_predicates_only_if_previous.py @@ -1,21 +1,21 @@ # License: MIT # Copyright © 2024 Frequenz Energy-as-a-Service GmbH -"""Tests for the OnlyIfPrevious implementation.""" +"""Tests for the WithPrevious implementation.""" from dataclasses import dataclass from typing import Callable, Generic, TypeVar import pytest -from frequenz.channels.experimental import OnlyIfPrevious +from frequenz.channels.experimental import WithPrevious _T = TypeVar("_T") @dataclass(frozen=True, kw_only=True) class PredicateTestCase(Generic[_T]): - """Test case for testing OnlyIfPrevious behavior with different predicates.""" + """Test case for testing WithPrevious behavior with different predicates.""" title: str messages: list[_T] @@ -113,12 +113,12 @@ def is_not_same_instance(old: object, new: object) -> bool: ids=lambda test_case: test_case.title, ) def test_only_if_previous(test_case: PredicateTestCase[_T]) -> None: - """Test the OnlyIfPrevious with different predicates and sequences. + """Test the WithPrevious with different predicates and sequences. Args: test_case: The test case containing the input values and expected results. """ - only_if_previous = OnlyIfPrevious( + only_if_previous = WithPrevious( test_case.predicate, first_is_true=test_case.first_is_true, ) @@ -127,9 +127,9 @@ def test_only_if_previous(test_case: PredicateTestCase[_T]) -> None: def test_only_if_previous_state_independence() -> None: - """Test that multiple OnlyIfPrevious instances maintain independent state.""" - only_if_previous1 = OnlyIfPrevious(is_greater) - only_if_previous2 = OnlyIfPrevious(is_greater) + """Test that multiple WithPrevious instances maintain independent state.""" + only_if_previous1 = WithPrevious(is_greater) + only_if_previous2 = WithPrevious(is_greater) # First message should be accepted (first_is_true default is True) assert only_if_previous1(1) is True @@ -141,17 +141,17 @@ def test_only_if_previous_state_independence() -> None: def test_only_if_previous_str_representation() -> None: - """Test the string representation of OnlyIfPrevious.""" - only_if_previous = OnlyIfPrevious(is_greater) - assert str(only_if_previous) == "OnlyIfPrevious:is_greater" + """Test the string representation of WithPrevious.""" + only_if_previous = WithPrevious(is_greater) + assert str(only_if_previous) == "WithPrevious:is_greater" assert ( - repr(only_if_previous) == f"" + repr(only_if_previous) == f"" ) def test_only_if_previous_sentinel_str() -> None: """Test the string representation of the sentinel value.""" - only_if_previous = OnlyIfPrevious(always_true) + only_if_previous = WithPrevious(always_true) # Access the private attribute for testing purposes # pylint: disable=protected-access From e122ec708b354c61c40d794f678bd91feea1a973 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:46:47 +0100 Subject: [PATCH 3/6] Improve the documentation Make it more clear that `WithPrevious` is a composable predicate, to build new predicates. Signed-off-by: Leandro Lucarella --- src/frequenz/channels/experimental/_predicates.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_predicates.py index 8082967a..9cfa0e05 100644 --- a/src/frequenz/channels/experimental/_predicates.py +++ b/src/frequenz/channels/experimental/_predicates.py @@ -1,7 +1,7 @@ # License: MIT # Copyright © 2024 Frequenz Energy-as-a-Service GmbH -"""Predicates to be used in conjuntion with `Receiver.filter()`.""" +"""Composable predicate to cache and compare with the previous message.""" from typing import Callable, Final, Generic, TypeGuard @@ -21,9 +21,9 @@ def __str__(self) -> str: class WithPrevious(Generic[ChannelMessageT]): - """A predicate to check if a message has a particular relationship with the previous one. + """A composable predicate to build predicates that can use also the previous message. - This predicate can be used to filter out messages based on a custom condition on the + This predicate can be used to filter messages based on a custom condition on the previous and current messages. This can be useful in cases where you want to process messages only if they satisfy a particular condition with respect to the previous message. From 7c49e88b3608c67d95c91fd4aa30f45cb146d4ea Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:48:14 +0100 Subject: [PATCH 4/6] Update the example to show how to receive only changed messages Now that `ChangedOnly` was removed it makes more sense to use that as an example instead of a more obscure example using `is` for comparison. Signed-off-by: Leandro Lucarella --- src/frequenz/channels/experimental/_predicates.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_predicates.py index 9cfa0e05..b6e5930b 100644 --- a/src/frequenz/channels/experimental/_predicates.py +++ b/src/frequenz/channels/experimental/_predicates.py @@ -28,26 +28,25 @@ class WithPrevious(Generic[ChannelMessageT]): process messages only if they satisfy a particular condition with respect to the previous message. - Example: Receiving only messages that are not the same instance as the previous one. + Example: Receiving only messages that are different from the previous one. ```python from frequenz.channels import Broadcast from frequenz.channels.experimental import WithPrevious - channel = Broadcast[int | bool](name="example") - receiver = channel.new_receiver().filter(WithPrevious(lambda old, new: old is not new)) + channel = Broadcast[int](name="example") + receiver = channel.new_receiver().filter(WithPrevious(lambda old, new: old != new)) sender = channel.new_sender() # This message will be received as it is the first message. await sender.send(1) assert await receiver.receive() == 1 - # This message will be skipped as it is the same instance as the previous one. + # This message will be skipped as it equals to the previous one. await sender.send(1) - # This message will be received as it is a different instance from the previous - # one. - await sender.send(True) - assert await receiver.receive() is True + # This message will be received as it is a different from the previous one. + await sender.send(0) + assert await receiver.receive() == 0 ``` Example: Receiving only messages if they are bigger than the previous one. From aa1d43a51460025db9274743bf8d9f8a913bda66 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:49:24 +0100 Subject: [PATCH 5/6] Make the predicate argument positional-only This matches the `filter()` method. Signed-off-by: Leandro Lucarella --- src/frequenz/channels/experimental/_predicates.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_predicates.py index b6e5930b..90ab5a55 100644 --- a/src/frequenz/channels/experimental/_predicates.py +++ b/src/frequenz/channels/experimental/_predicates.py @@ -85,6 +85,7 @@ class WithPrevious(Generic[ChannelMessageT]): def __init__( self, predicate: Callable[[ChannelMessageT, ChannelMessageT], bool], + /, *, first_is_true: bool = True, ) -> None: From 474a1dfa01f6e14bff09425a2c5f663e0ee1b25e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 4 Dec 2024 09:50:29 +0100 Subject: [PATCH 6/6] Rename files to match the new content We don't plan to add more predicates for now, so we rename `_predicates.py` to `_with_previous.py` and `test_predicates_only_if_previous.py` to `test_with_previous.py`. Signed-off-by: Leandro Lucarella --- src/frequenz/channels/experimental/__init__.py | 2 +- .../channels/experimental/{_predicates.py => _with_previous.py} | 0 ...est_predicates_only_if_previous.py => test_with_previous.py} | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename src/frequenz/channels/experimental/{_predicates.py => _with_previous.py} (100%) rename tests/experimental/{test_predicates_only_if_previous.py => test_with_previous.py} (100%) diff --git a/src/frequenz/channels/experimental/__init__.py b/src/frequenz/channels/experimental/__init__.py index 2cf30110..940cc304 100644 --- a/src/frequenz/channels/experimental/__init__.py +++ b/src/frequenz/channels/experimental/__init__.py @@ -10,8 +10,8 @@ """ from ._pipe import Pipe -from ._predicates import WithPrevious from ._relay_sender import RelaySender +from ._with_previous import WithPrevious __all__ = [ "WithPrevious", diff --git a/src/frequenz/channels/experimental/_predicates.py b/src/frequenz/channels/experimental/_with_previous.py similarity index 100% rename from src/frequenz/channels/experimental/_predicates.py rename to src/frequenz/channels/experimental/_with_previous.py diff --git a/tests/experimental/test_predicates_only_if_previous.py b/tests/experimental/test_with_previous.py similarity index 100% rename from tests/experimental/test_predicates_only_if_previous.py rename to tests/experimental/test_with_previous.py