Update Python build to scikit-build #4818

dantegd · 2022-07-18T14:40:17Z

PR does the required changes for Scikit-build using RAPIDS-CMake.

Merge latest skbuild dev branch changes

conda/recipes/cuml/build.sh

robertmaynard · 2022-07-28T20:20:44Z

conda/recipes/libcuml/build.sh

@@ -7,4 +7,4 @@ if [ -n "$MACOSX_DEPLOYMENT_TARGET" ]; then
    export MACOSX_DEPLOYMENT_TARGET=10.11
 fi

-./build.sh -n clean libcuml prims -v --allgpuarch
+./build.sh -n libcuml prims -v --allgpuarch


is this a debug change?

No, the clean target of build.sh ends up calling setup.py clean, which imports skbuild which is not part of the libcuml requirements. This seemed like the simplest change to fix that since clean doesn't really seem to be necessary for buulding the conda packages.

why does skbuild need to be brought into libcuml? Shouldn't it only be needed by cuml?

That is why we needed to remove clean here, so that skbuild is not needed by libcuml.

The alternative would be to have separate C++ clean and Python clean targets, because right now clean calls both C++ and setup.py clean, the latter of which does import skbuild...

I think this hasn't been an issue with other RAPIDS libs because their clean target in build.sh manually runs rm etc rather than invoking python setup.py clean. Any attempt to use setup.py will now require skbuild. Personally I'm not sure that there is a truly clean solution for this problem other than having separate clean targets for different components, but that's a broader change to be made across the various RAPIDS libs.

robertmaynard · 2022-07-28T20:22:00Z

ci/release/update-version.sh

@@ -58,3 +58,6 @@ done
 sed_runner "s/export UCX_PY_VERSION=.*/export UCX_PY_VERSION='${NEXT_UCX_PY_VERSION}'/g" ci/checks/style.sh
 sed_runner "s/export UCX_PY_VERSION=.*/export UCX_PY_VERSION='${NEXT_UCX_PY_VERSION}'/g" ci/cpu/build.sh
 sed_runner "s/export UCX_PY_VERSION=.*/export UCX_PY_VERSION='${NEXT_UCX_PY_VERSION}'/g" ci/gpu/build.sh
+
+# rapids-cmake version
+sed_runner 's/'"branch-.*\/RAPIDS.cmake"'/'"branch-${NEXT_SHORT_TAG}\/RAPIDS.cmake"'/g' fetch_rapids.cmake


This should replace line 35 since that RAPIDS.cmake entry no longer exists

line 35 still exists

python/CMakeLists.txt

robertmaynard · 2022-07-28T20:26:42Z

python/CMakeLists.txt

+  set(CUML_VERSION_MAJOR "22")
+  set(CUML_VERSION_MINOR "08")


These look to be debug and not needed

Suggested change

set(CUML_VERSION_MAJOR "22")

set(CUML_VERSION_MINOR "08")

Added comment that variables are used in get_raft.cmake while we temporarily use that

These should be computed from the cuml-python_VERSION_MAJOR and cuml-python_VERSION_MINOR values so that they don't go out of sync on new branch creation.

vyasr

Please double-check all the RPATHs, they all need to be repointed to search the library dir. Aside from that, just a few minor additional requests, but I think they should be addressed before merging.

vyasr · 2022-07-28T20:25:26Z

python/CMakeLists.txt

+include(rapids-cmake)
+include(rapids-cpm)
+include(rapids-find)


I don't think any of this is necessary.

Looks like these still need removing.

vyasr · 2022-07-28T20:26:58Z

python/CMakeLists.txt

+option(FIND_CUML_CPP "Search for existing CUML C++ installations before defaulting to local files" OFF)
+option(SINGLEGPU "Disable all mnmg components and comms libraries" OFF)
+
+message(VERBOSE "CUML_PY: Searching for existing CUML C++ installations before defaulting to local files: ${FIND_CUML_CPP}")


FWIW you can use the CMAKE_MESSAGE_CONTEXT variable to help set prefixes like CUML_PY for all messages within a scope.

vyasr · 2022-07-28T20:27:35Z

python/CMakeLists.txt

