-
Notifications
You must be signed in to change notification settings - Fork 470
fix: handle timezone correctly when DJANGO_CELERY_BEAT_TZ_AWARE=False (Issue #924) #926
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
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 tests are failing, so the change proposed appears to be not right. what do you think?
Dear Copilot Team,
Thank you for your thorough review and constructive feedback on this PR—we truly appreciate your expertise in helping refine the timezone handling logic. Below, we address your two key points to clarify the rationale and ensure alignment with Django’s behavior.
1. Use of
"timezone.now()" when
"DJANGO_CELERY_BEAT_TZ_AWARE=False"
We understand your concern about
"timezone.now()" returning a timezone-aware datetime when
"USE_TZ=True", even if
"DJANGO_CELERY_BEAT_TZ_AWARE=False". However, this choice is intentional to maintain consistency with Django’s database timezone handling.
When
"USE_TZ=True" (Django’s default), Django stores and retrieves datetimes in UTC from the database. The
"timezone.now()" method explicitly returns a timezone-aware datetime in UTC under this condition, ensuring alignment with the database’s UTC-based timestamps. Even when
"DJANGO_CELERY_BEAT_TZ_AWARE=False" (which skips forcing certain datetimes to be timezone-aware), using
"timezone.now()" guarantees that the generated
"now" value matches the UTC timezone of database-stored datetimes. This prevents mismatches during critical comparisons (e.g., checking if a task is due), which rely on consistent timezone contexts.
In summary,
"timezone.now()" is the correct choice here because it synchronizes with Django’s UTC-based database operations, regardless of the
"DJANGO_CELERY_BEAT_TZ_AWARE" setting.
2. Removing Timezone Info with
".replace(tzinfo=None)"
We appreciate your caution about potential ambiguity when stripping timezone awareness. In this specific scenario, however, the approach is safe and necessary due to Django’s behavior when
"USE_TZ=False".
When
"DJANGO_CELERY_BEAT_TZ_AWARE=True" but
"USE_TZ=False", Django does not auto-convert datetimes to UTC for storage. Instead, it stores them as naive datetimes in the local timezone (configured via
"TIME_ZONE"). To generate a naive datetime that matches this stored format, we first anchor the datetime to
"self.app.timezone" (the explicitly configured timezone) using
"datetime.datetime.now(self.app.timezone)", then strip the timezone info with
".replace(tzinfo=None)".
For example, if
"self.app.timezone" is
"Asia/Shanghai" (UTC+8),
"datetime.datetime.now(self.app.timezone)" produces a timezone-aware datetime like
"2025-07-31 14:30:00+08:00". Stripping the timezone with
".replace(tzinfo=None)" converts this to a naive datetime
"2025-07-31 14:30:00", which aligns with how Django stores datetimes when
"USE_TZ=False".
Using
"datetime.datetime.now()" (without a timezone parameter) would instead return a naive datetime in the system’s local time, which may not match Django’s configured
"TIME_ZONE" when
"USE_TZ=False". Thus, grounding the datetime to
"self.app.timezone" before stripping the timezone ensures accuracy.
We hope this clarifies the logic behind these changes. We remain committed to robust timezone handling and welcome further feedback to strengthen the fix. Please feel free to reach out with additional questions—we’re happy to discuss further.
…---Original---
From: ***@***.***>
Date: Thu, Jul 31, 2025 23:04 PM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [celery/django-celery-beat] fix: handle timezone correctly whenDJANGO_CELERY_BEAT_TZ_AWARE=False (Issue #924) (PR #926)
@Copilot commented on this pull request.
Pull Request Overview
This PR fixes timezone handling issues in django-celery-beat when DJANGO_CELERY_BEAT_TZ_AWARE=False by ensuring consistent datetime comparisons and properly handling all combinations of timezone settings.
Rewrote _default_now() method to handle timezone awareness logic hierarchy more clearly
Replaced maybe_make_aware with explicit make_aware conversion for better timezone handling
Updated test cases to use timezone.now() instead of datetime.utcnow() for consistency
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File Description
django_celery_beat/schedulers.py Core timezone handling logic updates in is_due() and _default_now() methods
t/unit/test_schedulers.py Updated test cases to use Django's timezone.now() instead of datetime.utcnow()
In django_celery_beat/schedulers.py:
> + # Use the default time zone time of Django + now = timezone.now()
When DJANGO_CELERY_BEAT_TZ_AWARE=False, using timezone.now() will return a timezone-aware datetime if USE_TZ=True, which contradicts the intent of the setting. This should return a naive datetime to be consistent with the setting's purpose.
⬇️ Suggested change - # Use the default time zone time of Django - now = timezone.now() + # Use the default time zone time of Django and convert to naive datetime + now = timezone.now().replace(tzinfo=None)
In django_celery_beat/schedulers.py:
> + # Remove the time zone information (convert to naive datetime) + now = datetime.datetime.now(self.app.timezone).replace(tzinfo=None)
Converting a timezone-aware datetime to naive by removing tzinfo can lead to ambiguous results. Consider using datetime.datetime.now() without timezone parameter to get a truly naive datetime in the local timezone.
⬇️ Suggested change - # Remove the time zone information (convert to naive datetime) - now = datetime.datetime.now(self.app.timezone).replace(tzinfo=None) + # Get a naive datetime object in the local timezone + now = datetime.datetime.now()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
…t with the CELERY_TIMEZONE time zone. (Issue celery#924)
…Y_TIMEZONE. (Issue celery#924)
Fix: The issue where the local time obtained does not match the CELERY_TIMEZONE. The previous fix version, DJANGO_CELERY_BEAT_TZ_AWARE = False, required that the local time should be consistent with the CELERY_TIMEZONE timezone. DJANGO_CELERY_BEAT_TZ_AWARE = False At this point, the local time zone of the system is obtained, and then it is converted to the timezone of self.app.timezone using astimezone to ensure consistency with the Celery timezone. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #926 +/- ##
==========================================
+ Coverage 88.19% 88.30% +0.11%
==========================================
Files 32 32
Lines 1008 1009 +1
Branches 105 105
==========================================
+ Hits 889 891 +2
Misses 101 101
+ Partials 18 17 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR fixes timezone handling issues when DJANGO_CELERY_BEAT_TZ_AWARE=False
by improving datetime comparison logic and ensuring consistent timezone behavior across different configuration combinations.
- Rewrote
_default_now()
method to properly handle timezone awareness based on bothDJANGO_CELERY_BEAT_TZ_AWARE
andUSE_TZ
settings - Replaced
maybe_make_aware
with explicit timezone handling logic inis_due()
method - Updated test cases to use more realistic datetime generation that matches the new implementation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
django_celery_beat/schedulers.py |
Core timezone handling logic improvements in _default_now() and is_due() methods |
t/unit/test_schedulers.py |
Updated test datetime generation to align with new timezone-aware implementation |
django_celery_beat/schedulers.py
Outdated
if (getattr(settings, 'DJANGO_CELERY_BEAT_TZ_AWARE', True) and | ||
getattr(settings, 'USE_TZ', True)): |
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.
[nitpick] The condition logic is unclear. Consider extracting the timezone awareness check into a separate variable or method to improve readability and make the intent more explicit.
if (getattr(settings, 'DJANGO_CELERY_BEAT_TZ_AWARE', True) and | |
getattr(settings, 'USE_TZ', True)): | |
tz_aware = ( | |
getattr(settings, 'DJANGO_CELERY_BEAT_TZ_AWARE', True) and | |
getattr(settings, 'USE_TZ', True) | |
) | |
if tz_aware: |
Copilot uses AI. Check for mistakes.
django_celery_beat/schedulers.py
Outdated
getattr(settings, 'USE_TZ', True)): | ||
return now | ||
|
||
# A naive datetime representing local time in the app's timezone |
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.
[nitpick] The comment should be more specific about when this path is taken. Consider adding details about the configuration conditions that lead to returning a naive datetime.
# A naive datetime representing local time in the app's timezone | |
# Return a naive datetime representing local time in the app's timezone. | |
# This path is taken when either DJANGO_CELERY_BEAT_TZ_AWARE or USE_TZ | |
# is set to False in Django settings. |
Copilot uses AI. Check for mistakes.
I will need more eyes and bit more time before reaching to a consensus for this |
Timezone Handling Fix for django-celery-beat (Issue #924)
Description
This PR resolves timezone compatibility issues when
DJANGO_CELERY_BEAT_TZ_AWARE=False
by:is_due()
DJANGO_CELERY_BEAT_TZ_AWARE
USE_TZ
Fixes #924
Technical Changes
1. Core Modifications
schedulers.py
_default_now()
to handle timezone awareness logic hierarchymaybe_make_aware
with explicitmake_aware
conversion📌 Core Changes
schedulers.py
_default_now()
to handle timezone awareness logic hierarchymaybe_make_aware
with explicitmake_aware
conversion