-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF: Construction of a DatetimeIndex from a list of Timestamp with timezone #51247
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
PERF: Construction of a DatetimeIndex from a list of Timestamp with timezone #51247
Conversation
11bbbcc
to
b5ef6ae
Compare
d4b1791
to
3a9cc7d
Compare
pyproject.toml
Outdated
@@ -27,7 +27,8 @@ dependencies = [ | |||
"numpy>=1.21.0; python_version>='3.10'", | |||
"numpy>=1.23.2; python_version>='3.11'", | |||
"python-dateutil>=2.8.2", | |||
"pytz>=2020.1" | |||
"pytz>=2020.1", | |||
"tzdata>=2022.1; 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.
does this need to be checked in setup.py? or even just import it in timezones.pyx?
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 honestly don't know, I couldn't reproduce this when building from source
Tempted to just ship it, and then check with the nightlies?
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.
@lithomas1 thoughts?
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.
LGTM. I'll tag this as build to run the wheel builders, which should sniff this stuff out.
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.
@lithomas1 did you try this?
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.
Yeah the wheel builder jobs ran on this PR. Looks like some failures. I don't have Windows access so I can't help more sorry.
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.
the wheel builder jobs ran on this PR
Ah, I see now sorry - thanks! Taking a look
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.
Actually, you might need a check for this in setup.py?
If you uninstall tzdata and try to run python setup.py develop, does this error correctly?
I don't know if setuptools reads this section of pyproject.toml.
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.
giving this a go
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.
Looks like this worked
In a new virtual environment on Windows, I:
- pip installed numpy
- pip installed cython
- pip install versioneer[toml]
- ran
python setup.py develop
and tzdata got installed, along with python-dateutil. Here's (part of) my output:
copying build\lib.win-amd64-cpython-38\pandas\_libs\window\aggregations.cp38-win_amd64.pyd -> pandas\_libs\window
copying build\lib.win-amd64-cpython-38\pandas\_libs\window\indexers.cp38-win_amd64.pyd -> pandas\_libs\window
copying build\lib.win-amd64-cpython-38\pandas\_libs\writers.cp38-win_amd64.pyd -> pandas\_libs
copying build\lib.win-amd64-cpython-38\pandas\io\sas\_sas.cp38-win_amd64.pyd -> pandas\io\sas
copying build\lib.win-amd64-cpython-38\pandas\io\sas\_byteswap.cp38-win_amd64.pyd -> pandas\io\sas
copying build\lib.win-amd64-cpython-38\pandas\_libs\json.cp38-win_amd64.pyd -> pandas\_libs
Creating c:\users\user\pandas-dev\.venv\lib\site-packages\pandas.egg-link (link to .)
Adding pandas 2.1.0.dev0+186.g4b054da685 to easy-install.pth file
Installed c:\users\user\pandas-dev
Processing dependencies for pandas==2.1.0.dev0+186.g4b054da685
Searching for tzdata>=2022.1
Reading https://pypi.org/simple/tzdata/
C:\Users\User\pandas-dev\.venv\lib\site-packages\pkg_resources\__init__.py:123: PkgResourcesDeprecationWarning: is an invalid version and will not be supported in a future release
warnings.warn(
Downloading https://files.pythonhosted.org/packages/fa/5e/f99a7df3ae2079211d31ec23b1d34380c7870c26e99159f6e422dcbab538/tzdata-2022.7-py2.py3-none-any.whl#sha256=2b88858b0e3120792a3c0635c23daf36a7d7eeeca657c323da299d2094402a0d
Best match: tzdata 2022.7
Processing tzdata-2022.7-py2.py3-none-any.whl
Installing tzdata-2022.7-py2.py3-none-any.whl to c:\users\user\pandas-dev\.venv\lib\site-packages
Adding tzdata 2022.7 to easy-install.pth file
Installed c:\users\user\pandas-dev\.venv\lib\site-packages\tzdata-2022.7-py3.8.egg
I'm on board in principle, dont know enough about pep508 to offer an informed opinion |
@MarcoGorelli IIUC you added a warning in timezones.pyx to address the associated issue(s). do you want to do this in addition to that? |
I don't think I did, no |
tzdata is versioned as I can't see how to specify a minimum for both, but putting But I'm worried that if someone has already installed I'm tempted to close, contact the tzdata conda-forge people and ask them to follow the same versioning as the PyPI package, and then specify a minimum which works for both Removing from the 2.0.0 milestone anyway, this probably isn't something worth backporting (esp. if we're not sure about it) |
- pip: | ||
- "cython" | ||
- "--extra-index-url https://pypi.anaconda.org/scipy-wheels-nightly/simple" | ||
- "--pre" | ||
- "numpy" | ||
- "scipy" | ||
- "tzdata>=2022.1" |
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.
should these somehow be marked as windows-specific?
doc/source/whatsnew/v1.5.0.rst
Outdated
@@ -657,7 +657,7 @@ Optional libraries below the lowest tested version may still work, but are not c | |||
+-----------------+-----------------+---------+ | |||
| tabulate |0.8.9 | X | | |||
+-----------------+-----------------+---------+ | |||
| tzdata |2022a | | | |||
| tzdata |2022.1 | | |
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.
do we still want to edit the 1.5 file?
cool, I've gone with 2022.1 then, the conda recipe yaml file can specify 2022a CI seems to be working fine now |
reckon we can get this in for the next release candidate? |
I tried this out in a fresh virtual environment on Windows, and tzdata got installed fine: here's my complete unedited traceback https://pastebin.com/JpuCz8PY |
ci/deps/actions-310.yaml
Outdated
- xarray>=0.21.0 | ||
- xlrd>=2.0.1 | ||
- xlsxwriter>=1.4.3 | ||
- zstandard>=0.15.2 | ||
|
||
- pip: | ||
- tzdata>=2022.1; 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.
It's probably better to be consistent and go with the pip installed tzdata everywhere, so we don't get surprises if/when Github updates its runner images.
I guess there's a risk of things breaking for people without the tzdata package installed, but that's more of a zoneinfo/Python stdlib problem.
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.
agree, but isn't that what I've done (i.e. pip-installed tzdata)?
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 meant on all platforms not just Windows, sorry if I was unclear.
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.
ok, sure, I've removed the windows-specific condition, and have installed/required it everywhere
…andas into require-tzdata-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.
LGTM. Let's give this a shot!
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 think there's one more timezone
extra reference in Installing optional dependencies with pip extras
in whatsnew/v2.0.0.rst
. Otherwise LGTM
Thanks @MarcoGorelli |
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. |
@MarcoGorelli could you do the backport? |
yup, thanks! guess this didn't make the rc then 😄 oh well |
…rom a list of Timestamp with timezone --------- Co-authored-by: MarcoGorelli <> (cherry picked from commit 3ea1780)
…rom a list of Timestamp with timezone --------- Co-authored-by: MarcoGorelli <> (cherry picked from commit 3ea1780)
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.trying to use https://peps.python.org/pep-0508/#environment-markers, let's see if this works