Skip to content

Commit

Permalink
Fix "Last day of the month" schedules in dagster (#25169)
Browse files Browse the repository at this point in the history
## Summary & Motivation
The is_numeric check here was incorrectly assumimg that if it wasn't
"*", it would be a number (and also assuming that it would still be a
string even if it was numeric, which was more harmless). But you can
have "L" in a cronstring to indicate the last day of the month.

## How I Tested These Changes
BK, load UI with one of these schedules and turn it on

## Changelog

- [x] `BUGFIX` Fixed an issue where schedules with "L" in the cronstring
(to indicate the last day of the month) would raise an error in the
Dagster UI.
  • Loading branch information
gibsondan authored and Neil Fulwiler committed Oct 10, 2024
1 parent 7540fdb commit 7b512a2
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 6 deletions.
12 changes: 6 additions & 6 deletions python_modules/dagster/dagster/_utils/schedules.py
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ def cron_string_iterator(
# Croniter >= 1.4 returns 3 items
cron_parts, nth_weekday_of_month, *_ = CroniterShim.expand(cron_string)

is_numeric = [len(part) == 1 and part[0] != "*" for part in cron_parts]
is_numeric = [len(part) == 1 and isinstance(part[0], int) for part in cron_parts]
is_wildcard = [len(part) == 1 and part[0] == "*" for part in cron_parts]

all_numeric_minutes = len(cron_parts[0]) > 0 and all(
Expand All @@ -657,7 +657,7 @@ def cron_string_iterator(
if (
all(is_numeric[0:3])
and all(is_wildcard[3:])
and int(cron_parts[2][0]) <= MAX_DAY_OF_MONTH_WITH_GUARANTEED_MONTHLY_INTERVAL
and cron_parts[2][0] <= MAX_DAY_OF_MONTH_WITH_GUARANTEED_MONTHLY_INTERVAL
): # monthly
known_schedule_type = ScheduleType.MONTHLY
elif all(is_numeric[0:2]) and is_numeric[4] and all(is_wildcard[2:4]): # weekly
Expand All @@ -668,16 +668,16 @@ def cron_string_iterator(
known_schedule_type = ScheduleType.HOURLY

if is_numeric[1]:
expected_hour = int(cron_parts[1][0])
expected_hour = cron_parts[1][0]

if all_numeric_minutes:
expected_minutes = [int(cron_part) for cron_part in cron_parts[0]]
expected_minutes = [cron_part for cron_part in cron_parts[0]]

if is_numeric[2]:
expected_day = int(cron_parts[2][0])
expected_day = cron_parts[2][0]

if is_numeric[4]:
expected_day_of_week = int(cron_parts[4][0])
expected_day_of_week = cron_parts[4][0]

if known_schedule_type:
start_datetime = datetime.datetime.fromtimestamp(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -582,3 +582,38 @@ def test_reversed_dst_transition_advances(execution_timezone, cron_string, times
prev_time = next_time

start_timestamp = start_timestamp - timestamp_interval


def test_last_day_of_month_cron_schedule():
# L means last day of month
execution_timezone = "Europe/Berlin"
cron_string = "*/15 13 L * *"

expected_datetimes = [
create_datetime(2023, 10, 31, 13, 0, 0, tz="Europe/Berlin"),
create_datetime(2023, 10, 31, 13, 15, 0, tz="Europe/Berlin"),
create_datetime(2023, 10, 31, 13, 30, 0, tz="Europe/Berlin"),
create_datetime(2023, 10, 31, 13, 45, 0, tz="Europe/Berlin"),
create_datetime(2023, 11, 30, 13, 0, 0, tz="Europe/Berlin"),
create_datetime(2023, 11, 30, 13, 15, 0, tz="Europe/Berlin"),
create_datetime(2023, 11, 30, 13, 30, 0, tz="Europe/Berlin"),
create_datetime(2023, 11, 30, 13, 45, 0, tz="Europe/Berlin"),
create_datetime(2023, 12, 31, 13, 0, 0, tz="Europe/Berlin"),
create_datetime(2023, 12, 31, 13, 15, 0, tz="Europe/Berlin"),
create_datetime(2023, 12, 31, 13, 30, 0, tz="Europe/Berlin"),
create_datetime(2023, 12, 31, 13, 45, 0, tz="Europe/Berlin"),
]

start_timestamp = expected_datetimes[0].timestamp() - 1

cron_iter = cron_string_iterator(start_timestamp, cron_string, execution_timezone)

for i in range(len(expected_datetimes)):
assert next(cron_iter) == expected_datetimes[i]

end_timestamp = expected_datetimes[-1].timestamp() + 1

cron_iter = reverse_cron_string_iterator(end_timestamp, cron_string, execution_timezone)

for i in range(len(expected_datetimes)):
assert next(cron_iter) == expected_datetimes[-(i + 1)]

0 comments on commit 7b512a2

Please sign in to comment.