Uh oh!
There was an error while loading. Please reload this page.
ARROW-17868: [C++][Python] Restore the ARROW_PYTHON CMake option - #14273
Conversation
kou
commented
Sep 29, 2022
@github-actions crossbow submit -g nightly-tests -g nightly-release -g nightly-packaging |
This comment was marked as outdated.
This comment was marked as outdated.
kou
commented
Sep 29, 2022
@github-actions crossbow submit -g nightly-tests -g nightly-release -g nightly-packaging |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Is all this required for R? AFAIR, Python is only used to test PyArrow-R interoperability here.
There was a problem hiding this comment.
@nealrichardson Do you know what components are needed here?
There was a problem hiding this comment.
It seems that we can disable ARROW_HDFS. Other components seems necessary. I'll try it.
Uh oh!
There was an error while loading. Please reload this page.
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Thanks! Added a few comments
There was a problem hiding this comment.
Should we just hardcode this to ON? (the ARROW_PYTHON=ON would have ensured that) I don't think we want to create wheels without dataset enabled?
There was a problem hiding this comment.
I don't think we want to create wheels without dataset enabled
I think so but ${ARROW_DATASET} is also used for export PYARROW_WITH_DATASET=${ARROW_DATASET} below. So I think that we use ${ARROW_DATASET} here for consistency. (ARROW_DATASET is initialized by : ${ARROW_DATASET:=ON}.)
There was a problem hiding this comment.
This seems unrelated to the other changes. Can you give a short reasoning for it?
There was a problem hiding this comment.
Yes. This is unrelated to ARROW_PYTHON. Sorry for including this to this pull request.
OPENSSL_USE_STATIC_LIBS is redundant here because there is -DARROW_DEPENDENCY_USE_SHARED=OFF in this command line. OPENSSL_USE_STATIC_LIBS is set automatically in https://github.com/apache/arrow/blob/master/cpp/cmake_modules/FindOpenSSLAlt.cmake#L48-L52 .
I just found this by sorting CMake options. So I mix this change into this pull request. Sorry.
There was a problem hiding this comment.
No need to be sorry. I was just wondering about the reasoning
There was a problem hiding this comment.
| -DARROW_DATASET=${ARROW_DATASET} \ | |
| -DARROW_DATASET=ON \ | |
| -DARROW_DATASET=ON \ |
There was a problem hiding this comment.
Thanks but I choose -DARROW_DATASET=${ARROW_DATASET} for consistency.
There was a problem hiding this comment.
I haven't yet used the presets myself, but shall we include ARROW_DATASET here as well? (that was included with the previous features-python preset.
Maybe you can also keep the exact name to keep it working for people that were using those presets?
cc @wjones127
There was a problem hiding this comment.
I think I'm fine not including datasets if it's not required for the Python build. In most workflows with CMakePresets.json, I think we expect developers to create their own presets in CMakeUserPresets.json, which will inherit from the provided once. (See an example here)
Once this is merged, I can send a notice to the mailing list with instructions on how to transition to the new presets. (I need to update my blog post anyways.)
There was a problem hiding this comment.
Maybe you can also keep the exact name to keep it working for people that were using those presets?
It makes sense. I added no -minimal/-maximal versions.
There was a problem hiding this comment.
For my understanding: is the --config Debug needed if you already use CMAKE_BUILD_TYPE=Debug above?
And will this line also build in parallel?
There was a problem hiding this comment.
Yeah the --config Debug is redundant.
In my experience, it seems to automatically build in parallel, but you can also add the option explicitly.
There was a problem hiding this comment.
Ah, sorry. I added this accidentally. I revert this.
I found -j4 isn't suitable here. We can use -j$(nproc) on Linux and -j$(sysctl -n hw.ncpu) on macOS. But I thought that it's better that we use Ninja here. And I started rewriting this for Ninja but I stopped it because this pull request isn't for the change. But this change remained.
Restore it but it's marked as a deprecated option. Because the Python component in Apache Arrow C++ was moved to PyArrow by ARROW-16340. It' removed in a feature release. Users should use CMake presets instead of ARROW_PYTHON but CMake presets requires CMake 3.19 or later.
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
kou
commented
Sep 29, 2022
@github-actions crossbow submit -g nightly-tests -g nightly-release -g nightly-packaging |
Revision: 954f1ad Submitted crossbow builds: ursacomputing/crossbow @ actions-a153f6e909
|
kou
commented
Sep 29, 2022
I think that this is ready to merge. |
ursabot
commented
Oct 1, 2022
Benchmark runs are scheduled for baseline = 4dfa617 and contender = 89c0214. 89c0214 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Restore it but it's marked as a deprecated option. Because the Python component in Apache Arrow C++ was moved to PyArrow by ARROW-16340. It' removed in a feature release.
Users should use CMake presets instead of ARROW_PYTHON but CMake presets requires CMake 3.19 or later.