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.
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
Update Python build to scikit-build #4818
Update Python build to scikit-build #4818
Changes from all commits
c6a65e6
d12eaeb
b92e40a
6a2b1d6
1a73eb9
5c74296
390c547
2a686e3
e700dcf
8001205
6d5a7c0
28cb356
25e0b35
7e5751d
f0b8465
e70c697
c3b3d25
754c65d
3e75c23
31d9be7
9c581c1
21c3dc8
280e5d3
575ecd0
d812b50
930a216
9104828
13e9dbe
c60b971
4fb224f
df63847
d354cb3
ae0c1b5
dbcfbe7
465b42c
c44d532
d1ea26e
be8cf5c
3294065
c750028
3575e03
d2e3862
3c17fdb
0d3274a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is this a debug change?
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.
No, the
clean
target ofbuild.sh
ends up callingsetup.py
clean, which importsskbuild
which is not part of thelibcuml
requirements. This seemed like the simplest change to fix that sinceclean
doesn't really seem to be necessary for buulding the conda packages.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.
why does
skbuild
need to be brought into libcuml? Shouldn't it only be needed by cuml?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.
That is why we needed to remove
clean
here, so thatskbuild
is not needed bylibcuml
.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 doesimport skbuild...
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.
I think this hasn't been an issue with other RAPIDS libs because their
clean
target in build.sh manually runsrm
etc rather than invokingpython setup.py clean
. Any attempt to usesetup.py
will now requireskbuild
. Personally I'm not sure that there is a truly clean solution for this problem other than having separateclean
targets for different components, but that's a broader change to be made across the various RAPIDS libs.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.
FWIW you can use the
CMAKE_MESSAGE_CONTEXT
variable to help set prefixes likeCUML_PY
for all messages within a scope.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.
#4843
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.
We couldn't do away with these?
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.
I think it's a bug. The FIL backend gets away with it because it doesn't use the
nn
anddistance
precompiled binaries, so will open an issue to look at it on a follow upThere 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.
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 usingPKG_USE_RAFT_DIST
,PKG_USE_RAFT_NN
, and/orRAFT_COMPILE_LIBRARIES
to decide whether or not to manually addraft::nn
and/orraft::distance
to the export sets as well? I'm thinking we need to userapids_export_package
manually on those targets here. CC @robertmaynardThere 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.
Actually thinking about it for a bit, I think we just need to add
raft::nn
andraft::distance
(conditionally) tocuml/cpp/cmake/thirdparty/get_raft.cmake
Line 55 in 5324d52
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.
I think that the raft::nn and raft::distance are made GLOBAL when they are brought in as well. ( if not they should be )
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.
They are https://github.com/rapidsai/raft/blob/branch-22.08/cpp/CMakeLists.txt#L493
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.
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:
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.
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