+  set(CUML_VERSION_MAJOR "22")
+  set(CUML_VERSION_MINOR "08")
+  set(CUML_USE_RAFT_NN ON)
+  set(CUML_USE_RAFT_DIST ON)


Are any of these getting used?

Yes, they are needed by get_raft.cmake

vyasr · 2022-07-28T20:29:08Z

python/CMakeLists.txt

+  set(CUML_USE_RAFT_NN ON)
+  set(CUML_USE_RAFT_DIST ON)
+
+  # We need to call get_raft due to cuML asing for raft::nn and


Suggested change

# We need to call get_raft due to cuML asing for raft::nn and

# We need to call get_raft due to cuML asking for raft::nn and

vyasr · 2022-07-28T20:31:40Z

python/CMakeLists.txt

+
+  # We need to call get_raft due to cuML asing for raft::nn and
+  # raft::distance targets
+  include(../cpp/cmake/thirdparty/get_raft.cmake)


Shouldn't the raft targets be exported from the libcuml target? I see that they're part of the public link interface and raft::raft is part of the export sets. Should get_raft.cmake be using PKG_USE_RAFT_DIST, PKG_USE_RAFT_NN, and/or RAFT_COMPILE_LIBRARIES to decide whether or not to manually add raft::nn and/or raft::distance to the export sets as well? I'm thinking we need to use rapids_export_package manually on those targets here. CC @robertmaynard

Actually thinking about it for a bit, I think we just need to add raft::nn and raft::distance (conditionally) to

cuml/cpp/cmake/thirdparty/get_raft.cmake

Line 55 in 5324d52

GLOBAL_TARGETS raft::raft

should be enough to properly add them to libcuml's exported targets, will try it locally. So I think there's no need for an additional rapids_export_package

I think that the raft::nn and raft::distance are made GLOBAL when they are brought in as well. ( if not they should be )

They are https://github.com/rapidsai/raft/blob/branch-22.08/cpp/CMakeLists.txt#L493

It seems then there is a bug somewhere, without calling get_raft.cmake, or modifying get_raft.cmake in cuML, we run into the following issue:

CMake Error at /home/ursa/miniconda3/envs/skbuild4/lib/cmake/cuml/cuml-targets.cmake:56 (set_target_properties): The link interface of target "cuml::cuml++" contains: raft::nn but the target was not found. Possible reasons include: * There is a typo in the target name. * A find_package call is missing for an IMPORTED target. * An ALIAS target is missing. Call Stack (most recent call first): /home/ursa/miniconda3/envs/skbuild4/lib/cmake/cuml/cuml-config.cmake:77 (include) CMakeLists.txt:68 (find_package)

The dependency tracking in rapids-CMake doesn't understand COMPONENTs and therefore cuml is find raft but not enabling any components.
I think we can re call find package raft with the right components to unblock us.

Please file an issue on rapids-CMake about this

vyasr · 2022-07-28T20:38:23Z

python/cuml/metrics/cluster/CMakeLists.txt

+endforeach()
+
+foreach(cython_module IN LISTS RAPIDS_CYTHON_CREATED_TARGETS)
+  set_target_properties(${cython_module} PROPERTIES INSTALL_RPATH "\$ORIGIN;\$ORIGIN/cpp")


Make sure this RPATH goes the necessary extra level higher.

vyasr · 2022-07-28T20:39:42Z

python/cuml/metrics/cluster/CMakeLists.txt

