Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 tasks done
bmeares opened this issue May 13, 2024 · 7 comments · Fixed by #914
Closed
3 tasks done

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

bmeares opened this issue May 13, 2024 · 7 comments · Fixed by #914
Labels

Comments

@bmeares
Copy link
Contributor

bmeares commented May 13, 2024

Things to check first

  • I have checked that my issue does not already have a solution in the FAQ

  • I have searched the existing issues and didn't find my bug already reported there

  • 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)
@bmeares bmeares added the bug label May 13, 2024
@agronholm
Copy link
Owner

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
Copy link
Contributor Author

bmeares commented May 13, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

bmeares commented May 14, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

bmeares commented May 14, 2024

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
Copy link
Contributor Author

bmeares commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants