[v4.0.0a4] Combining daily and weekly triggers increments by 14 days

bmeares commented 1 month ago

Things to check first

[X] I have checked that my issue does not already have a solution in the FAQ
[X] I have searched the existing issues and didn't find my bug already reported there
[X] I have checked that my bug is still present in the latest release

Version

v4.0.0a4

What happened?

Combing daily and weekly triggers (or days=7) skips every other interval, firing every 14 days. The same behavior is observed with different timezones and threshold values.

How can we reproduce the bug?

from datetime import datetime, timezone
from apscheduler.triggers.combining import AndTrigger
from apscheduler.triggers.interval import IntervalTrigger

start_dt = datetime(2024, 5, 1, tzinfo=timezone.utc)
daily_trigger = IntervalTrigger(days=1, start_time=start_dt)
weekly_trigger = IntervalTrigger(weeks=1, start_time=start_dt)
and_trigger = AndTrigger([daily_trigger, weekly_trigger])

and_trigger.next()
# datetime.datetime(2024, 5, 1, 0, 0, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 15, 0, 0, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 29, 0, 0, tzinfo=datetime.timezone.utc)

agronholm commented 1 month ago

What are you actually trying to achieve here? For the record, I think AndTrigger was a misfeature and I'm considering removing it altogether.

bmeares commented 1 month ago

Hi @agronholm , thank you for the quick reply (and for all your incredible work on APScheduler)! I'm very fond of the AndTrigger, but I understand if you decide to remove it 😞

For context, I'm writing a simple parser to replicate some of the combination logic of the deprecated library Rocketry. One failing test case is daily & weekly which I've written about above.

If you'd like, I'll open a PR later this week to address some of the minor bugs I've come across. Again, thank you for your amazing work!

agronholm commented 1 month ago

Thanks! But to delve further into this particular case, I'm not convinced that AndTrigger is what you want. What it does is that it only triggers at datetimes that all the embedded triggers agree on. So, would you mind elaborating on what the intended effect is in your configuration?

bmeares commented 1 month ago

The intended effect is the same schedule as IntervalTrigger(weeks=1) since that's when the daily and weekly schedules overlap.

After a bit of testing, a quick workaround is to divide one of the days by 2 when combined with another schedule:

from datetime import datetime, timezone
from apscheduler.triggers.combining import AndTrigger
from apscheduler.triggers.interval import IntervalTrigger

start_dt = datetime(2024, 5, 1, tzinfo=timezone.utc)

# Intended schedule of every 15 days by combining 3 and 5 (divided by 2)
every_3_days = IntervalTrigger(days=3, start_time=start_dt)
every_5_days = IntervalTrigger(days=5/2, start_time=start_dt)
and_trigger = AndTrigger([every_3_days, every_5_days])

and_trigger.next()
# datetime.datetime(2024, 5, 1, 0, 0, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 16, 0, 0, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 31, 0, 0, tzinfo=datetime.timezone.utc)

However this workaround creates issues when combining with a CronTrigger: Edit: On second thought, this cron case should raise MaxIterations because every 5 days starting on a Monday will never align with a CronTrigger of Monday through Friday.

from datetime import datetime, timezone
from apscheduler.triggers.combining import AndTrigger
from apscheduler.triggers.interval import IntervalTrigger
from apscheduler.triggers.cron import CronTrigger

start_dt = datetime(2024, 5, 13, tzinfo=timezone.utc) # Monday

weekdays = CronTrigger(
    day_of_week = 'mon-fri',
    hour = '*',
    minute = 0,
    second = 0,
    start_time = start_dt,
    timezone = start_dt.tzinfo,
)
every_5_days = IntervalTrigger(days=5/2, start_time=start_dt)
and_trigger = AndTrigger([weekdays, every_5_days])

and_trigger.next()
# datetime.datetime(2024, 5, 13, 0, 0, tzinfo=zoneinfo.ZoneInfo(key='UTC'))
and_trigger.next()
# datetime.datetime(2024, 5, 20, 12, 0, tzinfo=zoneinfo.ZoneInfo(key='UTC'))
and_trigger.next()
# datetime.datetime(2024, 5, 28, 0, 0, tzinfo=zoneinfo.ZoneInfo(key='UTC'))

The expected behavior is observed when using a unit like minutes:

from datetime import datetime, timezone
from apscheduler.triggers.combining import AndTrigger
from apscheduler.triggers.interval import IntervalTrigger

start_dt = datetime(2024, 5, 1, tzinfo=timezone.utc)

# Intended schedule of every 15 minutes by combining 3 and 5
every_3_minutes = IntervalTrigger(minutes=3, start_time=start_dt)
every_5_minutes = IntervalTrigger(minutes=5, start_time=start_dt)
and_trigger = AndTrigger([every_3_minutes, every_5_minutes])

and_trigger.next()
# datetime.datetime(2024, 5, 1, 0, 0, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 1, 0, 15, tzinfo=datetime.timezone.utc)
and_trigger.next()
# datetime.datetime(2024, 5, 1, 0, 30, tzinfo=datetime.timezone.utc)

Testing

In this testing script, I'm experimenting with various combinations of triggers. See parse_schedule() for the logic where the triggers are constructed. Here's an example of how the function is invoked:

from meerschaum.utils.schedule import parse_schedule
trigger = parse_schedule('every 6 hours and mon-fri starting 00:30 EDT')
trigger
# AndTrigger([IntervalTrigger(hours=3.0, start_time='2024-05-14 00:30:00-04:00'), CronTrigger(year='*', month='*', day='*', week='*', day_of_week='mon-fri', hour='*', minute='30', second='0', start_time='2024-05-14T00:30:00-04:00', timezone='tzlocal()')], threshold=0.0, max_iterations=1000000)
trigger.next()
# datetime.datetime(2024, 5, 14, 0, 30, tzinfo=tzlocal())
trigger.next()
# datetime.datetime(2024, 5, 14, 6, 30, tzinfo=tzlocal())

This schedule module is available in meerschaum==2.2.0.dev3 in case you want to run the above snippet. In the morning I'll dive into APScheduler to see if I can put together a PR.

agronholm commented 1 month ago

This makes absolutely no sense to me. Say you want something to be scheduled every 15 minutes, why would you want to complicate it by using AndTrigger instead of just IntervalTrigger(minutes=15)? As for the "weekly" triggering, maybe CalendarIntervalTrigger or CronTrigger would work better? I just don't see a use case here for AndTrigger.

bmeares commented 1 month ago

I agree that these examples can be achieved with one trigger instead of combining two — these are simple cases to demonstrate the underlying behavior.

The pattern I've noticed is combing a daily interval trigger with another trigger (interval or cron) requires one of the days to be divided by 2. There's a similar pattern when combining an hourly trigger with another trigger, though all of the hours need to be divided by 2 (e.g. the above example every 6 hours and mon-fri requires IntervalTrigger(hours=3) joined with CronTrigger(day_of_week='mon-fri')).

I hope this clears it up! I'm just trying to write a robust parser to handle every case, even the silly ones.

bmeares commented 1 month ago

Removing the second construction of _self._next_fire_times in AndTrigger.next() seems to fix the issue. I'm doing some more testing to ensure this is the fix and that it doesn't interfere with the working cases.

agronholm / apscheduler