Fix.6701.scheduler time zone change bug #6719

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

Open

wxtim wants to merge 4 commits into cylc:8.4.x from wxtim:fix.6701.Scheduler_time_zone_change_bug

+182 −43

Member

wxtim commented Apr 8, 2025 •

edited

Loading

Closes #6701

The first time the scheduler loads cylc.flow.wallclock the constants TIME_ZONE_STRING_LOCAL_BASIC and TIME_ZONE_STRING_LOCAL_EXTENDED are populated for the lifetime of the scheduler. This means that the time zone returned by get_time_string will remain the same, even if the time it adds to the time zone label changes time zone.

For example a scheduler started in UK winter will always return date_time.strftime(date_time_format_string) + 'Z'. If the clocks subsequently go forward, the time recorded for local time 09:00+01:00 will be 09:00Z which is equivelent to 10:00+01:00. The consequence is that the database table will record the time_submit field as nearly an hour after the time_submit_exit field.

This bug is particularly noticable in Cylc Review where the following was observed:

i.e. the task has submitted 22 minutes in the future!

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
Changelog entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

wxtim requested review from hjoliver and MetRonnie

April 8, 2025 13:10

wxtim force-pushed the fix.6701.Scheduler_time_zone_change_bug branch from 6a49f26 to 2ad10fe Compare

April 8, 2025 13:13

wxtim commented

View reviewed changes

cylc/flow/wallclock.py

               TIME_ZONE_STRING_UTC = "Z"
               TIME_ZONE_UTC_UTC_OFFSET = (0, 0)
-              TIME_ZONE_LOCAL_UTC_OFFSET = get_local_time_zone()
-              TIME_ZONE_LOCAL_UTC_OFFSET_HOURS = TIME_ZONE_LOCAL_UTC_OFFSET[0]
-              TIME_ZONE_LOCAL_UTC_OFFSET_MINUTES = TIME_ZONE_LOCAL_UTC_OFFSET[1]
               TIME_ZONE_LOCAL_INFO = {

Member Author

wxtim Apr 8, 2025

This is only used in cylc.flow.data_store_mgr.DataStoreMgr.generate_definition_elements which seems to be for fields which are not updated. @dwsutherland might wish to comment on whether this safe.

Member

MetRonnie Apr 9, 2025 •

edited

Loading

I can't think why this information is needed in the data store/GQL API at all.

Member

dwsutherland May 6, 2025 •

edited

Loading

Yes, the time zone is recorded on the workflow object from TIME_ZONE_LOCAL_INFO:

cylc-flow/cylc/flow/data_store_mgr.py

Line 697 in d5ac2e2

if get_utc_mode():

That will only be reread from TIME_ZONE_LOCAL_INFO on start/restart and reload...
Does TIME_ZONE_LOCAL_INFO also get updated on reload?

The job nodes just get passed the time by the events or job manager.

Member Author

wxtim May 6, 2025 •

edited

Loading

That leads to a real possibility that the time zones in the GUI will be wrong on DST change? I think I should create an "investigate this" ticket.

Member

MetRonnie May 6, 2025

What is the use case for having in the GraphQL API the local system time zone on scheduler start (except not when the workflow is running in UTC mode)? Note this is not the cycle point time zone

oliver-sanders assigned wxtim

oliver-sanders added this to the 8.4.x milestone

oliver-sanders added the bug label

Member

MetRonnie commented Apr 9, 2025

N.B. will conflict with #6640

MetRonnie reviewed

View reviewed changes

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

tests/unit/test_wallclock.py Outdated Show resolved Hide resolved

cylc/flow/wallclock.py

               TIME_ZONE_STRING_UTC = "Z"
               TIME_ZONE_UTC_UTC_OFFSET = (0, 0)
-              TIME_ZONE_LOCAL_UTC_OFFSET = get_local_time_zone()
-              TIME_ZONE_LOCAL_UTC_OFFSET_HOURS = TIME_ZONE_LOCAL_UTC_OFFSET[0]
-              TIME_ZONE_LOCAL_UTC_OFFSET_MINUTES = TIME_ZONE_LOCAL_UTC_OFFSET[1]
               TIME_ZONE_LOCAL_INFO = {

Member

MetRonnie Apr 9, 2025 •

edited

Loading

I can't think why this information is needed in the data store/GQL API at all.

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

cylc/flow/wallclock.py Outdated Show resolved Hide resolved

Member

MetRonnie commented Apr 9, 2025

@wxtim Can you please explain the bug in the PR description

wxtim requested a review from MetRonnie

April 10, 2025 14:34

MetRonnie reviewed

View reviewed changes

cylc/flow/wallclock.py Outdated Show resolved Hide resolved

cylc/flow/wallclock.py Outdated Show resolved Hide resolved

cylc/flow/wallclock.py Outdated Show resolved Hide resolved

cylc/flow/wallclock.py Outdated Show resolved Hide resolved

changes.d/6719.fix.md Outdated Show resolved Hide resolved

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

tests/unit/test_wallclock.py Outdated Show resolved Hide resolved

wxtim requested a review from MetRonnie

April 11, 2025 10:26

MetRonnie reviewed

View reviewed changes

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

MetRonnie linked an issue

that may be closed by this pull request

UTC Edge effect #6701

Open

MetRonnie modified the milestones: 8.4.x, 8.4.3

wxtim force-pushed the fix.6701.Scheduler_time_zone_change_bug branch 2 times, most recently from f055f41 to 56be9d1 Compare

April 15, 2025 13:27

wxtim requested a review from MetRonnie

April 15, 2025 13:28

wxtim commented

View reviewed changes

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

MetRonnie reviewed

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

Member Author

wxtim commented Apr 16, 2025

Credit to @MetRonnie for working out why Mac was behaving differently: Python on Mac doesn't appear to require time.tzset() to update the value of TZ in Python. This means that when the orginal fixture using monkeypatch.context exited from the patched context, Python returned to the original TZ, rather than remaining in the alternate.

MetRonnie reviewed

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

MetRonnie reviewed

View reviewed changes

tests/integration/test_workflow_db_mgr.py Outdated Show resolved Hide resolved


          Tests for failures caused by caching time zones on first starting

Cylc.

wxtim added 2 commits

April 17, 2025 09:57


          Fix issues with timezones

ad7210f

response to review

rewrote test in a less resource intensive way

make the set_tz fixture more generic


          * Improve unit testing of cylc/flow/wallclock.py

1bc013f

* Modernise docstring.

wxtim force-pushed the fix.6701.Scheduler_time_zone_change_bug branch from ccaf3be to 1bc013f Compare

April 17, 2025 08:57


          Apply suggestions from code review

35d6392

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

MetRonnie reviewed

View reviewed changes

tests/unit/test_wallclock.py Show resolved Hide resolved

MetRonnie approved these changes

View reviewed changes

wxtim requested a review from MetRonnie

April 17, 2025 12:32

wxtim force-pushed the fix.6701.Scheduler_time_zone_change_bug branch from 5ad2dfa to 35d6392 Compare

April 17, 2025 12:35

Member Author

wxtim commented Apr 17, 2025

@hjoliver - Review Poke

Member Author

wxtim commented May 8, 2025

@hjoliver Review poke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug