Skip to content
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 commands for scikit-build #66

Closed
vyasr opened this issue May 24, 2022 · 5 comments
Closed

Update commands for scikit-build #66

vyasr opened this issue May 24, 2022 · 5 comments

Comments

@vyasr
Copy link
Collaborator

vyasr commented May 24, 2022

With the new scikit-build build systems for RAPIDS Python packages, the default invocation of setup.py will lead to building the C++ library as well as the Python library because the default behavior is to not search for a preexisting C++ library. That gives us a few options for rapids-compose behavior going forward:

  1. Make this behavior the new default. This has the benefit of lining up with the already existing options. A user wanting to build the Python library and link to an existing C++ library can always pass -DFIND_$LIB_CPP=ON on the command line.
  2. Change compose to always pass the -DFIND_$LIB_CPP=ON option as part of the build-python commands.
  3. Do option 2, but also make the option configurable such that users could pass some other command to build-*-python to tell the build to not find a preexisting C++ library, and translate that to the corresponding CMake option under the hood.

Of these three options, I'm guessing option 1 is not what most users will expect, and we probably need to add some layer on top of the new build behavior. Between options 2 and 3, it basically boils down to whether compose should be configurable or if we should just tell users who really need to be able to control the build that they should invoke setup.py directly.

Any of these options would be straightforward to implement, I just want to get some feedback on the options before moving forward with any of them.

@cwharris
Copy link
Contributor

C++ devs will want to build C++ only, unless the public API changes. Python devs want to build the Python only, unless the C++ it out of sync somehow.

Option 1 deviates from the current behavior, so we should avoid that.
Option 2 matches the current behavior, so we should go with that.
Option 3 extends the current behavior, but I find this unnecessary, since we can just build-*-cpp && build-*-python to accomplish the same thing.

@trxcllnt
Copy link
Owner

Yeah, we should add the -DFIND_$LIB_CPP=ON flag to the python builds.

@vyasr
Copy link
Collaborator Author

vyasr commented May 24, 2022

Regarding option 3, the commands that @cwharris posted aren't equivalent to the behavior of -DFIND_$LIB_CPP=OFF. When that option is set to OFF, the Python build will independently build a new lib*.so shared library even if you have already built the C++ library. With cudf, for example, you'll end up with one libcudf.so in cpp/build/$BRANCH/$BUILD_TYPE/ and another one in python/cudf/_skbuild/$ARCH/cmake-build/cudf-cpp/. If you hardcode -DFIND_$LIB_CPP=ON, there is no way to recover the behavior of the Python build creating that separate library.

I think that's probably fine since the number of devs who need that feature will be very limited, but that's the reason I'm asking this question.

@trxcllnt
Copy link
Owner

Yeah I don't think people will need it, and if they do, they can manually invoke setup.py

@vyasr
Copy link
Collaborator Author

vyasr commented Nov 28, 2022

This is basically all set. Building cugraph inside compose has other issues which I'll address when I can, but other repos are working now.

@vyasr vyasr closed this as completed Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants