CI/TST: fix parquet tz test returning pytz fixed offset (pyarrow 18) #60143
Conversation
jorisvandenbossche
added
CI
| @@ -977,18 +976,6 @@ def test_timestamp_nanoseconds(self, pa): | |||
| def test_timezone_aware_index(self, request, pa, timezone_aware_date_list): | |||
| pytest.importorskip("pyarrow", "11.0.0") | |||
|
|
|||
| if ( | |||
| timezone_aware_date_list.tzinfo != datetime.timezone.utc | |||
| and pa_version_under18p0 | |||
There was a problem hiding this comment.
This was added in the PR removing our default usage of pytz. @mroeschke you were assuming that pyarrow would have fixed this in time for 18.0?
There was a problem hiding this comment.
(I changed the test to test the actual (although undesired) behaviour, just to ensure we are still testing the behaviour in general, and then this should also start failing once pyarrow fixes it, just like with the xfail)
There was a problem hiding this comment.
you were assuming that pyarrow would have fixed this in time for 18.0?
I don't recall anymore; this might been an unnecessary "is pyarrow installed" check.
|
The relevant builds that were failing in main (macos and arm) are passing here |
pandas/tests/io/test_parquet.py
Outdated
| if timezone_aware_date_list.tzinfo != datetime.timezone.utc: | ||
| # pyarrow returns pytz.FixedOffset while pandas constructs datetime.timezone | ||
| # https://github.com/pandas-dev/pandas/issues/37286 | ||
| import pytz |
There was a problem hiding this comment.
| import pytz | |
| pytz = pytest.importorskip("pytz") |
I suppose your prior import was safe since having pyarrow installed would require pytz to also be installed here? IMO would be nice to extra safe with importorskip in case that ever changes :)
There was a problem hiding this comment.
I actually first wrote it like that, but then intentionally did a plain import. Because otherwise, if we would for some reason no longer have pytz in the test builds that have pyarrow, we would silently never run this test. I would rather be notified at that point that we need pytz.
Now, while writing that, if the env doesn't have pytz, then pyarrow would actually fall back to another tz implementation. We could also write the test more generically so it does not only work with pytz. But I think at the moment we install pytz in almost all test envs?
There was a problem hiding this comment.
But I think at the moment we install pytz in almost all test envs?
Yeah tests envs where we install optional dependencies will have pyarrow and pytz. Sure I'm OK then skipping the importorskip
There was a problem hiding this comment.
Actually looks like we need to install pytz for the pyarrow nightly build i.e. add pyarrow to actions-311-pyarrownightly.yaml https://github.com/pandas-dev/pandas/actions/runs/11595223185/job/32283356899?pr=60143
(I guess my above comment didn't really apply to our one-off builds like the pyarrow nightly)
There was a problem hiding this comment.
OK, made the test robust with both with and without pytz; seems fine to test both cases
|
Thanks @jorisvandenbossche |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
…andas-dev#60143) * CI/TST: fix parquet tz test returning pytz fixed offset (pyarrow 18) * only convert to pytz if installed (cherry picked from commit 9cd4a28)
|
Manual backport -> #60151 |
This should fix the parquet failures that started occurring on main. PyArrow 18 was just released, and this release did not change anything regarding timezones AFAIK (but the tests were only skipping something for pyarrow<18)