-
Notifications
You must be signed in to change notification settings - Fork 662
fix(deps): drop pytz
from dependencies
#10976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
# adding apache-flink would lock pyarrow to an old version (for all backends), | ||
# due to the transitional dependency apache-beam imposing a low upper bound. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not linked to pytz
, but while testing changes in Flink backend I wondered why apache-flink
was never installed, and it took me some time to find the explanation in #8696 (comment). I figured I might as well add a comment to save future readers some time. I can split it into a separate PR if deemed more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Yes, this is definitely a bit hairy at the moment and will be until the Flink team can support a later version of Arrow.
"sqlglot>=23.4", | ||
"toolz>=0.11", | ||
"typing-extensions>=4.3.0", | ||
"tzdata>=2022.7", # fallback time zone data on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From tzdata
documentation on PyPI:
This is a Python package containing zic-compiled binaries for the IANA time zone database. It is intended to be a fallback for systems that do not have system time zone data installed (or don’t have it installed in a standard location), as a part of PEP 615
Unfortunately, without it Windows tests all fail due to a lack of TZ information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, pandas uses "tzdata>=2022.7"
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to only install this on Windows (using a platform selector like sys_platform == 'win32'
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be sufficient to cover the OS we test against (Ubuntu & MacOS have system TZ data, Windows would have the fallback), but I don't know if less popular OS / other Linux distributions come with system time zone data, so it's a bit of a gamble - one that I personally wouldn't take.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question about tzdata
, but not blocking. Thanks for doing this!
Description of changes
Hi! This PR proposes dropping
pytz
from primary dependencies and replacing it with stdlib equivalents, as recommended bypytz
docs:Given that Ibis only supports Python 3.9 and higher, this should be pretty safe.