sot / kadi

Chandra commands and events
https://sot.github.io/kadi
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Hardwire time zone to US/Eastern #283

Closed taldcroft closed 1 year ago

taldcroft commented 1 year ago

Description

Django defaults to the America/Chicago time zone. Annoyingly, when django.setup() gets run on Unix it sets the TZ environment variable to that default. This then spills over to impacting the local time zone in the Python time or datetime modules.

What this means is that applications trying to print a time in the local time zone were using America/Chicago after django.setup() is called. This happens when kadi events are queried.

This solution of hardwiring to US/Eastern is not very nice, but in practice it is going to work since all CXC operations and operational use of kadi are in this time zone.

An alternate idea was to unset the TZ environment variable after django.setup() is called. This appears to work but I am worried about some unexpected problems in django if it expects that env var to be set. Therefore I opted for a documented django fix.

Interface impacts

After using kadi events then local times will always be displayed in Eastern instead of Chicago.

Testing

Unit tests

Functional tests

In flight ska (2023.1)

In [1]: import datetime
   ...: 
   ...: dt = datetime.datetime(2020, 11, 1, 1)

In [2]: dt.astimezone()
Out[2]: datetime.datetime(2020, 11, 1, 1, 0, 
  tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000), 'EDT'))

In [3]: import kadi.events

In [4]: kadi.events.eclipses
Out[4]: <EventQuery: Eclipse>

## Central Daylight Time
In [5]: dt.astimezone()
Out[5]: datetime.datetime(2020, 11, 1, 1, 0, 
  tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=68400), 'CDT')) 

With this patch

In [1]: import datetime
   ...: 
   ...: dt = datetime.datetime(2020, 11, 1, 1)

In [2]: dt.astimezone()
Out[2]: datetime.datetime(2020, 11, 1, 1, 0, 
  tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000), 'EDT'))

In [3]: import kadi.events

In [4]: kadi.events.eclipses
Out[4]: <EventQuery: Eclipse>

# Still EDT
In [5]: dt.astimezone()
Out[5]: datetime.datetime(2020, 11, 1, 1, 0, 
  tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000), 'EDT'))
jeanconn commented 1 year ago

I note that as the SOT user on GRETA it looks like my default TZ is GMT, so on those machines, unless set differently by user, I think there would just be a different mismatch with this fix. I suppose, technically, one would want to use TZ if set and set it otherwise? But maybe this is fine. How did this come up originally? Which applications?

javierggt commented 1 year ago

I know that FOT MP thinks in UTC (JS3 has mentioned it during calls), so I think Jean's comment is good. The question is if this will change stuff in FOT applications.

taldcroft commented 1 year ago

Good points. I don't think there will be any impact to FOT applications since django currently (and always has) stomped on the TZ env var. This is on cheru in ska3 (flight):

In [1]: import os

In [2]: os.environ['TZ']
Out[2]: 'GMT'

In [3]: import kadi.events

In [4]: kadi.events.eclipses # this calls django.setup()
Out[4]: <EventQuery: Eclipse>

In [5]: os.environ['TZ']
Out[5]: 'America/Chicago'

But it is a fair point that maybe the other solution (which was actually my first idea) of restoring TZ to be whatever it was before calling django.setup() is a better idea. On most unix systems this will end up unsetting TZ but on GRETA it would restore to GMT.

taldcroft commented 1 year ago

FYI the easiest way to see this is:

>>> from cxotime import CxoTime
>>> t = CxoTime.now()
>>> t.print_conversions()  # After django mucks w/ TZ
local       2023 Thu Mar 30 10:09:55 AM CDT  <<****
iso_local   2023-03-30T10:09:55.151000-05:00
date        2023:089:15:09:55.151           
cxcsec      796576264.335                   
decimalyear 2023.24283                      
iso         2023-03-30 15:09:55.151         
unix        1680188995.151                  
taldcroft commented 1 year ago

I would swear that my original solution of restoring the TZ env var (i.e. removing it) worked this morning, but now it does not. Here is the code in kadi/events/query.py

@contextlib.contextmanager
def restore_env_var(name):
    """
    Context manager to restore ``name`` environment variable to its original value.
    """
    prev_value = os.environ.get(name)
    yield
    if prev_value is None:
        os.environ.pop(name, None)
    else:
        os.environ[name] = prev_value

# django.setup() has an unfortunate side effect of setting the TZ environment variable,
# which subsequently impacts Python timezone handling. In particular the `time` and
# `datetime` packages will believe the local time zone is `American/Chicago`. So we
# use a context manager to restore the TZ environment variable to its original value.
try:
    with restore_env_var("TZ"):
        django.setup()
except RuntimeError as exc:
    if "populate() isn't reentrant" in str(exc):
        print(f"Warning: {exc}", file=sys.stderr)
    else:
        raise

Here is the result, showing that the TZ env var is correctly removed but that somewhere Python still thinks we are in Chicago.

In [1]: import kadi.events
   ...: 
   ...: kadi.events.eclipses
Out[1]: <EventQuery: Eclipse>

In [2]: from cxotime import CxoTime

In [3]: CxoTime().print_conversions()
local       2023 Thu Mar 30 10:38:39 AM CDT 
iso_local   2023-03-30T10:38:39.076000-05:00
date        2023:089:15:38:39.076           
cxcsec      796577988.260                   
decimalyear 2023.24288                      
iso         2023-03-30 15:38:39.076         
unix        1680190719.076                  

In [4]: import os

In [5]: "TZ" in os.environ
Out[5]: False

I'm stumped and I've already spent way too much time on this trivial thing, so I'd suggest that this be approved unless we discover a way that the current PR will really break things more than they are currently broken.