ENH: support zoneinfo tzinfos #46425

jbrockmendel · 2022-03-18T22:24:59Z

closes ENH: support zoneinfo tzinfo objects #37654 (Replace xxxx with the Github issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

The tzlocal-handling code is effectively using the tzinfo/datetime API "correctly", so this adapts zoneinfo cases to go through that path.

We may be able to eek out some extra perf by combining tzlocal/zoneinfo checks or something. Just did the simplest thing here.

cc @pganssle note the comment about the cache in timezones.pyx L55. Is this dangerous and if so can you suggest an alternative?

Potentially closes #43516, haven't checked.

pandas/conftest.py

jbrockmendel · 2022-03-19T04:33:24Z

looks like one of the code checks doesn't like "backports.zoneinfo" in the deps file. guessing its the "." causing trouble

pganssle · 2022-03-19T14:52:00Z

cc @pganssle note the comment about the cache in timezones.pyx L55. Is this dangerous and if so can you suggest an alternative?

It's certainly "dangerous" in the sense that your UTC-detection logic will fail even for UTC-like zones. I think the whole endeavor is a bit foolhardy, as there is no reliable, guaranteed interface for determining whether a given zone is UTC. Your heuristics will work in a great many cases, so "user cleared the cache and people are using ZoneInfo("UTC")" is just another one of those.

That said, str(some_zoneinfo_zone) returns the key parameter if it exists, so you could presumably detect this with isinstance(zone, ZoneInfo) and str(zone) == "UTC".

pganssle · 2022-03-19T17:38:33Z

pandas/_libs/tslibs/timezones.pyx

@@ -210,6 +217,8 @@ cdef inline bint is_fixed_offset(tzinfo tz):
            return 1
        else:
            return 0
+    elif is_zoneinfo(tz):
+        return False


Why False when the other return values are all 0 or 1?

Also, this isn't quite right. You can tell if a ZoneInfo has a fixed offset by passing None to utcoffset; if it returns a value, that's the fixed offset.

This also works for pytz, dateutil.tz.tzoffset and datetime.timezone. It won't work for dateutil.tz.gettz("UTC") right now, but once the zoneinfo backport is merged, it will.

This whole function can probably be simplified to: return bool(tz.utcoffset(None) is not None), or (to handle the fixed offset tzfile cases for dateutil <= 2.8.2):

if treat_tz_as_dateutil(tz): if len(tz._trans_idx) == 0 and len(tz._trans_list) == 0: return 1 else: return 0 return 0 if tz.utcoffset(None) is None else 1

Of course, even that will break when the next version of dateutil comes out, so you probably want some version pinning in place (like for python-dateutil < 3.0).

Why False when the other return values are all 0 or 1?

No good reason; I'll change it to match.

This whole function can probably be simplified to [...]

Thanks. I'll probably make a dedicated branch to both Do This Right and audit our usages of is_fixed_offset, which I don't think we're very consistent about.

pganssle · 2022-03-19T17:40:59Z

pandas/_libs/tslibs/timezones.pyx

@@ -41,7 +42,7 @@ cdef int64_t NPY_NAT = get_nat()
 cdef tzinfo utc_stdlib = timezone.utc
 cdef tzinfo utc_pytz = UTC
 cdef tzinfo utc_dateutil_str = dateutil_gettz("UTC")  # NB: *not* the same as tzutc()
-
+cdef tzinfo utc_zoneinfo = ZoneInfo("UTC")


You probably don't want this to be called unconditionally. Practically speaking, this will always exist, but it's not guaranteed by the ZoneInfo API. It should probably be possible to import this file without this succeeding.

OK. Is the failure mode something like "UTC" not being present in /usr/share/zoneinfo/?

The user-facing downside of not doing this here is things going through slightly slower code paths. Not the end of the world, but worth avoiding if it doesn't require too much gymnastics.

Yes, or if the user is on Windows and doesn't have tzdata installed, which is actually probably a reasonably common failure mode — user doesn't care about tzdata because they're not using zoneinfo, then they import pandas and this constructor fails when trying to import the timezones module.

I imagine it's pretty easy to make the impact of this minimal. One way to do it:

cdef tzinfo utc_zoneinfo = None cdef bool is_utc_zoneinfo(tzinfo tz): global utc_zoneinfo if utc_zoneinfo is None: try: utc_zoneinfo = ZoneInfo("UTC") except ZoneInfoNotFoundError: return False return tz is utc_zoneinfo

Presumably this function will get inlined wherever you call it, and in the "common case" where ZoneInfo("UTC") is easily imported it's going to be two identity checks instead of 1.

