-
-
Notifications
You must be signed in to change notification settings - Fork 78
added timezone support for cron schedules in periodic tasks #1474
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
ewjoachim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far, we're missing tests. Also, I'd prefer we reach a consensus on the issue before merging the PR, as we may decide on a slightly different API to mitigate the risks.
| logger.error(f"{tzinfo} is not a valid timezone.") | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A failure could be impactful, I think we need to raise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue I can see is that it might confuse users when their chosen timezone isn’t applied. Besides that, does it have any other impact?
And if we’re okay with it crashing, wouldn’t it be simpler not to catch the error in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue I can see is that it might confuse users when their chosen timezone isn’t applied. Besides that, does it have any other impact?
If I have scheduled batch processing at 12AM while there's no activity, and it happens at 2PM during peak hours, it could easily cause disruption.
| cron: str, | ||
| periodic_id: str, | ||
| configure_kwargs: tasks.ConfigureTaskOptions, | ||
| tzinfo: str | None | ZoneInfo = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Please put None last in the types)
| cron: str | ||
| periodic_id: str | ||
| configure_kwargs: tasks.ConfigureTaskOptions | ||
| tzinfo: str | None | ZoneInfo = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather we convert str to ZoneInfo at constructor time, and we only ever store ZoneInfo (or None) on the object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning we shouldn't accept a ZoneInfo object?
Other than being simpler for the user, do you have any other specific reason for restricting it to a string value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no, I meant: whether we recieve a str or a ZoneInfo, what we store is a ZoneInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused. Okay.
| return croniter.croniter(self.cron) | ||
| croniter_instance = croniter.croniter(self.cron) | ||
| # croniter sets the timezone info object in | ||
| croniter_instance.tzinfo = self.get_tzinfo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that the public API of croniter ? I couldn't find the official doc, so it's hard to say.
Also, I believe, support of ZoneInfo is recent, which means we need to severly restrict the accepted versions in pyproject.toml (which might create some conflict, but the lib didn't move a lot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise the comment was incomplete. It's not the public API, but at least it's not a private attribute. They set tzinfo attribute in the initialiser of the croniter instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean by the ZoneInfo support being recent? As I believe it's supported in v6.0.0 (from December, 2024)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will see if I run into any issues once I write tests.
| Timezone in which the cron expression should be interpreted. Accepts a | ||
| timezone name string (e.g., "Africa/Blantyre"), a `zoneinfo.ZoneInfo` instance, | ||
| or `None`. When `None` (the default), the underlying `croniter` library | ||
| will interpret the schedule in UTC (the current behaviour). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mention (the current behaviour) makes sense as a PR comment because we're talking code evolution, but doesn't make sense in the docstring. In 2 years time, what will "current" refer to ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. It's indeed confusing.
Closes #1472
This PR adds native timezone support to Procrastinate’s periodic task scheduler.
The
Blueprint.periodicdecorator now accepts atzinfoargument, which can be either a string or azoneinfo.ZoneInfoobject. When provided, croniter will evaluate the cron expression in the specified timezone rather than defaulting to UTC.Previously, users had to manually convert local times to UTC to ensure tasks ran at the intended hours. This often led to confusion and misaligned schedules. With this enhancement, users can define periodic tasks directly in their local timezone, improving predictability and reducing the risk of scheduling errors.
Key Changes
tzinfoattribute is now set on thecroniterinstance withinPeriodicTask.Example Usage
Without
tzinfo: runs from 10 AM to 7 PM UTCWith
tzinfo: runs from 8 AM to 5 PM local timeSuccessful PR Checklist:
PR label(s):