Skip to content

Celery.timezone: try tzlocal.get_localzone() before using LocalTimezone #9862

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.

Sign up for GitHub

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

Jump to bottom
Merged
auvipy merged 3 commits into from
Sep 20, 2025
Merged

Celery.timezone: try tzlocal.get_localzone() before using LocalTimezone #9862

auvipy merged 3 commits into from
Sep 20, 2025

Conversation

@woutdenolf
Copy link
Contributor

@woutdenolf woutdenolf commented Aug 13, 2025 •
edited
Loading

Closes #9861

Use tzlocal.get_localzone() and fall back to LocalTimezone() for the local system timezone.

from tzlocal import get_localzone
from zoneinfo import ZoneInfo
from datetime import tzinfo

# Current LocalTimezone example
from celery.utils.time import LocalTimezone
local_tz = LocalTimezone()
print(local_tz)  # e.g., <LocalTimezone: UTC+02> (non-standard name)
assert isinstance(local_tz, tzinfo)

# Proposed tzlocal-based approach
local_tz = get_localzone()
print(local_tz)  # e.g., Europe/Brussels (IANA name)
assert isinstance(local_tz, tzinfo)
assert isinstance(local_tz, ZoneInfo)

@codecov
Copy link

codecov bot commented Aug 13, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.64%. Comparing base (6da3282) to head (122c0b1).
⚠️ Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
celery/utils/time.py 66.66% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9862      +/-   ##
==========================================
- Coverage   78.70%   78.64%   -0.06%     
==========================================
  Files         153      153              
  Lines       19258    19268      +10     
  Branches     2561     2562       +1     
==========================================
- Hits        15157    15154       -3     
- Misses       3803     3816      +13     
  Partials      298      298
Flag Coverage Δ
unittests 78.62% <66.66%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@woutdenolf woutdenolf force-pushed the improvement_issue_1351 branch 2 times, most recently from 16d4c6f to b39b3dd Compare August 13, 2025 20:42
Nusnus
Nusnus reviewed Aug 13, 2025
View reviewed changes
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

The integration tests are broken right now, so take note the CI will fail.

Fix/workaround in progress.

@auvipy auvipy self-requested a review August 14, 2025 03:13
@auvipy auvipy force-pushed the improvement_issue_1351 branch from fc92abe to dba2a2e Compare August 26, 2025 04:21
@auvipy auvipy added this to the 5.6.0 milestone Aug 31, 2025
auvipy
auvipy requested changes Aug 31, 2025
View reviewed changes
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.

we should add unit test coverage for the proposed changes

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Aug 31, 2025 •
edited
Loading

I'm not sure what kind of test I should add. I'm assuming there are already tests for the local timezone and they pass with the changes.

I could perhaps make a test that patches get_localzone to make it fail and verify the LocalTimezone fallback?

@auvipy
Copy link
Member

auvipy commented Aug 31, 2025

that would be fine.

@auvipy auvipy force-pushed the improvement_issue_1351 branch from dba2a2e to c99da5e Compare August 31, 2025 09:45
@woutdenolf
Copy link
Contributor Author

woutdenolf commented Aug 31, 2025 •
edited
Loading

These timezone tests were already there

  • t/unit/app/test_app.py::test_App::test_timezone_none_set
  • t/unit/app/test_app.py::test_App::test_uses_utc_timezone

I added two tests explicitly for the local timezone

  • t/unit/app/test_app.py::test_App::test_use_local_timezone
  • t/unit/app/test_app.py::test_App::test_use_local_timezone_failure

@auvipy auvipy requested a review from Copilot August 31, 2025 12:17
Copilot AI reviewed Aug 31, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a 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 replaces Celery's custom LocalTimezone implementation with the tzlocal library to obtain the local system timezone. The change provides better timezone detection using standard IANA timezone names instead of Celery's custom UTC offset-based representation.

Key changes:

  • Adds tzlocal as a new dependency for improved local timezone detection
  • Updates the _Zone.local property to use tzlocal.get_localzone() with fallback to LocalTimezone
  • Adds comprehensive test coverage for the new timezone detection logic

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
celery/utils/time.py Implements the core timezone detection logic using tzlocal with exception handling and fallback
t/unit/app/test_app.py Adds test cases for successful tzlocal usage and fallback scenarios when tzlocal fails
setup.cfg Adds tzlocal dependency to RPM requirements
requirements/default.txt Adds tzlocal to default Python requirements

auvipy
auvipy requested changes Sep 18, 2025
View reviewed changes
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.

do you want to add any additional tests or documentation for this?

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Sep 18, 2025 •
edited
Loading

This PR is complete for me.

The change is fully test-covered and no need for docs imo. It is still the local time zone as before. The only difference is that thanks to the IANA name it will show up nicer (human readable) in Flower after this is merged https://github.com/mher/flower/pull/1351/files.

@auvipy auvipy merged commit 989aac0 into celery:main Sep 20, 2025
106 of 108 checks passed
@mdalp mdalp mentioned this pull request Sep 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nusnus Nusnus Nusnus left review comments

Copilot code review Copilot Copilot left review comments

@auvipy auvipy auvipy approved these changes

Assignees

No one assigned

Labels

PR Type: Enhancement

Projects

None yet

Milestone

5.6.0

Development

Successfully merging this pull request may close these issues.

Full IANA time zone support

3 participants

@woutdenolf @auvipy @Nusnus