-
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-37381: [Python][CI][Packaging] Enable ORC in Windows wheels and Appveyor CI #40609
Conversation
b200344
to
0c91267
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
222db36
to
2318d9a
Compare
2318d9a
to
8c1d0d4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
8c1d0d4
to
4e7ec79
Compare
4e7ec79
to
c3e0a9c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Do you know any approach to unzip tar.xz file on Windows in the CI build? @kou |
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.
+1
@github-actions crossbow submit wheel-windows* |
Revision: 1a2634b Submitted crossbow builds: ursacomputing/crossbow @ actions-2e59d7101d
|
Failed with above logs. I don't think we need to rerun them. |
Failure in AMD64 Ubuntu 20.04 R 4.3 Force-Tests true is also unrelated.
|
@kou Do you want to take another pass? |
Thanks! |
I am not sure that we should enable ORC in the wheels if this essentially always segfaults (on first use, or before you know what to do) for users. At least we could add a check on our side, if on windows, that the path is available, and if not raise an informative error to the user instead of segfaulting |
Right. It seems that the ORC project should first make it possible to use without the timezone data files. |
This PR aims to enable ORC in the wheels CI in case there is any new regression. I will work on the ORC side to make it robust as much as possible. |
For CI we also have Appveyor, which was enabled here as well. But the wheels is what we deliver to our users. If we enable ORC there, I think we need to make this more robust on our side on the short term (or disable it again in the wheels until this is fixed in ORC and we use that version) |
Sorry I'm not familiar with that. It sounds that we need to revert this change at least for wheels. |
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 0eefb07. There were 8 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
The pyarrow orc reader always crashes when it tries to create an internal orc reader. This is caused by failing to read tz database on the local host. This also disables windows wheel build when ARROW_ORC is turned on.
What changes are included in this PR?
Download IANA timezone database on the test host and explicitly setting TZDIR to make the CIs happy.
Are these changes tested?
Make sure all python wheel windows CIs pass.
Are there any user-facing changes?
No.