-
Notifications
You must be signed in to change notification settings - Fork 67
Migrate from cmake-format to gersemi #966
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
base: main
Are you sure you want to change the base?
Changes from all commits
cc2eec2
de0b179
ca403b7
516eecd
484446a
42e7bfb
8333367
eb2fcb1
c146787
61cc7d5
5a2e823
cc610ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| indent: 2 | ||
| line_length: 100 |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,8 @@ jobs: | |||||
| - checks | ||||||
| - conda-cpp-tests | ||||||
| - docs-build | ||||||
| - wheel-build-gersemi-rapids-cmake | ||||||
| - wheel-tests-gersemi-rapids-cmake | ||||||
| - telemetry-setup | ||||||
| secrets: inherit | ||||||
| uses: rapidsai/shared-workflows/.github/workflows/pr-builder.yaml@main | ||||||
|
|
@@ -50,6 +52,28 @@ jobs: | |||||
| arch: "amd64" | ||||||
| container_image: "rapidsai/ci-conda:26.04-latest" | ||||||
| script: "ci/build_docs.sh" | ||||||
| wheel-build-gersemi-rapids-cmake: | ||||||
| needs: telemetry-setup | ||||||
| secrets: inherit | ||||||
| uses: rapidsai/shared-workflows/.github/workflows/wheels-build.yaml@main | ||||||
| with: | ||||||
| build_type: pull-request | ||||||
| script: ci/build_wheel_gersemi_rapids_cmake.sh | ||||||
| package-name: gersemi-rapids-cmake | ||||||
| package-type: python | ||||||
| pure-wheel: true | ||||||
| append-cuda-suffix: false | ||||||
| # This selects "ARCH=amd64 + the earliest supported Python + CUDA". | ||||||
| matrix_filter: map(select(.ARCH == "amd64")) | min_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))]) | [.] | ||||||
| wheel-tests-gersemi-rapids-cmake: | ||||||
| needs: wheel-build-gersemi-rapids-cmake | ||||||
| secrets: inherit | ||||||
| uses: rapidsai/shared-workflows/.github/workflows/wheels-test.yaml@main | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This doesn't appear to require GPUs, isn't sensitive to CPU architecture, and doesn't need to be kept fully in sync with the RAPIDS support matrix. As long as it'll run in the range of Python versions supported by RAPIDS, that should be sufficient. I think this should be done with I recommend adopting something like https://github.com/rapidsai/cuvs/blob/efe7c978d0aac628cbbf2222de26c525d5393311/.github/workflows/pr.yaml#L180-L204 to cover the oldest and latest Python version that RAPIDS supports. |
||||||
| with: | ||||||
| build_type: pull-request | ||||||
| script: ci/test_wheel_gersemi_rapids_cmake.sh | ||||||
| # This selects "ARCH=amd64 + the earliest supported Python + CUDA". | ||||||
| matrix_filter: map(select(.ARCH == "amd64")) | min_by([(.PY_VER|split(".")|map(tonumber)), (.CUDA_VER|split(".")|map(tonumber))]) | [.] | ||||||
| telemetry-summarize: | ||||||
| # This job must use a self-hosted runner to record telemetry traces. | ||||||
| runs-on: linux-amd64-cpu4 | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2023-2025, NVIDIA CORPORATION. | ||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION. | ||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||
|
|
||||||||
| repos: | ||||||||
|
|
@@ -18,6 +18,27 @@ repos: | |||||||
| - id: check-json | ||||||||
| - id: pretty-format-json | ||||||||
| args: ["--autofix", "--no-sort-keys"] | ||||||||
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||||||||
| rev: v0.14.8 | ||||||||
| hooks: | ||||||||
| - id: ruff-check | ||||||||
| args: [--fix] | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's make it explicit that this should use configuration from And even if it did, being explicit is helpful for understanding how the check is working. |
||||||||
| exclude: | | ||||||||
| (?x) | ||||||||
| ^docs/conf[.]py$ | ||||||||
|
Comment on lines
+26
to
+28
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Instead of excluding the entire file, I recommend using That way, |
||||||||
| - id: ruff-format | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| exclude: | | ||||||||
| (?x) | ||||||||
| ^docs/conf[.]py$ | ||||||||
|
Comment on lines
+30
to
+32
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we should just let |
||||||||
| - repo: https://github.com/pre-commit/mirrors-mypy | ||||||||
| rev: v1.19.0 | ||||||||
| hooks: | ||||||||
| - id: mypy | ||||||||
| additional_dependencies: | ||||||||
| - gersemi | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Let's use the same pinning through all of these checks that's used in the package itself, so a new release of (this implies switching the existing |
||||||||
| exclude: | | ||||||||
| (?x) | ||||||||
| ^docs/conf[.]py$ | ||||||||
| - repo: https://github.com/pre-commit/mirrors-clang-format | ||||||||
| rev: v20.1.4 | ||||||||
| hooks: | ||||||||
|
|
@@ -39,19 +60,28 @@ repos: | |||||||
| hooks: | ||||||||
| - id: rapids-dependency-file-generator | ||||||||
| args: ["--clean", "--warn-all", "--strict"] | ||||||||
| - repo: https://github.com/rapidsai/pre-commit-hooks | ||||||||
| rev: v1.2.1 | ||||||||
| hooks: | ||||||||
| - id: verify-copyright | ||||||||
| args: [--fix, --spdx] | ||||||||
| files: | | ||||||||
| (?x) | ||||||||
| [.](cmake|cpp|cu|cuh|h|hpp|sh|pxd|py|pyx)$| | ||||||||
| CMakeLists[.]txt$| | ||||||||
| meta[.]yaml$| | ||||||||
| ^testing/export/write_language-multiple-nested-enables/A/B/static[.]not_cu$| | ||||||||
| dependencies[.]yaml$| | ||||||||
| pyproject[.]toml$| | ||||||||
| ^[.]pre-commit-config[.]yaml$| | ||||||||
| [.]stubs$ | ||||||||
| exclude: | | ||||||||
| (?x) | ||||||||
| ^python/gersemi-rapids-cmake/gersemi_rapids_cmake_detail/stubs/rapids/rapids-cmake/generated/ | ||||||||
| - id: verify-codeowners | ||||||||
| args: [--fix, --project-prefix=rapids, --no-cpp, --no-python] | ||||||||
| - repo: local | ||||||||
| hooks: | ||||||||
| - id: cmake-format | ||||||||
| name: cmake-format | ||||||||
| entry: ./ci/checks/run-cmake-format.sh cmake-format | ||||||||
| language: python | ||||||||
| types: [cmake] | ||||||||
| # Note that pre-commit autoupdate does not update the versions | ||||||||
| # of dependencies, so we'll have to update this manually. | ||||||||
| additional_dependencies: | ||||||||
| - cmakelang==0.6.13 | ||||||||
| verbose: true | ||||||||
| require_serial: true | ||||||||
| - id: cmake-lint | ||||||||
| name: cmake-lint | ||||||||
| entry: ./ci/checks/run-cmake-format.sh cmake-lint | ||||||||
|
|
@@ -67,22 +97,36 @@ repos: | |||||||
| (?x)^( | ||||||||
| ^testing/.*$ | ||||||||
| ) | ||||||||
| - repo: https://github.com/rapidsai/pre-commit-hooks | ||||||||
| rev: v1.2.1 | ||||||||
| hooks: | ||||||||
| - id: verify-copyright | ||||||||
| args: [--fix, --spdx] | ||||||||
| - id: regenerate-stubs | ||||||||
| name: regenerate-stubs | ||||||||
| entry: python3 ci/regenerate_stubs.py | ||||||||
| language: python | ||||||||
| types: [cmake] | ||||||||
| additional_dependencies: | ||||||||
| - &gersemi gersemi==0.25.1 | ||||||||
| pass_filenames: false | ||||||||
| - id: gersemi-rapids-cmake | ||||||||
| name: gersemi-rapids-cmake | ||||||||
| entry: ci/checks/gersemi.sh -i --warnings-as-errors --extensions rapids_cmake_detail -- | ||||||||
| language: python | ||||||||
| types: [cmake] | ||||||||
| files: | | ||||||||
| (?x) | ||||||||
| [.](cmake|cpp|cu|cuh|h|hpp|sh|pxd|py|pyx)$| | ||||||||
| CMakeLists[.]txt$| | ||||||||
| meta[.]yaml$| | ||||||||
| ^testing/export/write_language-multiple-nested-enables/A/B/static[.]not_cu$| | ||||||||
| dependencies[.]yaml$| | ||||||||
| pyproject[.]toml$| | ||||||||
| ^[.]pre-commit-config[.]yaml$ | ||||||||
| - id: verify-codeowners | ||||||||
| args: [--fix, --project-prefix=rapids, --no-cpp, --no-python] | ||||||||
| ^rapids-cmake/ | ||||||||
| additional_dependencies: | ||||||||
| - *gersemi | ||||||||
| require_serial: true | ||||||||
| - id: gersemi-testing | ||||||||
| name: gersemi-testing | ||||||||
| entry: ci/checks/gersemi.sh -i --warnings-as-errors --extensions rapids_cmake_detail --definitions testing/ -- | ||||||||
| language: python | ||||||||
| types: [cmake] | ||||||||
| files: | | ||||||||
| (?x) | ||||||||
| ^testing/ | ||||||||
| additional_dependencies: | ||||||||
| - *gersemi | ||||||||
| require_serial: true | ||||||||
| - repo: https://github.com/shellcheck-py/shellcheck-py | ||||||||
| rev: v0.10.0.1 | ||||||||
| hooks: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,46 @@ | ||||||||||||||||||||||||||||
| #!/bin/bash | ||||||||||||||||||||||||||||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | ||||||||||||||||||||||||||||
| # SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| set -euo pipefail | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| source rapids-init-pip | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rapids-logger "Generating build requirements" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rapids-dependency-file-generator \ | ||||||||||||||||||||||||||||
| --output requirements \ | ||||||||||||||||||||||||||||
| --file-key "py_build_gersemi_rapids_cmake" \ | ||||||||||||||||||||||||||||
| --matrix "" \ | ||||||||||||||||||||||||||||
| | tee /tmp/requirements-build.txt | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rapids-logger "Installing build requirements" | ||||||||||||||||||||||||||||
| rapids-pip-retry install \ | ||||||||||||||||||||||||||||
| -v \ | ||||||||||||||||||||||||||||
| --prefer-binary \ | ||||||||||||||||||||||||||||
| -r /tmp/requirements-build.txt | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
Comment on lines
+9
to
+22
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Since the wheel is getting build with build isolation (where a dedicated virtualenv is set up using the build dependencies declares in |
||||||||||||||||||||||||||||
| rapids-generate-version > ./VERSION | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I re-read @vyasr 's comments at rapidsai/pre-commit-hooks#62 (comment) today and it reminded me... we should have a plan to deal with the following scenario:
I'd consider this blocking only because the answer might be "don't use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Crap. We might have to take the same approach we took with rapids-metadata... and I've heard of people and/or software packages (don't remember which) getting grumpy about
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One possibility is, in cases where a breaking change was made to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I'd love to avoid introducing more network requests if we could. Especially unauthenticated requests to anything owned by GitHub, which face lower rate limits than authenticated requests. On the other hand, the solution we have for formatting CMake code today already relies on fetching something from GitHub at runtime FORMAT_FILE_URL="https://raw.githubusercontent.com/rapidsai/rapids-cmake/${RAPIDS_BRANCH}/cmake-format-rapids-cmake.json"
export RAPIDS_CMAKE_FORMAT_FILE=/tmp/rapids_cmake_ci/cmake-format-rapids-cmake.json
mkdir -p "$(dirname "${RAPIDS_CMAKE_FORMAT_FILE}")"
wget -O ${RAPIDS_CMAKE_FORMAT_FILE} "${FORMAT_FILE_URL}"(rapidsai/cudf - ci/check_style.sh) Though that's a single small-ish file, probably smaller than a tarball of all the stubs here would be. And that being in That download might be causing problems too if it was run every time anyone did
I agree, this would solve the problem but also introduces some new process and maintenance burden that may not be worth it. I can think of some other options, none are that great but maybe they'll spark some ideas. Option 1: system hook to update
|
||||||||||||||||||||||||||||
| rapids-generate-version > ./python/gersemi-rapids-cmake/gersemi_rapids_cmake_detail/VERSION | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
It looks to me like this is a symlink pointing to the top-level |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| cd ./python/gersemi-rapids-cmake | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Builds here currently have the
Let's apply the fix that was applied everywhere else in rapidsai/build-planning#242
Suggested change
|
||||||||||||||||||||||||||||
| rapids-logger "Building 'gersemi-rapids-cmake' wheel" | ||||||||||||||||||||||||||||
| rapids-telemetry-record build-gersemi-rapids-cmake.log rapids-pip-retry wheel \ | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I only see I think it might be left over from some earlier, unfinished experiment with telemetry tracking. If you didn't intentionally include this and it's just here because you copied things from one of those repos, remove it... one less thing that can break, and I doubt that the builds of this library will be so expensive that the telemetry data would be worth that risk and maintenance burden right now. |
||||||||||||||||||||||||||||
| -w dist \ | ||||||||||||||||||||||||||||
| -v \ | ||||||||||||||||||||||||||||
| --no-deps \ | ||||||||||||||||||||||||||||
| --disable-pip-version-check \ | ||||||||||||||||||||||||||||
| . | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| cp dist/* "${RAPIDS_WHEEL_BLD_OUTPUT_DIR}" | ||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This pairs with my suggestion above about making this match all the other RAPIDS wheel-building scripts, using the patterns from rapidsai/build-planning#242 Notice that it also cuts out an unnecessary step via This is a pure-Python project that isn't being post-processed in any way (e.g. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rapids-logger "validate packages with 'pydistcheck'" | ||||||||||||||||||||||||||||
| pydistcheck \ | ||||||||||||||||||||||||||||
| --inspect \ | ||||||||||||||||||||||||||||
| "$(echo "${RAPIDS_WHEEL_BLD_OUTPUT_DIR}"/*.whl)" | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every RAPIDS project that uses [tool.pydistcheck]
select = [
"distro-too-large-compressed",
]
# PyPI limit is 100 MiB, fail CI before we get too close to that
max_allowed_size_compressed = '75M'(rapidsai/nx-cugraph - pyproject.toml) This PR doesn't have a That's fine if you want to do that (I wrote them and think they're mostly useful), but commenting in case this was unintentional. |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| rapids-logger "validate packages with 'twine'" | ||||||||||||||||||||||||||||
| twine check \ | ||||||||||||||||||||||||||||
| --strict \ | ||||||||||||||||||||||||||||
| "$(echo "${RAPIDS_WHEEL_BLD_OUTPUT_DIR}"/*.whl)" | ||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+46
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The convention across the rest of RAPIDS is to put this stuff in a file For example: https://github.com/rapidsai/nx-cugraph/blob/2530c42e24d9b92190349e61a2941ebab225ac85/ci/build_wheel_nx-cugraph.sh#L10 Could we do that here, for consistency? I know the indirection seems like it might not be worth it, but I think it is to make all-of-RAPIDS searches and automated updates easier. |
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| #!/bin/bash | ||
|
|
||
| # SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # It's not possible for pre-commit to install a local Python package, so | ||
| # manually add it to the PYTHONPATH instead. | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| PYTHONPATH="$(dirname "$0")/../../python/gersemi-rapids-cmake:${PYTHONPATH:-}" gersemi "${@}" |
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.
This suggestion isn't only about consistency... it could also prevent CI being blocked. Note that
telemetry-setuphere hascontinue-on-error: true, so that it could fail and not block CI.I think it being in a
needed:entry would mean it being skipped would causewheel-build-gersemi-rapids-cmaketo be skipped, which would failpr-builder, which would block CI.telemetry-setupwon't be required here if you accept the suggestion to removerapids-telemetry-recordfrom the wheel-building script. Let's rely onchecksas most other RAPIDSwheel-build-*workflows do.