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

Migrate all Python builds from scikit-build to scikit-build-core #2

Closed
8 tasks done
vyasr opened this issue Dec 12, 2023 · 9 comments
Closed
8 tasks done

Migrate all Python builds from scikit-build to scikit-build-core #2

vyasr opened this issue Dec 12, 2023 · 9 comments
Assignees

Comments

@vyasr
Copy link
Contributor

vyasr commented Dec 12, 2023

RAPIDS libraries are generally built with CMake. To facilitate better integration of the C++ builds with Python builds, we switched from using pure setuptools builds to using scikit-build. This change was crucial to enabling wheels by providing a single standard entrypoint (all the usual Python pip [install|wheel|etc] machinery) for building a Python package while also compiling the required C++ components. However, scikit-build's approach to enabling this is fundamentally limited because it relies on plugging into setuptools directly in ways that setuptools only marginally supports. The result is a tool that works most of the time, but has various sharp edges (e.g. incomplete support for MANIFEST.in, broken installations in certain cases, etc) and limitations (an inability to support true editable installations, mixed support for pyproject.toml/setup.py, etc).

The solution is to switch to the newer scikit-build-core builder, a modern standards-based builder that offers the same class of functionality as scikit-build (integrating a Python build with CMake) in a more reliable manner. Doing so will allow us to completely removed deprecated components of our build systems (various uses of setup.py), get rid of workarounds for scikit-build (e.g. the languages we must specify at the CMake level), and get full support for critical features like editable installs.

PRs Contributing To This Effort

Preview Give feedback
  1. CMake breaking ci improvement python
  2. breaking improvement
  3. Python breaking ci cmake conda improvement
  4. CMake Cython / Python breaking ci conda improvement
  5. CMake breaking ci conda improvement python
  6. CMake Python breaking ci conda improvement libcudf
    vyasr
  7. 0 - Blocked CMake Python breaking conda improvement
    vyasr
  8. breaking improvement
    vyasr
@jameslamb
Copy link
Member

I'm +1 on this.

For reference, the README has a thorough and direct list of differences between scikit-build and scikit-build-core: https://github.com/scikit-build/scikit-build-core

@vyasr vyasr self-assigned this Dec 13, 2023
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this issue Dec 13, 2023
rapids-bot bot pushed a commit to rapidsai/raft that referenced this issue Dec 13, 2023
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this issue Dec 13, 2023
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this issue Dec 14, 2023
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this issue Dec 14, 2023
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this issue Dec 14, 2023
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this issue Dec 14, 2023
@bdice
Copy link
Contributor

bdice commented Dec 16, 2023

@bdice
Copy link
Contributor

bdice commented Dec 16, 2023

Also, this might not need migrated, but rapids-cmake also relies on scikit-build as a test dependency (and cython>=0.29,<0.30 which might be possible to clean up at the same time).

https://github.com/rapidsai/rapids-cmake/blob/208b0d1240caaca41603fccc10b7ac44aea47477/dependencies.yaml#L146

@vyasr
Copy link
Contributor Author

vyasr commented Dec 18, 2023

I chose not to include cugraph-ops in this work because that package uses nanobind instead of Cython for its bindings, so it's not sensitive to rapids-cython changes. I also don't know much about its nanobind usage, and it looks like they do something bespoke with setting the CMake install directory. I'd rather leave that one to the devs to manage the transition since it doesn't impact rapids-cython at all.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 19, 2023

Regarding rapids-cmake, I'm pretty sure that dependency is superfluous and can be removed altogether. We never actually launch builds via Python in rapids-cmake. Removed that dependency in rapidsai/rapids-cmake#512.

rapids-bot bot pushed a commit to rapidsai/ucxx that referenced this issue Jan 3, 2024
@jameslamb
Copy link
Member

jameslamb commented Jan 9, 2024

I think there's one more block of work that should be done as part of this.

There are some projects with configuration in environment variables that scikit-build recognized but scikit-build-core does not. From scikit-build-core's migration guide (docs)

The SKBUILD_CONFIGURE_OPTIONS environment variable is now named SKBUILD_CMAKE_ARGS for consistency.

The SKBUILD_BUILD_OPTIONS environment variable is not supported. Some specific features are accessible using alternative variables. In particular, use CMAKE_BUILD_PARALLEL_LEVEL or SKBUILD_CMAKE_VERBOSE to control build parallelism or CMake verbosity directly.

So I see 3 small pieces of additional work:

  • convert all uses of SKBUILD_CONFIGURE_OPTIONS to SKBUILD_CMAKE_ARGS
  • convert stuff like SKBUILD_BUILD_OPTIONS="-j4" to CMAKE_BUILD_PARALLEL_LEVEL=4
  • convert stuff like SKBUILD_BUILD_OPTIONS="-v" to SKBUILD_CMAKE_VERBOSE="true"

For all projects that have recently cut over to scikit-build-core.


I didn't find a ton of these from GitHub search (e.g. this link), but there are a few.

For example, cuml cut over to scikit-build-core (rapidsai/cuml#5693) but is still using SKBUILD_BUILD_OPTIONS to control parallelism (code link).

@vyasr
Copy link
Contributor Author

vyasr commented Jan 11, 2024

Good catches @jameslamb! Let me work through these. As far as build options we've just been removing the build options everywhere, so I'll do that, and hopefully the configure options are mostly fixed already since those need to be right for everything to work correctly.

@vyasr
Copy link
Contributor Author

vyasr commented Jan 11, 2024

These are the remaining issues in repos that have made the scikit-build-core transition. We can close this issue once the following PRs are merged:

@vyasr
Copy link
Contributor Author

vyasr commented Jan 12, 2024

Closing this now. All the associated work has been merged.

@vyasr vyasr closed this as completed Jan 12, 2024
ChristinaZ pushed a commit to ChristinaZ/raft that referenced this issue Jan 17, 2024
rapids-bot bot pushed a commit to rapidsai/wholegraph that referenced this issue Mar 20, 2024
See rapidsai/build-planning#2. This repo was overlooked at the time.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Jake Awe (https://github.com/AyodeAwe)

URL: #150
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