-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-45295: [Python][CI] Make download_tzdata_on_windows more robust and use tzdata package for tzinfo database on Windows for ORC #45425
base: main
Are you sure you want to change the base?
Conversation
|
@github-actions crossbow submit wheel-windows-cp39-amd64 |
|
@kou is there a way to test this PR? |
@github-actions crossbow submit wheel-windows-cp39-amd64 |
|
@github-actions crossbow submit wheel-windows-* |
Revision: 29467e2 Submitted crossbow builds: ursacomputing/crossbow @ actions-435df0c2a2 |
Thanks for getting the jobs running, I'll check on them tomorrow. |
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 seems to be working and is back to the original issue failing on a specific test on Arrow to download tzdata:
def test_download_tzdata_on_windows():
tzdata_path = os.path.expandvars(r"%USERPROFILE%\Downloads\tzdata")
# Download timezone database and remove data in case it already exists
if (os.path.exists(tzdata_path)):
shutil.rmtree(tzdata_path)
> download_tzdata_on_windows()
...
with urlopen('https://data.iana.org/time-zones/tzdata-latest.tar.gz') as response:
Is the function even necessary?
I understand is a util to download tzdata but I am not sure we want to provide that utility if users can just use importlib.resources
to get the tzdata
one.
Should we just remove the utility function?
@AlenkaF @jorisvandenbossche thoughts?
Revision: 1d715f9 Submitted crossbow builds: ursacomputing/crossbow @ actions-4b3b781c2e |
@github-actions crossbow submit wheel-windows-* |
Revision: 84ab16e Submitted crossbow builds: ursacomputing/crossbow @ actions-715daf418e |
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.
@github-actions crossbow submit wheel-windows-* |
Revision: 459376c Submitted crossbow builds: ursacomputing/crossbow @ actions-b4f931ecfe |
@github-actions crossbow submit wheel-windows-* |
Last round of crossbow job failures look to be due to an issue with GitHub, I'm seeing it on other PRs too. Re-ran jobs and will keep an eye out. @raulcd whenever you have a moment, could you please look at my recent change to util.py? |
Revision: 459376c Submitted crossbow builds: ursacomputing/crossbow @ actions-c246a1f639 |
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.
@amoeba thanks for looking into this!
Apologies for the slow reply if I could have prevented some wasted time figuring this out, but indeed as you summarize in #45425 (comment), the way we understood it in the past is that we unfortunately cannot use the tzdata as shipped by the Python package. This would require some changes to support that format upstream in https://github.com/HowardHinnant/date.
See #31472 for the issue about it.
@amoeba could you update the title and top comment description of the PR, as I think it no longer reflects the change entirely?
I also see in the wheel build logs the following message (https://github.com/ursacomputing/crossbow/actions/runs/13191501020/job/36825167067#step:9:1122):
so wondering what is going wrong (or are we not downloading the tzdata files in the wheel builds?) |
Co-authored-by: Joris Van den Bossche <[email protected]>
Thanks for the review @jorisvandenbossche. Good catch on the message in CI. I'm not sure yet but will investigate. |
Rationale for this change
We have two Windows issues and this PR is addressing both:
download_tzdata_on_windows
can fail due to TLS issues in certain CI environments.These two issues are being solved in one PR simply because they appeared together during the 19.0.1 release process but they're separate.
What changes are included in this PR?
download_tzdata_on_windows
more robust to TLS errors by attempting to userequests
if it's available and falling back to urllib otherwise.Are these changes tested?
Yes.
Are there any user-facing changes?
No.