If you are very concerned with performance I'd probably be trying to lazy-import zoneinfo in general anyway, in which case you have more "off-roads" to improve performance for people who don't use zoneinfo, though hopefully in the not-too-distant future pandas will switch to using zoneinfo or pytz-deprecation-shim anyway, at which point you'll be fine with eagerly importing.

pganssle · 2022-03-19T17:42:54Z

pandas/_libs/tslibs/tzconversion.pyx

-    elif is_tzlocal(tz):
-        return _tz_convert_tzlocal_utc(val, tz, to_utc=True)
+    elif is_tzlocal(tz) or is_zoneinfo(tz):
+        return _tz_localize_using_tzinfo_api(val, tz, to_utc=True)


I think this should be the default behavior, rather than something triggered only for zoneinfo objects.

im coming around to a similar opinion. Probably for a separate PR since that would involve a lot of code shuffling (attempts to unify the many places we do these checks have been stymied xref #46397, #46246)

pganssle · 2022-03-19T17:48:55Z

pandas/_libs/tslibs/timezones.pyx

@@ -2,6 +2,7 @@ from datetime import (
    timedelta,
    timezone,
 )
+from zoneinfo import ZoneInfo


This is an unconditional import — do you not require support for Python < 3.8?

If so, you should presumably have some more complicated logic here and in the is_zoneinfo logic.

Something like this works well for "is this an instance of X" without forcing the user to import the module that contains X:

https://github.com/pganssle/pytz-deprecation-shim/blob/bf0174adfce698e280c623d17fe8e82961de0c48/src/pytz_deprecation_shim/helpers.py#L11-L30

https://github.com/pganssle/pytz-deprecation-shim/blob/bf0174adfce698e280c623d17fe8e82961de0c48/src/pytz_deprecation_shim/_common.py#L6-L13

Since obviously some object isn't going to be an instance of zoneinfo.ZoneInfo if the interpreter has never imported the zoneinfo module.

This is an unconditional import — do you not require support for Python < 3.8?

I think our minimum version is 3.8, but I'm on 3.9 locally and forgot when writing this branch. Will change to try/except.

no leave this

we support >= 3.8

zoneinfo isnt stdlib until 3.9, so importing unconditionally would require making backports.zoneinfo a hard dependency (and having that in the CI dep file is causing problems AFAICT)

Even with backports.zoneinfo it needs to be conditional since they're in different namespaces, plus I really like the approach I took in the pytz deprecation shims for checking if a time zone is a pytz zone, where my checks never actually import pytz unless it's already been imported (since obviously if it's never been imported then I know the object isn't a pytz type). That may be overkill, but it's also not that hard to implement, so ⚖️ 🤷

I'll need to take a closer look at the pytz deprecation shim. We're getting rid of a lot of pytz usage in 2.0 (#34916) and these days I keep finding ways we could simplify the code even more if we dropped pytz support altogether.

pandas/_libs/tslibs/timezones.pyx

jbrockmendel · 2022-04-16T15:23:06Z

is the pre-commit failure real or a false-positive?

mroeschke · 2022-04-16T17:29:48Z

is the pre-commit failure real or a false-positive?

I think it's a false positive as pre-commit.ci is being set up.

Looks pretty good; just needs a whatsnew entry.

Also these tests are running okay on the CI on all platforms? I see from the zoneinfo docs that tzdata might need to be installed if a platform doesn't have system tz data: https://docs.python.org/3/library/zoneinfo.html#data-sources

jbrockmendel · 2022-04-16T18:36:06Z

Also these tests are running okay on the CI on all platforms?

Looks OK. I dont know how common the no-tzdata case is. i think we're OK punting on that and can revisit if/when someone opens an issue about it

mroeschke · 2022-04-16T18:55:48Z

doc/source/whatsnew/v1.5.0.rst

@@ -481,7 +481,7 @@ Timedelta

 Time Zones
 ^^^^^^^^^^
-
+- Bug in :class:`Timestamp` constructor raising when passed a ``ZoneInfo`` tzinfo object (:issue:`46425`)


Could it be more of a enhancement that all Timstamp/DTI/etc can all accept ZoneInfo objects now?

mroeschke · 2022-04-16T18:58:08Z

Looks OK. I dont know how common the no-tzdata case is. i think we're OK punting on that and can revisit if/when someone opens an issue about it

It was more of a question if our ci/deps/ files need to install tzdata to be sure these tests are being run, but it sounds like they are.

IMO users should be responsible for installing tzdata if they need for personal use cases.

jreback

lgtm @mroeschke comments though on whatsnew either way ok

mroeschke · 2022-04-18T00:56:22Z

Thanks LGTM.

Yeah a follow up calling this out as an enhancement that all Timstamp/DTI/etc can all accept ZoneInfo objects now would be good.

* ENH: support zoneinfo tzinfos * add backports.zoneinfo to ci deps * add backports.zoneinfo to pypy file * py38 compat * fix zoneinfo check * fix check on py38 * mypy fixup * mypy fixup * fix tznaive acse * fix fold * whatnsew * flesh out comment

ENH: support zoneinfo tzinfos

5344180

mroeschke reviewed Mar 18, 2022

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

jbrockmendel added 2 commits March 18, 2022 18:38

add backports.zoneinfo to ci deps

7786aa9

add backports.zoneinfo to pypy file

de77647

pganssle reviewed Mar 19, 2022

View reviewed changes

jbrockmendel added 12 commits March 20, 2022 12:45

Merge branch 'main' into enh-zoneinfo

b43b72c

py38 compat

63860a9

fix zoneinfo check

02101ea

fix check on py38

8cdc27b

Merge branch 'main' into enh-zoneinfo

edc1170

mypy fixup

60ca7c0

mypy fixup

0061f2c

fix tznaive acse

c0bc310

Merge branch 'main' into enh-zoneinfo

0c35501

Merge branch 'main' into enh-zoneinfo

5045e45

Merge branch 'main' into enh-zoneinfo

90c0df4

fix fold

6f7b7b1

jreback added the Timezones Timezone data dtype label Apr 13, 2022

jreback added this to the 1.5 milestone Apr 13, 2022

jreback reviewed Apr 13, 2022

View reviewed changes

pandas/_libs/tslibs/timezones.pyx Show resolved Hide resolved

jbrockmendel added 2 commits April 13, 2022 10:13

Merge branch 'main' into enh-zoneinfo

e99686a

Merge branch 'main' into enh-zoneinfo

c4bed63

jbrockmendel mentioned this pull request Apr 16, 2022

REF: Localizer class to de-duplicate tzconversion code #46397

Merged

jbrockmendel added 2 commits April 16, 2022 11:36

Merge branch 'main' into enh-zoneinfo

0c85a2f

whatnsew

22c2063

mroeschke reviewed Apr 16, 2022

View reviewed changes

jbrockmendel added 2 commits April 17, 2022 12:47

Merge branch 'main' into enh-zoneinfo

f3ba2b4

flesh out comment

8dc732a

jreback approved these changes Apr 18, 2022

View reviewed changes

mroeschke approved these changes Apr 18, 2022

View reviewed changes

mroeschke merged commit 4ffdab3 into pandas-dev:main Apr 18, 2022

jbrockmendel deleted the enh-zoneinfo branch April 18, 2022 14:24

simonjayhawkins mentioned this pull request Aug 26, 2022

BUG: AttributeError: 'NoneType' object has no attribute 'total_seconds' using ZoneInfo #48273

Closed

3 tasks

jorisvandenbossche mentioned this pull request Jun 16, 2023

API: Default to stdlib timezone objects instead of pytz #34916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: support zoneinfo tzinfos #46425

ENH: support zoneinfo tzinfos #46425

jbrockmendel commented Mar 18, 2022 •

edited

Loading

jbrockmendel commented Mar 19, 2022

pganssle commented Mar 19, 2022

pganssle Mar 19, 2022

jbrockmendel Mar 19, 2022

pganssle Mar 19, 2022

jbrockmendel Mar 19, 2022

pganssle Mar 20, 2022

pganssle Mar 19, 2022

jbrockmendel Mar 19, 2022

pganssle Mar 19, 2022

jbrockmendel Mar 19, 2022

jreback Mar 19, 2022

jbrockmendel Mar 19, 2022

pganssle Mar 20, 2022 •

edited

Loading

jbrockmendel Mar 21, 2022

jbrockmendel commented Apr 16, 2022

mroeschke commented Apr 16, 2022

jbrockmendel commented Apr 16, 2022

mroeschke Apr 16, 2022

mroeschke commented Apr 16, 2022

jreback left a comment

mroeschke commented Apr 18, 2022

ENH: support zoneinfo tzinfos #46425

ENH: support zoneinfo tzinfos #46425

Conversation

jbrockmendel commented Mar 18, 2022 • edited Loading

jbrockmendel commented Mar 19, 2022

pganssle commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Apr 16, 2022

mroeschke commented Apr 16, 2022

jbrockmendel commented Apr 16, 2022

Choose a reason for hiding this comment

mroeschke commented Apr 16, 2022

jreback left a comment

Choose a reason for hiding this comment

mroeschke commented Apr 18, 2022

jbrockmendel commented Mar 18, 2022 •

edited

Loading

pganssle Mar 20, 2022 •

edited

Loading