BUG: nonexistent Timestamp pre-summer/winter DST w/dateutil timezone

[AlexKirko]

• closes BUG: nonexistent Timestamp pre-summer/winter DST change with dateutil timezone #31043
• tests added 1 / passed 1
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

This implements rounding down to microseconds into Timedelta.total_seconds(). Lack of this rounding led to dateutil.tz.tzinfo.utcoffset and dst breaking less than 128 nanoseconds before winter/summer DST switch. This happened due to an unintended cast to float in Timedelta.total_seconds() compounded with Timedelta.value being np.int64 type which doesn't support long arithmetic. The loss of precision led to rounding up, which meant that total_seconds since epoch time implied DST time while the Timedelta.value hasn't yet reached DST.

Details and code examples below.

This was quite a journey, but I found out what's going on. Let's say we have a dateutil.tz.tzinfo object named du_tz and want to find out the DST-aware UTC offset.

1. We call du_tz.utcoffset(dt) which calls du_tz._find_ttinfo(dt) which calls du_tz._resolve_ambiguous_time(dt) to find the index of the last transition time that it uses to return the correct offset.
2. du_tz._resolve_ambiguous_time(dt) calls du_tz._find_last_transition(dt) which calls the _datetime_to_timestamp(dt) dateutil function.
3. This is what this function does:

def _datetime_to_timestamp(dt):
    """
    Convert a :class:`datetime.datetime` object to an epoch timestamp in
    seconds since January 1, 1970, ignoring the time zone.
    """
    return (dt.replace(tzinfo=None) - EPOCH).total_seconds()

The problem is dateutil's reliance on Timedelta.total_seconds which casts to float:

    def total_seconds(self):
        """
        Total duration of timedelta in seconds (to ns precision).
        """
        return self.value / 1e9

Demonstration:

IN:
import datetime
import pandas as pd

epoch =  1552211999999999872
ts = pd.Timestamp(epoch, tz='dateutil/US/Pacific')

EPOCH = datetime.datetime.utcfromtimestamp(0)

delta = ts.replace(tzinfo=None) - EPOCH
print(delta.value)
OUT:
1552183199999999872
IN:
print(delta.total_seconds())
OUT:
1552183200.0
IN:
print(ts.tz.dst(ts))
OUT:
datetime.timedelta(seconds=3600)

The same thing happens with a pytz timezone, only pytz relies on something else to check for DST transitions:

IN:
import datetime
import pandas as pd

epoch =  1552211999999999872
ts = pd.Timestamp(epoch, tz='US/Pacific')

EPOCH = datetime.datetime.utcfromtimestamp(0)

delta = ts.replace(tzinfo=None) - EPOCH
print(delta.value)
OUT:
1552183199999999872
IN:
print(delta.total_seconds())
OUT:
1552183200.0
IN:
print(ts.tz.dst(ts))
OUT:
datetime.timedelta(0)

So timedelta value is okay, but timedelta.total_seconds hits the precision limit of floats, and this leads to rounding and an incorrect DST offset.

[jreback]

lgtm. ping on green.

[mroeschke]

Can we test the example in the issue here? (i.e. that Timestamp(Timestamp(epoch_time, tz=..)) doesn't change value)

[AlexKirko]

@mroeschke Sure!
But the value breaks only 1 nanosecond before the switch. Currently, I'm struggling to make everything else work on every azure pipeline (see comment below).

[AlexKirko]

@mroeschke
Added to the test. Pinging you on change.

[AlexKirko]

@jreback
Okay, I found why this breaks on minimum versions and macOS. Both specify python-dateutil == 2.6.1.
Before version 2.7.0, dateutil didn't fully rely on timedelta.total_seconds(). Instead it implemented this function:

def _total_seconds(td):
    # Python 2.6 doesn't have a total_seconds() method on timedelta objects
    return ((td.seconds + td.days * 86400) * 1000000 +
            td.microseconds) // 1000000


_total_seconds = getattr(timedelta, 'total_seconds', _total_seconds)

They had this for Python 2.6 datetime support and dropped it in this PR.
This implementation grabs datetime.timedelta.total_seconds() with getattr, and we can't influence this from the pandas side.

I set up a python 3.6 environment and tested this hypothesis. The tests fail with dateutil 2.6.1 and pass with dateutil 2.7.0.

Basically, I want to know how I should proceed.
I don't see a way to work around this without overriding dateutil's tzinfo methods, and this seems like a horrible idea.
Or we could change our minimum supported version of dateutil to 2.7.0 if that's acceptable. We don't support Python 2 in pandas, so there is really no need to support a version of dateutil this low.
I wonder why we specify version 2.6.1 for macOS though.

[jreback]

so just change the test have different cases for >=2,7 and <2,7
we already moved to
2.6.1 as the min (for 1.0.0);

[AlexKirko]

Need to parse version properly, so have to import this.

[AlexKirko]

@mroeschke
This tests that the value no longer shifts when we call the constructor again. In the issue, this failed for epoch = 1552211999999999999

[AlexKirko]

Because dateutil.__version__ breaks the mypy check, I'm using pandas.compat._optional._get_version. Seems like it was written precisely for something like this, and it's certainly a better idea than importing pkg_resources.get_distribution.
Once the numpy dev PR is merged, I'll merge master into my branch, and we should be green.

[AlexKirko]

@jreback
All green, please review.

[jreback]

thanks @AlexKirko very nice

[pganssle]

s/overrid/overrode

[pganssle]

This does not make sense to me - datetime.timedelta.total_seconds should succeed, because it's equivalent to:

def total_seconds(td):
  useconds = td.days * 86400
  useconds += td.seconds 
  useconds *= 1000000
  useconds += td.microseconds
  return useconds / 1e6

I think there's actually a deeper issue here, which is that td.microseconds and td.seconds are rounded rather than truncated. Consider this:

def to_ns(td):
  ns = td.days * 86400
  ns += td.seconds
  ns *= 1000000
  ns += td.microseconds
  ns *= 1000
  ns += td.nanoseconds
  return ns

td = timedelta(1552211999999999872, unit="ns")
print(td.value)  # 1552211999999999872
print(to_ns(td))  # 1552212000000000872

That seems to be the actual root cause of this issue and should probably be fixed.

[pganssle]

Forgot to mention earlier: you should probably use dateutil/America/Los_Angeles, as that is the canonical name for this zone. The US/... zones are symlinks for backwards compatibility.