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

remove unnecessary libcuspatial wheel builds, enforce cmake-format and cmake-lint #1516

Open
wants to merge 2 commits into
base: branch-25.02
Choose a base branch
from

Conversation

jameslamb
Copy link
Member

Description

Contributes to rapidsai/build-planning#136

Proposes the following:

  • only building libcuspatial nightly wheels once per `(CUDA version, CPU architecture) combination, to save CI resources (see avoid unnecessary wheel builds ucxx#344)
  • having wheel-build-cuspatial depend on wheel-build-libcuspatial in nightly CI, not wheel-publish-libcuspatial
    • wheel-build-cuspatial just needs libcuspatial wheels from S3, not nightly PyPI
    • doing this reduces the end-to-end time for nightly builds (via increasing parallelism)
  • using cmake-format and cmake-lint here to standardize CMake code style, similar to the way that other RAPIDS libraries do
    • there were already some code snippets for this checked in here, but they weren't being used in CI

Notes for Reviewers

cmake-format / cmake-lint changes

The new files cpp/scripts/run-cmake-format.sh and cpp/cmake/config.json were borrowed and then lightly adapted from cuDF (code link).

All changes in CMakeLists.txt and .cmake files here were made automatically by cmake-format, like this:

FORMAT_FILE_URL="https://raw.githubusercontent.com/rapidsai/rapids-cmake/branch-25.02/cmake-format-rapids-cmake.json"
export RAPIDS_CMAKE_FORMAT_FILE=/tmp/rapids_cmake_ci/cmake-formats-rapids-cmake.json
mkdir -p $(dirname ${RAPIDS_CMAKE_FORMAT_FILE})
wget -O ${RAPIDS_CMAKE_FORMAT_FILE} ${FORMAT_FILE_URL}

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jameslamb jameslamb added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 23, 2025
Copy link

copy-pr-bot bot commented Jan 23, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added cmake Related to CMake code or build configuration Python Related to Python code libcuspatial Relates to the cuSpatial C++ library Java Related to Java code ci labels Jan 23, 2025
@jameslamb
Copy link
Member Author

/ok to test

Comment on lines -123 to -124
PUBLIC "$<BUILD_INTERFACE:${CUPROJ_SOURCE_DIR}/include>"
PUBLIC "$<BUILD_INTERFACE:${CUPROJSHIM_SOURCE_DIR}/include>"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only manual changes in this PR... fixes this finding from cmake-lint:

python/cuproj/cuproj/cuprojshim/CMakeLists.txt:140,02: [E1122] Duplicate keyword argument PUBLIC

My read of https://cmake.org/cmake/help/latest/command/target_include_directories.html is that these forms of https://cmake.org/cmake/help/latest/command/target_include_directories.html are equivalent, so this is just a style thing and I don't think it should affect behavior at all.

@@ -38,5 +46,8 @@
"public_var_pattern": "[A-z][0-9A-z_]+",
"argument_var_pattern": "[A-z][A-z0-9_]+",
"keyword_pattern": "[A-z][0-9A-z_]+"
},
"markup": {
"enable_markup": false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents cmake-format from reformatting comments.

ref: https://cmake-format.readthedocs.io/en/latest/configuration.html

@@ -12,6 +12,14 @@
"FIND_PACKAGE_ARGUMENTS": "*"
}
},
"rapids_cython_create_modules": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teaching cmake-format the signature of this function helps it to split it up into one keyword argument per line.

@jameslamb jameslamb changed the title WIP: remove unnecessary libcuspatial wheel builds, enforce cmake-format and cmake-lint remove unnecessary libcuspatial wheel builds, enforce cmake-format and cmake-lint Jan 23, 2025
@jameslamb jameslamb marked this pull request as ready for review January 23, 2025 23:24
@jameslamb jameslamb requested review from a team as code owners January 23, 2025 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function Java Related to Java code libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

1 participant