WIP: ENH: Support ZoneInfo timezones #40135
Conversation
|
|
||
| from pandas import ( | ||
| Timestamp, | ||
| date_range, | ||
| ) | ||
| import pandas._testing as tm | ||
|
|
||
| if PY39: | ||
| from zoneinfo import ZoneInfo |
There was a problem hiding this comment.
there's also a backport i think
There was a problem hiding this comment.
@jbrockmendel , thanks!
Added the backport. Since this is my first time messing with the dev requirements, please tell me if I missed something:
- added
backports.zoneinfoto environment.yml - ran the hook to mirror the addition in the main pip requirements
- added it to all the CI pipeline yml files. For MacOS and numpy dev added it to the pip section, since it wasn't available on conda
- added it separately to the py37_32bit setup script
There was a problem hiding this comment.
once you get this to work, would have to add this as a required dep for < 3.9 (which is prob ok to do).
There was a problem hiding this comment.
@jreback, hello! It certainly wouldn't hurt to add it, but it shouldn't be necessary if I manage to do it right. The idea was to use this issue to move away from relying on tz internals to get stuff done and to use only the public interface, which is the same between tzinfo implementations.
To be frank, last time I tried to tackle this, I got bogged down in the amount of rewriting that needs to get done with how barren base tzinfo is, but hopefully I'll have better luck this time. If not, and if we determine that this approach isn't viable, then we will need to add backports.tzinfo as a dependency and rely on zoneinfo specifics as another tzinfo implementation that we support.
But I know that thinking that we can drop all the internal stuff without an unacceptable drop in performance is optimistic.
| def test_zoneinfo_support(): | ||
| # GH 37654 | ||
| # Ensure that TimeStamp supports ZoneInfo objects | ||
| tz = ZoneInfo("US/Pacific") |
There was a problem hiding this comment.
As a suggestion, the tz_aware_fixture object should include a ZoneInfo object so it's exposed across the testing suite.
P.S. Glad to hear you're doing well
There was a problem hiding this comment.
@mroeschke Thank you! I'm very happy to feel better too.
Created the fixture, so that should make my future breaking the tz conversion and Timestamp much easier to debug.
Maybe you have any idea why this new test could be throwing an unraisable error, though? I'm a bit stumped, because in all other tests (or if I just open the console and execute the code), the error propagates through the call stack properly. I'll probably replace the test or cludge something together tomorrow, but I don't want to accumulate tech debt in case it's something less innocuous than it looks to be:
pandas/tests/tslibs/test_conversion.py::test_zoneinfo_support
C:\Users\amatamune\Anaconda3\envs\pandas-dev\lib\site-packages\_pytest\unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: 'pandas._libs.tslibs.tzconversion.tz_convert_from_utc_single'
Traceback (most recent call last):
File "pandas\_libs\tslibs\timezones.pyx", line 263, in pandas._libs.tslibs.timezones.get_dst_info
num = int(get_utcoffset(tz, None).total_seconds()) * 1_000_000_000
AttributeError: 'NoneType' object has no attribute 'total_seconds'
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))There was a problem hiding this comment.
any idea why this new test could be throwing an unraisable error, though?
get_dst_info is defined without an except? -1 clause, meaning that an exception raised inside of it is suppressed, instead printing the "ignored in [...]" and returning (uh, not sure what it returns, but its almost certainly not useful)
For troubleshooting, changing cdef object get_dst_info(tzinfo tz): to cdef object get_dst_info(tzinfo tz) except? -1: should get you a more pythonic failure
There was a problem hiding this comment.
Got it, thank you, that's super-useful!
There was a problem hiding this comment.
We probably won't need that test anyway. Adding zoneinfo to fixture seems to take care of testing.
|
|
||
| from pandas import ( | ||
| Timestamp, | ||
| date_range, | ||
| ) | ||
| import pandas._testing as tm | ||
|
|
||
| if PY39: | ||
| from zoneinfo import ZoneInfo |
There was a problem hiding this comment.
once you get this to work, would have to add this as a required dep for < 3.9 (which is prob ok to do).
|
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
| @@ -1081,6 +1087,7 @@ def iris(datapath): | |||
| timezone.utc, | |||
| timezone(timedelta(hours=1)), | |||
| timezone(timedelta(hours=-1), name="foo"), | |||
| ZoneInfo("US/Pacific"), | |||
There was a problem hiding this comment.
US/Pacific is a deprecated zone. America/Los_Angeles is the preferred key ID.
|
@AlexKirko closing as stale. |
WIP for #37654. Starting from scratch on this one. I will mark this ready for a review when it gets the job done. Apart from refactoring the code to rely only on the public interface of tzinfo independent of the implementation, the main worry is performance. I'll make sure to benchmark appopriately.
Apologies for the long absence. Had some health issues, but they are, thankfully, pinned down now, and I have mostly recovered.