-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import time | ||
| from collections.abc import Iterable | ||
| from typing import Callable, Generic, cast | ||
| from zoneinfo import ZoneInfo, ZoneInfoNotFoundError | ||
|
|
||
| import attr | ||
| import croniter | ||
|
|
@@ -35,10 +36,26 @@ class PeriodicTask(Generic[P, R, Args]): | |
| cron: str | ||
| periodic_id: str | ||
| configure_kwargs: tasks.ConfigureTaskOptions | ||
| tzinfo: str | None | ZoneInfo = None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was confused. Okay. |
||
|
|
||
| @cached_property | ||
| def croniter(self) -> croniter.croniter: | ||
| return croniter.croniter(self.cron) | ||
| croniter_instance = croniter.croniter(self.cron) | ||
| # croniter sets the timezone info object in | ||
| croniter_instance.tzinfo = self.get_tzinfo() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| return croniter_instance | ||
|
|
||
| def get_tzinfo(self): | ||
| tzinfo = self.tzinfo | ||
| if isinstance(tzinfo, str): | ||
| try: | ||
| return ZoneInfo(tzinfo) | ||
| except (ZoneInfoNotFoundError, ValueError): | ||
| logger.error(f"{tzinfo} is not a valid timezone.") | ||
| return None | ||
|
Comment on lines
+54
to
+55
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A failure could be impactful, I think we need to raise.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
| if isinstance(tzinfo, ZoneInfo): | ||
| return tzinfo | ||
| return None | ||
|
|
||
|
|
||
| TaskAtTime = tuple[PeriodicTask, int] | ||
|
|
@@ -52,6 +69,7 @@ def periodic_decorator( | |
| self, | ||
| cron: str, | ||
| periodic_id: str, | ||
| tzinfo: str | None | ZoneInfo = None, | ||
| **configure_kwargs: Unpack[tasks.ConfigureTaskOptions], | ||
| ) -> Callable[[tasks.Task[P, R, Concatenate[int, Args]]], tasks.Task[P, R, Args]]: | ||
| """ | ||
|
|
@@ -68,6 +86,7 @@ def wrapper( | |
| cron=cron, | ||
| periodic_id=periodic_id, | ||
| configure_kwargs=configure_kwargs, | ||
| tzinfo=tzinfo, | ||
| ) | ||
| return cast(tasks.Task[P, R, Args], task) | ||
|
|
||
|
|
@@ -79,6 +98,7 @@ def register_task( | |
| cron: str, | ||
| periodic_id: str, | ||
| configure_kwargs: tasks.ConfigureTaskOptions, | ||
| tzinfo: str | None | ZoneInfo = None, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Please put None last in the types) |
||
| ) -> PeriodicTask[P, R, Concatenate[int, Args]]: | ||
| key = (task.name, periodic_id) | ||
| if key in self.periodic_tasks: | ||
|
|
@@ -99,12 +119,12 @@ def register_task( | |
| "kwargs": str(configure_kwargs), | ||
| }, | ||
| ) | ||
|
|
||
| self.periodic_tasks[key] = periodic_task = PeriodicTask( | ||
| task=task, | ||
| cron=cron, | ||
| periodic_id=periodic_id, | ||
| configure_kwargs=configure_kwargs, | ||
| tzinfo=tzinfo, | ||
| ) | ||
| return periodic_task | ||
|
|
||
|
|
||
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.