+# fixed in https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7410 and will be available in
+# CMake 3.24, so we can remove the Development component once we upgrade to CMake 3.24.
+find_package(Python REQUIRED COMPONENTS Development NumPy)
+set(targets_using_numpy


I just did a quick scan of a handful of these and I didn't see any numpy logic in the pyx files. Is the dependency inherited (maybe from raft), or is this not really necessary?

vyasr · 2022-07-28T20:40:16Z

python/cuml/neighbors/CMakeLists.txt

+foreach(target IN LISTS targets_using_numpy)
+  target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")
+endforeach()
+


Suggested change

foreach(target IN LISTS targets_using_numpy)

target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")

endforeach()

vyasr · 2022-07-28T20:42:08Z

python/setup.py

-cmdclass["build"] = cuml_build
-cmdclass["build_ext"] = cuml_build_ext
+cmdclass = versioneer.get_cmdclass()
+cmdclass["build_ext"] = build_ext


This is redundant, skbuild injects its own commands automatically.

Suggested change

cmdclass["build_ext"] = build_ext

vyasr · 2022-07-28T20:43:17Z

python/setup.py

-      setup_requires=['cython'],
+      url="https://github.com/rapidsai/cudf",
+      setup_requires=['Cython>=0.29,<0.30'],
+      packages=find_packages(include=['cuml', 'cuml.*']),


In all the other RAPIDS packages we include the pxd files as part of the package data in case someone wants to build Cython against the Cython in our libs. Not strictly relevant here, but I thought I'd point it out in case it's an unintentional oversight. If needed the code can be copied from one of the other RAPIDS libs.

…wards before

vyasr

One very minor outstanding comment, otherwise this looks fine to me (pending Rob's comments as well).

vyasr · 2022-07-29T20:43:34Z

python/cuml/common/CMakeLists.txt

+  set_target_properties(${cython_module} PROPERTIES INSTALL_RPATH "\$ORIGIN;\$ORIGIN/cpp")
+endforeach()
+
+# todo: ml_cuda_utils.h should be in the include folder of cuML or the functionality


vyasr · 2022-07-29T20:43:43Z

python/CMakeLists.txt

+option(FIND_CUML_CPP "Search for existing CUML C++ installations before defaulting to local files" OFF)
+option(SINGLEGPU "Disable all mnmg components and comms libraries" OFF)
+
+message(VERBOSE "CUML_PY: Searching for existing CUML C++ installations before defaulting to local files: ${FIND_CUML_CPP}")


vyasr · 2022-07-29T20:49:35Z

python/CMakeLists.txt

+include(rapids-cmake)
+include(rapids-cpm)
+include(rapids-find)


Looks like these still need removing.

ci/release/update-version.sh

robertmaynard · 2022-08-01T15:34:13Z

python/CMakeLists.txt

+  set(CUML_VERSION_MAJOR "22")
+  set(CUML_VERSION_MINOR "08")


These should be computed from the cuml-python_VERSION_MAJOR and cuml-python_VERSION_MINOR values so that they don't go out of sync on new branch creation.

codecov-commenter · 2022-08-02T00:25:38Z

Codecov Report

Merging #4818 (0d3274a) into branch-22.08 (5324d52) will increase coverage by 0.39%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.08    #4818      +/-   ##
================================================
+ Coverage         77.62%   78.02%   +0.39%     
================================================
  Files               180      180              
  Lines             11384    11385       +1     
================================================
+ Hits               8837     8883      +46     
+ Misses             2547     2502      -45

Flag	Coverage Δ
dask	`46.21% <ø> (+0.69%)`	⬆️
non-dask	`67.27% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/cuml/feature_extraction/_vectorizers.py	`89.93% <0.00%> (+0.37%)`	⬆️
python/cuml/common/import_utils.py	`59.82% <0.00%> (+0.85%)`	⬆️
.../dask/extended/linear_model/logistic_regression.py	`92.00% <0.00%> (+57.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5324d52...0d3274a. Read the comment docs.

dantegd · 2022-08-02T15:29:44Z

@gpucibot merge

PR does the required changes for Scikit-build using RAPIDS-CMake. - [x] Update .gitignore - [x] Create `python/cuml/CMakeLists.txt` file - [x] Add `CMakeLists.txt` using RAPIDS-CMake to Python folders - [x] Update `setup.py` - [x] Update `build.sh` - [x] Update CI files - [x] Update conda env files - [x] Clean code Authors: - Dante Gama Dessavre (https://github.com/dantegd) Approvers: - Divye Gala (https://github.com/divyegala) - Corey J. Nolet (https://github.com/cjnolet) - Sevag H (https://github.com/sevagh) - Vyas Ramasubramani (https://github.com/vyasr) - Robert Maynard (https://github.com/robertmaynard) URL: rapidsai#4818

dantegd and others added 3 commits July 14, 2022 17:08

FEA added CMakeLists to python folders

c6a65e6

FIX Multiple cmake fixes

d12eaeb

FIX Multiple small fixes

b92e40a

github-actions bot added CMake conda conda issue CUDA/C++ Cython / Python Cython or Python issue labels Jul 18, 2022

dantegd added feature request New feature or request breaking Breaking change and removed conda conda issue Cython / Python Cython or Python issue CMake CUDA/C++ labels Jul 18, 2022

dantegd and others added 10 commits July 19, 2022 16:28

DBG Move files back to original structure

6a2b1d6

FIX CMake fixes

1a73eb9

FIX Use find_raft

5c74296

FIX Small pytests fixes

390c547

DBG Rename dask tests to accomodate for pytest

2a686e3

FEA Add singlegpu option to cmake files

e700dcf

FEA Initial build.sh update

8001205

FIX Typo

6d5a7c0

FIX Remove accidentally deleted function

28cb356

Merge pull request #5 from dantegd/dev-skbuild

25e0b35

Merge latest skbuild dev branch changes

github-actions bot added CMake conda conda issue CUDA/C++ Cython / Python Cython or Python issue labels Jul 20, 2022

dantegd and others added 3 commits July 19, 2022 22:03

FIX Copyright years

7e5751d

FIX PEP8 fixes

f0b8465

Merge branch 'dev-skbuild' into fea-skbuild

e70c697

dantegd marked this pull request as ready for review July 28, 2022 15:57

dantegd requested review from a team as code owners July 28, 2022 15:57

cjnolet approved these changes Jul 28, 2022

View reviewed changes

sevagh approved these changes Jul 28, 2022

View reviewed changes

v22.08 Release automation moved this from PR-WIP to PR-Reviewer approved Jul 28, 2022

robertmaynard requested changes Jul 28, 2022

View reviewed changes

v22.08 Release automation moved this from PR-Reviewer approved to PR-Needs review Jul 28, 2022

vyasr requested changes Jul 28, 2022

View reviewed changes

dantegd mentioned this pull request Jul 29, 2022

[FEA] Make dependency tracking aware of COMPONENTS rapidsai/rapids-cmake#228

Closed

dantegd added 4 commits July 29, 2022 12:26

FIX Review feedback

d1ea26e

FIX Totally did not add the single and multigpu linked libraries back…

be8cf5c

…wards before

Merge branch 'dev-skbuild' into fea-skbuild

3294065

FIX PEP8 fixes

c750028

vyasr approved these changes Jul 29, 2022

View reviewed changes

dantegd added 2 commits August 1, 2022 08:49

FIX Small fixes from review

3575e03

FIX Redo update-version.sh change

d2e3862

robertmaynard requested changes Aug 1, 2022

View reviewed changes

dantegd added 2 commits August 1, 2022 10:39

ENH Use cuml-python versions to compute get_raft variables

3c17fdb

ENH Add python/cmakelists to update-versions

0d3274a

v22.08 Release automation moved this from PR-Needs review to PR-Reviewer approved Aug 2, 2022

robertmaynard approved these changes Aug 2, 2022

View reviewed changes

rapids-bot bot merged commit 33c0170 into rapidsai:branch-22.08 Aug 2, 2022

v22.08 Release automation moved this from PR-Reviewer approved to Done Aug 2, 2022

csadorf mentioned this pull request Oct 19, 2022

Fix/build-and-test-cuml-22.08+ trxcllnt/rapids-compose#83

Merged

jameslamb mentioned this pull request Jun 9, 2023

[python-package] [ci] switch to PEP 517 / 518 builds (remove setup.py) (fixes #5061) microsoft/LightGBM#5759

Merged

	# We need to call get_raft due to cuML asing for raft::nn and
	# We need to call get_raft due to cuML asking for raft::nn and

	foreach(target IN LISTS targets_using_numpy)
	target_include_directories(${target} PRIVATE "${Python_NumPy_INCLUDE_DIRS}")
	endforeach()

Update Python build to scikit-build #4818

Update Python build to scikit-build #4818

Conversation

dantegd commented Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr Jul 28, 2022 • edited

Choose a reason for hiding this comment

dantegd Jul 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 2, 2022

Codecov Report

dantegd commented Aug 2, 2022

dantegd commented Jul 18, 2022 •

edited

vyasr Jul 28, 2022 •

edited

dantegd Jul 28, 2022 •

edited