Skip to content

MAINT: Use RAPIDS 24.10 cmake and scikit-build-core setup#195

Merged
seberg merged 4 commits intorapidsai:mainfrom
seberg:rapids-24.10-build
Jan 2, 2025
Merged

MAINT: Use RAPIDS 24.10 cmake and scikit-build-core setup#195
seberg merged 4 commits intorapidsai:mainfrom
seberg:rapids-24.10-build

Conversation

@seberg
Copy link
Copy Markdown
Contributor

@seberg seberg commented Dec 17, 2024

This changes the setup to align it with the typical setup in 24.10 RAPIDS (and future) using scikit-build-core.
legate is currently using 24.10 rapids-cmake, so it seems good to keep this in sync.

@seberg seberg force-pushed the rapids-24.10-build branch 5 times, most recently from c1cb9f4 to f2eb602 Compare December 17, 2024 13:43
pyproject.toml Outdated
[tool.rapids-build-backend]
build-backend = "scikit_build_core.build"
dependencies-file = "dependencies.yaml"
commit_files = ["legateboost/GIT_COMMIT"]
Copy link
Copy Markdown
Contributor Author

@seberg seberg Dec 17, 2024

Choose a reason for hiding this comment

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

rapids-build-backend default to adding a GIT_COMMIT file, which is fine. But because the project name legate-boost and the module name legateboost mismatch, we have to add it explicitly (I think).

EDIT: spelled commit-files here of course.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes you're exactly right about this. Here's where that behavior was introduced: rapidsai/rapids-build-backend#30

This changes the setup to align it with the typical setup in 24.10
RAPIDS (and future) using scikit-build-core.
`legate` is currently using 24.10 rapids-cmake, so it seems good to
keep this in sync.

Signed-off-by: Sebastian Berg <[email protected]>
@seberg seberg force-pushed the rapids-24.10-build branch from f2eb602 to 25ede7f Compare December 17, 2024 13:51
@seberg seberg marked this pull request as ready for review December 17, 2024 14:20
@seberg seberg requested a review from jameslamb December 17, 2024 14:20
Copy link
Copy Markdown
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Awesome work!

I totally support this... getting caught up to a newer rapids-cmake and switching to scikit-build-core are really nice changes. It's great to see setup.py deleted 😁

Left a few small comments, but overall I support this. I think we should answer the rapids-cmake-sha question before merging this.

key: requires
includes:
- build
- build_tools
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want this to look a bit more like other RAPIDS libraries, then by convention these 2 lists would be spelled like this:

  py_build_legateboost:
    output: pyproject
    pyproject_dir: .
    extras:
      table: build-system
    includes:
      - rapids_build_skbuild
  py_rapids_build_legateboost:
    output: pyproject
    pyproject_dir: .
    extras:
      table: tool.rapids-build-backend
      key: requires
    includes:
      - build
      - build_tools

There are some docs on those conventions at https://github.com/rapidsai/build-planning/blob/d9e3c606d95c835ee384ac6480a4af0ac6cb024a/docs/docs/packaging.md#L85

Not critical, but following RAPIDS conventions does make it easier for more other RAPIDS folks to come in and help out on these repos.

cmake_flags = [
f"-Dlegate_ROOT:STRING={legate_dir}",
f"-DCMAKE_CUDA_ARCHITECTURES={cuda_arch}",
]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With this removed, will the following still work?

If installing with `pip`, set the `CUDAARCHS` environment variable, as described in the CMake docs ([link](https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_ARCHITECTURES.html)).
```shell
CUDAARCHS="70;80" \
pip install --no-build-isolation --no-deps .
```

It might need to be updated to something like

SKBUILD_CMAKE_ARGS="-DCMAKE_CUDA_ARCHITECTURES='70;80'"

Or something similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good find! Pushing another commit to introduce rapids_cuda_init_architectures(legateboost), that seems like a reasonable thing to me. In legate-raft and legate-dataframe, I did use the "RAPIDS" architectures (but then, they use rapids libraries...)

The difference is that if you just do a plain pip install . RAPIDS seems to default to the "RAPIDS" architectures and not "native".
So there is a slightly unfortunate change, but at least this will keep working (maybe only with the new commit).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah perfect, rapids_cuda_init_architectures() seems like the right way to do this (I did not actually know about that 🙈 )

# default
set(rapids-cmake-version 24.08)
set(rapids-cmake-version 24.10)
set(rapids-cmake-sha "3cc764f287a6f3caeee5dd1c96c24b1710d4cdf1")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know from memory... does the version matter if we're still pinning to a specific commit of rapids-cmake?

I can't tell what version is getting downloaded from the build logs since they're pretty quiet: https://github.com/rapidsai/legate-boost/actions/runs/12374421493/job/34536930592?pr=195

I'd support updating their verbosity. Not sure the right place...maybe PIP INSTALL_ARGS should pick up -v in build.sh? Or maybe we need to set CMake verbosity in scikit-build-core: https://scikit-build-core.readthedocs.io/en/latest/configuration.html#verbosity. Or maybe both?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added a lot of verbosity, but I think not in the one command (the first cmake run) that actually matters :).

As far as I can tell, this was not used at all and I removed it. My suspicion would be it was a temporary work-around at some point, but didn't double check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ha ok thanks! I think it's great that we're getting this CMake configure output now in CI logs... a lot of solutions to bugs are found in those logs in my experience:

image

If just removing the sha works then I think we're good to merge this 😁

@seberg seberg requested a review from jameslamb December 31, 2024 11:39
Copy link
Copy Markdown
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

looks great!

@seberg
Copy link
Copy Markdown
Contributor Author

seberg commented Jan 2, 2025

Putting it in, I suppose the build in CI may actually fail because gh-196 is required. I'll ask for a review there (it is trivial).

@seberg seberg merged commit 074ac15 into rapidsai:main Jan 2, 2025
@seberg seberg deleted the rapids-24.10-build branch January 2, 2025 11:50
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

Successfully merging this pull request may close these issues.

2 participants