From e026f46a144e747c4685032f76945b7d7ddbd90b Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 00:36:21 +0100 Subject: [PATCH 1/6] Add tests for cooldown_period being applied correctly --- tests/triggers/test_combining.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/triggers/test_combining.py b/tests/triggers/test_combining.py index 03df3a71..8019cb36 100644 --- a/tests/triggers/test_combining.py +++ b/tests/triggers/test_combining.py @@ -200,11 +200,32 @@ def test_two_interval_triggers(self, timezone, serializer): # The end time of the 6 second interval has been reached assert trigger.next() is None + def test_cooldown_period(self, timezone, serializer): + start_time = datetime(2020, 5, 16, 14, 17, 30, 254212, tzinfo=timezone) + trigger = OrTrigger( + [ + IntervalTrigger(seconds=4, start_time=start_time), + IntervalTrigger(seconds=6, start_time=start_time), + ], + cooldown_period=1, + ) + if serializer: + trigger = serializer.deserialize(serializer.serialize(trigger)) + + assert trigger.next() == start_time + assert trigger.next() == start_time + timedelta(seconds=4) + assert trigger.next() == start_time + timedelta(seconds=6) + assert trigger.next() == start_time + timedelta(seconds=8) + assert trigger.next() == start_time + timedelta(seconds=12) # No second trigger -> cooldown + assert trigger.next() == start_time + timedelta(seconds=16) + assert trigger.next() == start_time + timedelta(seconds=18) + def test_repr(self, timezone): date1 = datetime(2020, 5, 16, 14, 17, 30, 254212, tzinfo=timezone) date2 = datetime(2020, 5, 18, 15, 1, 53, 940564, tzinfo=timezone) - trigger = OrTrigger([DateTrigger(date1), DateTrigger(date2)]) + trigger = OrTrigger([DateTrigger(date1), DateTrigger(date2)], 1) + print(repr(trigger)) assert repr(trigger) == ( "OrTrigger([DateTrigger('2020-05-16 14:17:30.254212+02:00'), " - "DateTrigger('2020-05-18 15:01:53.940564+02:00')])" + "DateTrigger('2020-05-18 15:01:53.940564+02:00')], cooldown_period=1.0)" ) From 6157355b0f4bc6b42e218ec420fd4ef7d8790381 Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 00:37:07 +0100 Subject: [PATCH 2/6] Implement cooldown_period in OrTrigger. --- src/apscheduler/triggers/combining.py | 79 ++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 14 deletions(-) diff --git a/src/apscheduler/triggers/combining.py b/src/apscheduler/triggers/combining.py index 1f8cee44..6e3a5e14 100644 --- a/src/apscheduler/triggers/combining.py +++ b/src/apscheduler/triggers/combining.py @@ -123,28 +123,79 @@ class OrTrigger(BaseCombiningTrigger): :param triggers: triggers to combine """ + cooldown_period: timedelta = attrs.field(converter=as_timedelta, default=0) + _last_fire_time: datetime | None = attrs.field(default=None, eq=False, init=False) + + def _get_next_valid_fire_time(self) -> tuple[datetime | None, list[int]]: + """ + Find the next valid fire time that respects the cooldown period. + + Returns: + A tuple of (fire_time, trigger_indices) where fire_time is the next valid + fire time (or None if no valid time exists) and trigger_indices is a list + of indices of triggers that produced this fire time. + """ + earliest_time = min( + (fire_time for fire_time in self._next_fire_times if fire_time is not None), + default=None, + ) + if earliest_time is None: + return None, [] + + # Find all triggers that produced this fire time + trigger_indices = [ + i for i, fire_time in enumerate(self._next_fire_times) + if fire_time == earliest_time + ] + + # Check if we need to respect cooldown period + if (self.cooldown_period > timedelta(0) and + self._last_fire_time is not None and + earliest_time - self._last_fire_time < self.cooldown_period): + # Get next fire times for all triggers that would have fired + for i in trigger_indices: + self._next_fire_times[i] = self.triggers[i].next() + # Recursively find next valid fire time + return self._get_next_valid_fire_time() + + return earliest_time, trigger_indices + def next(self) -> datetime | None: - # Fill out the fire times on the first run + """ + Get the next fire time that respects the cooldown period. + + Returns: + The next valid fire time, or None if no more fire times exist. + """ + # Initialize fire times if needed if not self._next_fire_times: self._next_fire_times = [t.next() for t in self.triggers] + self._last_fire_time = None - # Find out the earliest of the fire times - earliest_time: datetime | None = min( - (fire_time for fire_time in self._next_fire_times if fire_time is not None), - default=None, - ) - if earliest_time is not None: - # Generate new fire times for the trigger(s) that generated the earliest - # fire time - for i, fire_time in enumerate(self._next_fire_times): - if fire_time == earliest_time: - self._next_fire_times[i] = self.triggers[i].next() + # Get next valid fire time and affected triggers + try: + fire_time, trigger_indices = self._get_next_valid_fire_time() + except RecursionError: + # TODO: Replace the recursion with a loop + raise MaxIterationsReached + + if fire_time is not None: + # Update last fire time and get next fire times for triggered sources + self._last_fire_time = fire_time + for i in trigger_indices: + self._next_fire_times[i] = self.triggers[i].next() - return earliest_time + return fire_time def __setstate__(self, state: dict[str, Any]) -> None: require_state_version(self, state, 1) super().__setstate__(state) + self.cooldown_period = state["cooldown_period"] + + def __getstate__(self) -> dict[str, Any]: + state = super().__getstate__() + state["cooldown_period"] = self.cooldown_period + return state def __repr__(self) -> str: - return f"{self.__class__.__name__}({self.triggers})" + return f"{self.__class__.__name__}({self.triggers}, cooldown_period={self.cooldown_period.total_seconds()})" From 9dea0ef186a70c633505aec841b590cb7e3eeb6c Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 00:52:59 +0100 Subject: [PATCH 3/6] Added max_iterations to OrTrigger and removed recursion. Also fixed last_fire_time not being serialized --- src/apscheduler/triggers/combining.py | 70 +++++++++++++++++---------- tests/triggers/test_combining.py | 22 ++++++++- 2 files changed, 65 insertions(+), 27 deletions(-) diff --git a/src/apscheduler/triggers/combining.py b/src/apscheduler/triggers/combining.py index 6e3a5e14..2233fa0e 100644 --- a/src/apscheduler/triggers/combining.py +++ b/src/apscheduler/triggers/combining.py @@ -125,40 +125,53 @@ class OrTrigger(BaseCombiningTrigger): cooldown_period: timedelta = attrs.field(converter=as_timedelta, default=0) _last_fire_time: datetime | None = attrs.field(default=None, eq=False, init=False) + max_iterations: int | None = 10000 def _get_next_valid_fire_time(self) -> tuple[datetime | None, list[int]]: """ Find the next valid fire time that respects the cooldown period. + Raises: + MaxIterationsReached: If the maximum number of iterations is reached + Returns: A tuple of (fire_time, trigger_indices) where fire_time is the next valid fire time (or None if no valid time exists) and trigger_indices is a list of indices of triggers that produced this fire time. """ - earliest_time = min( - (fire_time for fire_time in self._next_fire_times if fire_time is not None), - default=None, - ) - if earliest_time is None: - return None, [] - - # Find all triggers that produced this fire time - trigger_indices = [ - i for i, fire_time in enumerate(self._next_fire_times) - if fire_time == earliest_time - ] - - # Check if we need to respect cooldown period - if (self.cooldown_period > timedelta(0) and - self._last_fire_time is not None and - earliest_time - self._last_fire_time < self.cooldown_period): - # Get next fire times for all triggers that would have fired - for i in trigger_indices: - self._next_fire_times[i] = self.triggers[i].next() - # Recursively find next valid fire time - return self._get_next_valid_fire_time() + for _ in range(self.max_iterations): + earliest_time = min( + ( + fire_time + for fire_time in self._next_fire_times + if fire_time is not None + ), + default=None, + ) + if earliest_time is None: + return None, [] + + # Find all triggers that produced this fire time + trigger_indices = [ + i + for i, fire_time in enumerate(self._next_fire_times) + if fire_time == earliest_time + ] + + # Check if we need to respect cooldown period + if ( + self.cooldown_period > timedelta(0) + and self._last_fire_time is not None + and earliest_time - self._last_fire_time < self.cooldown_period + ): + # Get next fire times for all triggers that would have fired + for i in trigger_indices: + self._next_fire_times[i] = self.triggers[i].next() + continue - return earliest_time, trigger_indices + return earliest_time, trigger_indices + else: + raise MaxIterationsReached def next(self) -> datetime | None: """ @@ -176,7 +189,6 @@ def next(self) -> datetime | None: try: fire_time, trigger_indices = self._get_next_valid_fire_time() except RecursionError: - # TODO: Replace the recursion with a loop raise MaxIterationsReached if fire_time is not None: @@ -191,11 +203,19 @@ def __setstate__(self, state: dict[str, Any]) -> None: require_state_version(self, state, 1) super().__setstate__(state) self.cooldown_period = state["cooldown_period"] + self._last_fire_time = state["last_fire_time"] + self.max_iterations = state["max_iterations"] def __getstate__(self) -> dict[str, Any]: state = super().__getstate__() state["cooldown_period"] = self.cooldown_period + state["last_fire_time"] = self._last_fire_time + state["max_iterations"] = self.max_iterations return state def __repr__(self) -> str: - return f"{self.__class__.__name__}({self.triggers}, cooldown_period={self.cooldown_period.total_seconds()})" + return ( + f"{self.__class__.__name__}" + f"({self.triggers}, cooldown_period={self.cooldown_period.total_seconds()}" + f", max_iterations={self.max_iterations})" + ) diff --git a/tests/triggers/test_combining.py b/tests/triggers/test_combining.py index 8019cb36..7ff44f90 100644 --- a/tests/triggers/test_combining.py +++ b/tests/triggers/test_combining.py @@ -220,12 +220,30 @@ def test_cooldown_period(self, timezone, serializer): assert trigger.next() == start_time + timedelta(seconds=16) assert trigger.next() == start_time + timedelta(seconds=18) + def test_max_iterations(self, timezone, serializer): + start_time = datetime(2020, 5, 16, 14, 17, 30, 254212, tzinfo=timezone) + trigger = OrTrigger( + [ + IntervalTrigger(seconds=1, start_time=start_time), + IntervalTrigger(seconds=1, start_time=start_time), + ], + cooldown_period=100, + # Max iterations should be reached before the cooldown period + max_iterations=10, + ) + if serializer: + trigger = serializer.deserialize(serializer.serialize(trigger)) + + # The triggers will keep firing after each other indefinitely + assert trigger.next() == start_time + pytest.raises(MaxIterationsReached, trigger.next) + def test_repr(self, timezone): date1 = datetime(2020, 5, 16, 14, 17, 30, 254212, tzinfo=timezone) date2 = datetime(2020, 5, 18, 15, 1, 53, 940564, tzinfo=timezone) - trigger = OrTrigger([DateTrigger(date1), DateTrigger(date2)], 1) + trigger = OrTrigger([DateTrigger(date1), DateTrigger(date2)], cooldown_period=1, max_iterations=10000) print(repr(trigger)) assert repr(trigger) == ( "OrTrigger([DateTrigger('2020-05-16 14:17:30.254212+02:00'), " - "DateTrigger('2020-05-18 15:01:53.940564+02:00')], cooldown_period=1.0)" + "DateTrigger('2020-05-18 15:01:53.940564+02:00')], cooldown_period=1.0, max_iterations=10000)" ) From cfb82bc6a129491cfcd5064635c837145b7358b7 Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 01:00:17 +0100 Subject: [PATCH 4/6] Closes #453. Also, just remembered to use pre-commit (-: --- tests/triggers/test_combining.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/triggers/test_combining.py b/tests/triggers/test_combining.py index 7ff44f90..74cb01c1 100644 --- a/tests/triggers/test_combining.py +++ b/tests/triggers/test_combining.py @@ -216,7 +216,9 @@ def test_cooldown_period(self, timezone, serializer): assert trigger.next() == start_time + timedelta(seconds=4) assert trigger.next() == start_time + timedelta(seconds=6) assert trigger.next() == start_time + timedelta(seconds=8) - assert trigger.next() == start_time + timedelta(seconds=12) # No second trigger -> cooldown + assert trigger.next() == start_time + timedelta( + seconds=12 + ) # No second trigger -> cooldown assert trigger.next() == start_time + timedelta(seconds=16) assert trigger.next() == start_time + timedelta(seconds=18) @@ -241,7 +243,11 @@ def test_max_iterations(self, timezone, serializer): def test_repr(self, timezone): date1 = datetime(2020, 5, 16, 14, 17, 30, 254212, tzinfo=timezone) date2 = datetime(2020, 5, 18, 15, 1, 53, 940564, tzinfo=timezone) - trigger = OrTrigger([DateTrigger(date1), DateTrigger(date2)], cooldown_period=1, max_iterations=10000) + trigger = OrTrigger( + [DateTrigger(date1), DateTrigger(date2)], + cooldown_period=1, + max_iterations=10000, + ) print(repr(trigger)) assert repr(trigger) == ( "OrTrigger([DateTrigger('2020-05-16 14:17:30.254212+02:00'), " From 9b423b36dcf6f1b9a09ba687f124cd3f5930d2be Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 01:10:34 +0100 Subject: [PATCH 5/6] Added changelog entry --- docs/versionhistory.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/versionhistory.rst b/docs/versionhistory.rst index 9614762c..697db23e 100644 --- a/docs/versionhistory.rst +++ b/docs/versionhistory.rst @@ -62,6 +62,8 @@ APScheduler, see the :doc:`migration section `. acquire the same schedules at once - Changed ``SQLAlchemyDataStore`` to automatically create the explicitly specified schema if it's missing (PR by @zhu0629) +- Added cooldown_period to OrTrigger + (#453 _; PR by @HomerusJa) **4.0.0a5** From 5f504e48978c75eb952f5dbb49e624d04d87c93c Mon Sep 17 00:00:00 2001 From: HomerusJa Date: Sat, 21 Dec 2024 02:49:59 +0100 Subject: [PATCH 6/6] Updated the docstrings to comply with the other ones --- src/apscheduler/triggers/combining.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/apscheduler/triggers/combining.py b/src/apscheduler/triggers/combining.py index 2233fa0e..766c8367 100644 --- a/src/apscheduler/triggers/combining.py +++ b/src/apscheduler/triggers/combining.py @@ -121,6 +121,8 @@ class OrTrigger(BaseCombiningTrigger): fire times. :param triggers: triggers to combine + :param cooldown_period: minimum time between two consecutive fires (in seconds, or as + timedelta) """ cooldown_period: timedelta = attrs.field(converter=as_timedelta, default=0) @@ -131,13 +133,10 @@ def _get_next_valid_fire_time(self) -> tuple[datetime | None, list[int]]: """ Find the next valid fire time that respects the cooldown period. - Raises: - MaxIterationsReached: If the maximum number of iterations is reached - - Returns: - A tuple of (fire_time, trigger_indices) where fire_time is the next valid - fire time (or None if no valid time exists) and trigger_indices is a list - of indices of triggers that produced this fire time. + :raises MaxIterationsReached: If the maximum number of iterations is reached + :returns: A tuple of (fire_time, trigger_indices) where fire_time is the next valid + fire time (or None if no valid time exists) and trigger_indices is a list + of indices of triggers that produced this fire time. """ for _ in range(self.max_iterations): earliest_time = min( @@ -174,12 +173,6 @@ def _get_next_valid_fire_time(self) -> tuple[datetime | None, list[int]]: raise MaxIterationsReached def next(self) -> datetime | None: - """ - Get the next fire time that respects the cooldown period. - - Returns: - The next valid fire time, or None if no more fire times exist. - """ # Initialize fire times if needed if not self._next_fire_times: self._next_fire_times = [t.next() for t in self.triggers]