Simplify Python CMake #14565
Merged
Simplify Python CMake #14565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vyasr
added
CMake
vyasr
commented
Dec 4, 2023
Comment on lines
+81
to
+87
| # TODO: This install is currently overzealous. We should only install the libraries that are | ||
| # downloaded by CPM during the build, not libraries that were found on the system. However, in | ||
| # practice right this would only be a problem is if libcudf was not found but some of the | ||
| # dependencies were, and we have no real use cases where that happens. |
There was a problem hiding this comment.
I don't want to try and fix this right now. I would like to manage the scikit-build-core transition first, then revisit the best way for us to handle install rules. Some anecdotal experiments suggest that the configure/build paths might be a bit different in scikit-build-core, so I don't want to do redundant work.
vyasr
force-pushed
the
fix/simplify_python_cmake
branch
from
804ae4a to
8ec5fb8
Compare
This reverts commit 2ace699e1ddf4463fa071cc245c6cd97946aed58.
…hether to search there or not
vyasr
force-pushed
the
fix/simplify_python_cmake
branch
from
0862e02 to
d26798a
Compare
vyasr
force-pushed
the
fix/simplify_python_cmake
branch
from
f622387 to
355b0b2
Compare
raydouglass
approved these changes
Dec 6, 2023
brandon-b-miller
approved these changes
Dec 6, 2023
robertmaynard
requested changes
Dec 8, 2023
cpp/cmake/thirdparty/get_arrow.cmake
Outdated
| @@ -432,7 +446,35 @@ if(NOT DEFINED CUDF_VERSION_Arrow) | |||
| ) | |||
| endif() | |||
|
|
|||
| # If the pyarrow package contains libarrow we want to link against it directly instead of searching | |||
There was a problem hiding this comment.
This logic should be explicitly opt-in.
If you are building libcudf on a machine with a system installed pyarrow you would always use that with no way to opt out.
There was a problem hiding this comment.
Ugh you're right, but this is unfortunate. It does mean we continue to have to specify nonzero information from the CLI when invoking certain (wheel) builds, but I guess that's not the worst.
robertmaynard
approved these changes
Dec 11, 2023
karthikeyann
pushed a commit
to karthikeyann/cudf
that referenced
this pull request
Dec 12, 2023
This PR makes a number of changes to streamline the CMake code in the Python build. There are additional potential improvements that may be possible but I'd prefer to wait on until after the scikit-build-core migration because I suspect that there may be subtle differences (particularly around the install rules). The major improvements here are: - nvcomp installation no longer needs as much special casing thanks to rapidsai/rapids-cmake#496 adding missing targets - get_arrow.cmake now determines how to find arrow by checking for 1) the presence of Python, 2) the presence of pyarrow, and 3) the presence of libarrow.so.* inside the pyarrow directory. This approach works for both pip and conda packages as well as pure C++ builds and removes the need for manual determination of where to look by the builder. The assumption baked in is that the developer has installed the desired pyarrow package when building. - The `CUDF_BUILD_WHEELS` variable is now removed. All Python builds that don't find the C++ library all go down the same path now. This is particularly relevant for doing something like a local `pip install`. This is the desired behavior because if you're building the Python package in that kind of environment you always want the same behaviors. The different case is when the Python build finds the C++ build (e.g. in a conda environment where libcudf is a separate package). - The logic for linking to pyarrow headers is now centralized in a single function since it needs to be used by every Cython target. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - https://github.com/brandon-b-miller - Robert Maynard (https://github.com/robertmaynard) - Ray Douglass (https://github.com/raydouglass)
3 tasks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR makes a number of changes to streamline the CMake code in the Python build. There are additional potential improvements that may be possible but I'd prefer to wait on until after the scikit-build-core migration because I suspect that there may be subtle differences (particularly around the install rules). The major improvements here are:
CUDF_BUILD_WHEELSvariable is now removed. All Python builds that don't find the C++ library all go down the same path now. This is particularly relevant for doing something like a localpip install. This is the desired behavior because if you're building the Python package in that kind of environment you always want the same behaviors. The different case is when the Python build finds the C++ build (e.g. in a conda environment where libcudf is a separate package).Checklist