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

pass day_or croniter argument to CronClock & CronSchedule #3612

Merged
merged 15 commits into from
Nov 6, 2020

Conversation

sp1thas
Copy link

@sp1thas sp1thas commented Nov 3, 2020

Summary

The purpose of this PR is to allow to day_or croniter on CronClock and CronSchedule instantiation.

Changes

This PR changes the CronClockSchema and CronScheduleSchema by adding the new day_or field. day_or has been added to CronClock.__init__ and CronSchedule. Some tests for have also been added.

Importance

This change makes every cron clock more configurable, therefore, the whole procedure of scheduling can become more flexible.

This PR was inspired by the following case:

Let's suppose that we have a scheduler with multiple clocks. We want to add a clock that creates events every first Thursday of the month, this periodicity can be achived by this cron: 0 0 1-7 * 4.

Unfortunately, this cron will not work out of the box:

>>> import datetime
>>> import croniter
>>> cron = croniter.croniter('0 0 1-7 * 4')
>>> cron.get_next(datetime.datetime)
datetime.datetime(2020, 11, 1, 0, 0)
>>> cron.get_next(datetime.datetime)
datetime.datetime(2020, 11, 2, 0, 0)
>>> cron.get_next(datetime.datetime)
datetime.datetime(2020, 11, 3, 0, 0)

because of legacy/problematic behavior: so link.

To fix this issue, day_or=False should be pass on initialization.

>>> cron = croniter.croniter('0 0 1-7 * 4', day_or=False)
>>> cron.get_next(datetime.datetime)
datetime.datetime(2020, 11, 5, 0, 0)
>>> cron.get_next(datetime.datetime)
datetime.datetime(2020, 12, 3, 0, 0)

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Copy link

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sp1thas. Looking at the croniter docs, it looks like the only kwarg to croniter that it makes sense for us to expose is day_or - perhaps adding that as a top-level kwarg to CronClock/CronSchedule would make more sense than exposing all of cron-iter's kwargs?

Other than that, this seems fine to me. cc'ing @cicdw for a 2nd glance-over just to be sure (schedules also need to be supported by cloud, so there's some extra nuance here when making changes to them).

@cicdw
Copy link
Member

cicdw commented Nov 4, 2020

Yup this can be supportable by Cloud (once the serializer changes have been released) with no additional work needed as all of the work is handled by the (de)serialization of the schedule / clock itself.

I think I agree with you @jcrist that day_or might be better than general kwargs; if they ever introduce new kwargs it could cause confusion between the version of croniter running in Cloud vs locally so an explicit list seems better (and allows people to discover the functionality easier).

@sp1thas sp1thas changed the title CronClock & CronSchedule: Pass extra croniter kwargs pass day_or croniter argument to CronClock & CronSchedule Nov 5, 2020
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #3612 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

src/prefect/schedules/clocks.py Outdated Show resolved Hide resolved
src/prefect/schedules/clocks.py Outdated Show resolved Hide resolved
src/prefect/schedules/schedules.py Outdated Show resolved Hide resolved
src/prefect/schedules/clocks.py Outdated Show resolved Hide resolved
tests/schedules/test_clocks.py Show resolved Hide resolved
sp1thas and others added 9 commits November 5, 2020 17:46
Co-authored-by: Jim Crist-Harif <jcrist@users.noreply.github.com>
Co-authored-by: Jim Crist-Harif <jcrist@users.noreply.github.com>
Co-authored-by: Jim Crist-Harif <jcrist@users.noreply.github.com>
Co-authored-by: Jim Crist-Harif <jcrist@users.noreply.github.com>
Copy link

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me.

@jcrist jcrist merged commit 528db93 into PrefectHQ:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants