-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
DEP: Remove tzdata as a hard dependency outside of Windows #63335
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
pyproject.toml
Outdated
| "numpy>=1.26.0", | ||
| "python-dateutil>=2.8.2", | ||
| "tzdata>=2023.3" | ||
| "tzdata>=2023.3; platform_system == '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.
With tests seeming to be fine with no minimum here (ref: #63264 (comment)), I'm wondering if we should remove the lower bound.
|
No objection here. I didn't follow the conversation super-closely though. |
|
Just noting that the conda-foge recipe should also be updated to make move |
README.md
Outdated
| - [NumPy - Adds support for large, multi-dimensional arrays, matrices and high-level mathematical functions to operate on these arrays](https://www.numpy.org) | ||
| - [python-dateutil - Provides powerful extensions to the standard datetime module](https://dateutil.readthedocs.io/en/stable/index.html) | ||
| - [tzdata - Provides an IANA time zone database](https://tzdata.readthedocs.io/en/latest/) | ||
| - [tzdata - Provides an IANA time zone database](https://tzdata.readthedocs.io/en/latest/) (Only required 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.
or emscriptem?
|
I am fine with this, and also fine with removing the lower bound on the version. Do you know what kind of error you now would get on Windows if you don't have tzdata installed? (I don't know if the |
Dr-Irv
left a comment
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 probably will require a change to the pandas-feedstock for conda
|
I added a test for the error message; a bit hacky, can remove if people prefer. |
pyproject.toml
Outdated
| "tzdata>=2023.3" | ||
| "tzdata>=2023.3; platform_system == 'Windows'", | ||
| # Emscripten is the platform system for Pyodide. | ||
| "tzdata>=2023.3; platform_system == 'Emscripten'", |
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.
I see some other packages use sys_platform != "emscripten" instead. Do we know if there is a preference? (I don't directly find a good source about this in the docs)
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.
Interesting, I took it from here.
https://pyodide.org/en/0.16.1/faq.html#how-to-detect-that-code-is-run-with-pyodide
But looking at the latest version of the docs, it now uses sys.platform.
https://pyodide.org/en/stable/usage/faq.html#how-to-detect-that-code-is-run-with-pyodide
Will update.
|
Thanks @rhshadrach! |
|
One comment on the raised error you now get when tzdata is not available: That is the same error as you would get with an actual non-existing timezone (eg a typo). Would it still be useful to amend that message with some additional content if |
|
No opposition assuming the implementation is reasonable; it's not clear to me what parts of the code we'd have to add try-excepts to do so. But I do think a more informative error message here is |
tzdataPython package #63264 (Replace xxxx with the GitHub issue number)doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.AGENTS.md.This may still need more discussion.
Docs change: