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

Fix Issue #604: Celery Beat Crashing at the End of Daylight Savings #605

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

polarmt
Copy link

@polarmt polarmt commented Nov 11, 2022

Dependencies

celery/celery#7901

Note: The two PRs are codependent and need to be merged together.

Overview

This is my suggested fix for #604. The following issue assumes America/Los_Angeles.

Testing

All testing was done on the following versions:
django: 3.2.15
celery: 5.2.7
django-celery-beat: 2.4.0

USE_TZ is not set.

Reproducing the Issue

In the Django shell (python manage.py shell):

from datetime import datetime, timedelta
from celery.utils import time
from django_celery_beat.utils import make_aware
from pytz import timezone

dt  = datetime(2022, 11, 6, 1, 34, 0)
now = make_aware(dt)

Without the changes in this PR, this should throw an AmbiguousTimeError.

Behavior of remaining During Transition (Without celery/celery#7901)

We can run the following script to test how the Celery beat will calculate the time remaining during the transition:

from datetime import datetime, timedelta
from celery.utils import time
from django_celery_beat.utils import make_aware
from pytz import timezone

dt  = datetime(2022, 11, 6, 1, 34, 0)
now = make_aware(dt)

tz = timezone('America/Los_Angeles')
start = datetime(2022, 11, 6, 1, 15, 0)
start = tz.localize(start, is_dst=True)
ends_in = timedelta(minutes=80)

print(time.remaining(time.maybe_make_aware(start), ends_in, time.maybe_make_aware(now)))

now here would be 1:34 AM PST or 1:34:00-8:00 while start would be 1:15 AM PDT or 1:15:00-7:00. The correct return value of time.remaining(...) here would be 1 minute since 1 hour 20 minutes from start would be 1:35:00-8:00. The problem is that time.remaining(...) returns 1 hour 1 minute because time.remaining will convert 1:15:00-7:00 to 1:15-8:00.

Behavior of remaining During Transition (With celery/celery#7901)

We can use the same script as the above subsection after making the changes in celery/celery#7901. This will cause time.remaining(...) to return 1 minute instead of 1 hour 1 minute, which is the correct value.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

beside it will be very useful to have proper unit tests for this change

value = timezone.make_aware(value, timezone.get_default_timezone())
tm_isdst = time.localtime().tm_isdst

# tm_isdst may return -1 if it cannot be determined
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this part, can any other way we can check it?

Copy link
Author

Choose a reason for hiding this comment

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

I referenced these posts 1 (top answer only) and 2. There are other suggested solutions online. I can summarize the problems with these solutions.

Use of localize

Source 1
Source 2
Source 3

The root cause was inside localize. Using the localize to fix the problem with localize sounds counterintuitive. For instance, link 1 mentions the following in the final solution:

Once you've **localized** a datetime in fact you can simply check it's .dst() method to see if it's zero.

time.daylight

Source

This solution might work, but the underlying code is just to do a similar check to what I do here. This code is cleaner, and I can change it if you would like.

Calculations

Source

One way is to calculate the timezone based on the current time and a previous time, but I think that this solution is long and too ad-hoc when we can achieve this with a few lines of code.

Conclusion

I think that time.daylight or time.localtime+tm_isdst is the best way to go for this. If you believe in the other solutions, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

I have to check to give you proper answer

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. Let me know when you've checked.

@polarmt
Copy link
Author

polarmt commented Nov 14, 2022

beside it will be very useful to have proper unit tests for this change

I added the unit tests.

@polarmt polarmt force-pushed the master branch 2 times, most recently from bc6f65c to b75637f Compare November 14, 2022 23:39
@cclauss
Copy link
Contributor

cclauss commented Dec 19, 2023

Now that tests are passing on Django v5 this all becomes more difficult because:

  • Support for pytz timezones is removed.
  • The is_dst argument is removed from:
    • QuerySet.datetimes()
    • django.utils.timezone.make_aware()
    • [ ... ]

https://docs.djangoproject.com/en/5.0/releases/5.0/#features-removed-in-5-0

@cclauss
Copy link
Contributor

cclauss commented Jan 16, 2024

@polarmt Please rebase to resolve the git conflicts.

@cclauss
Copy link
Contributor

cclauss commented Mar 4, 2024

@polarmt Tests are ~50/50 pass/fail and in Django v5 the default value of the USE_TZ setting is changed from False to True.

Now that we have release v2.6.0 that supports Django v5, how should we proceed with this effort?

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