rapidsai / build-planning

Tracking for RAPIDS-wide build tasks
https://github.com/rapidsai
0 stars 4 forks source link

update scikit-build-core floors, set a minimum version, address deprecation warnings #58

Closed jameslamb closed 3 months ago

jameslamb commented 6 months ago

Description

The PRs for #2 switched most RAPIDS projects' build backends to scikit-build-core.

Across RAPIDS repos, we have the following mix of conditions in pyproject.toml today:

Taken together, that means "use the latest scikit-build-core newer than 0.7.0 at build time, and opt in to all of its newest non-experimental behaviors".

As a result of that mix, these warnings that have started showing up in build logs could eventually become errors:

WARNING: Use cmake.version instead of cmake.minimum-version with scikit-build-core >= 0.8

(example build link)

To prevent that happening unexpectedly and to provide a bit more stability, I think we should pursue the following across all RAPIDS repos:

  1. switch from cmake.minimum-version to cmake.version in [tool.scikit-build-core] table in pyproject.toml
  2. bump our scikit-build-core floor to the latest release of scikit-build-core
  3. set a minimum-version (minimum scikit-build-core version) in the [tool.scikit-build-core] table in pyproject.toml

Benefits of this work

Reduces the risk of a scikit-build-core release causing an all-RAPIDS-wheels-builds-are-broken-at-the-same-time type of event.

Removes a source of deprecation warnings from logs, reducing a bit of noise and improving the usefulness of build logs for debugging purposes.

Approach

If we agree to do this, it'd be a good candidate for rapids-reviser. Modify the scikit-build-core floor in dependencies.yaml and re-run rapids-dependency-file-generator, and the make the other edits in pyproject.toml files diretly.

Notes

Why do we need a floor on the dependency AND to set minimum-version?

See https://scikit-build-core.readthedocs.io/en/latest/configuration.html#minimum-version-defaults.

Like CMake, scikit-build-core's minimum-version configuration isn't just a constraint that's there for package managers or to raise an exception if you have a too-old version.

When scikit-build-core introduces new behaviors, it will sometimes gate them behind an "only do this if minimum-version>={something}" type of condition.

For example: https://github.com/scikit-build/scikit-build-core/blob/f6ed5a28fc85e621b03d984011d17def888ee0db/src/scikit_build_core/build/metadata.py#L41-L45

### Tasks
- [x] cudf (https://github.com/rapidsai/cudf/pull/16503)
- [x] cugraph (https://github.com/rapidsai/cugraph/pull/4597)
- [x] cugraph-gnn (nothing to do... not actually using scikit-build-core right now. Will be once wholegraph is moved there, but should be updated with https://github.com/rapidsai/wholegraph/pull/203 if new wholegraph commits keep getting moved over: https://github.com/rapidsai/cugraph-gnn/pull/21)
- [x] cugraph-ops (https://github.com/rapidsai/cugraph-ops/pull/682)
- [x] cuml (https://github.com/rapidsai/cuml/pull/6012)
- [x] cuopt (https://github.com/rapidsai/cuopt/pull/1955)
- [x] cuspatial (https://github.com/rapidsai/cuspatial/pull/1430)
- [x] cuvs (https://github.com/rapidsai/cuvs/pull/280)
- [x] kvikio (https://github.com/rapidsai/kvikio/pull/430)
- [x] pynvjitlink (https://github.com/rapidsai/pynvjitlink/pull/99)
- [x] raft (https://github.com/rapidsai/raft/pull/2406)
- [x] rmm (https://github.com/rapidsai/rmm/pull/1637)
- [x] ucxx (https://github.com/rapidsai/ucxx/pull/258)
- [x] wholegraph (https://github.com/rapidsai/wholegraph/pull/203)
### other items related to updating scikit-build-core
- [x] devcontainers (https://github.com/rapidsai/devcontainers/pull/377)
vyasr commented 6 months ago
  1. The cmake.minimum-version (and ninja.minimum-version, for that matter) changes happened at our request, see https://github.com/scikit-build/scikit-build-core/issues/571#issuecomment-1858574412 and https://github.com/scikit-build/scikit-build-core/issues/583. I agree that we should update those now. If we are going to update them, we should consider whether we ought to also follow scikit-build-core's recommendation to remove these entirely from the build-system.requires table. We previously decided against that, in large part because this more precise version selection constraint was missing.
  2. I don't particularly feel the need to bump up our floor to the latest scikit-build-core version, but I'm fine with doing so if you'd like. We'll need to bump up to at least 0.8 to be able to make the above change, anything beyond that is fine with me but not strictly necessary.
  3. I definitely agree that we should do this.
vyasr commented 6 months ago

In addition to updating RAPIDS projects that use SKBC to build, we should also make sure to update the samples in dfg (see https://github.com/rapidsai/dependency-file-generator/issues/98).

jameslamb commented 3 months ago

This has new urgency because scikit-build-core just release a v0.10.0 which changed the handling of cmake.minimum-version in a way that will breaks RAPIDS repos.

notes: https://github.com/scikit-build/scikit-build-core/releases/tag/v0.10.0

henryiii commented 3 months ago

Details on what broke? cmake.minimum-version shouldn't change at all if you don't change minimum-version (and minimum-version has to be set below some value like .8 in order to use the old cmake.minimum-version instead of cmake.version in the first place (minus a warning about using it appearing if it's not set at all)).

jameslamb commented 3 months ago

Sorry @henryiii , I'll have more details for you in a few minutes. We haven't actually observed the new release breaking anything, so it was careless of me to say "will breaks RAPIDS repos." I think "might" would have been more appropriate.

In short... we don't set minimum-version and so always are on the latest scikit-build-core's behavior (something I hoped to change with this issue we're talking on, but which we just haven't gotten to across RAPIDS yet). We saw the release notes for the new scikit-build-core==0.10.0 and drew some conclusions about how the automatic CMake version detection would interact with our repos.

Those conclusions might not have been right. I've put up https://github.com/rapidsai/rmm/pull/1638 to test our assumptions and try to get more information about the actual impact.

bdice commented 3 months ago

Conversation moved to here (we do see CI failures, and are discussing the root causes here): https://github.com/rapidsai/rmm/pull/1637#issuecomment-2272248793

jakirkham commented 3 months ago

Thanks James! 🙏

Appreciate you getting these PRs started

Have moved this issue to in progress to reflect that

henryiii commented 3 months ago

FYI, you can set:

[tool.scikit-build]
minimum-version = "build-system.requires"

To single-source the minimum version from the scikit-build-core>=0.10.

jakirkham commented 3 months ago

Thanks Henry! 🙏

That's a nice addition. Noticing this got added in 0.10.0 😄

Linking the docs for reference

henryiii commented 3 months ago

It's also at https://iscinumpy.dev/post/scikit-build-core-0-10/.

jameslamb commented 3 months ago

Thanks @henryiii , really nice feature! I've just made that change on all the PRs linked in the task list at the top of this issue. If those are merged, then RAPIDS will be on scikit-build-core >= 0.10 and using minimum-version = "build-system.requires".

jakirkham commented 3 months ago

Reviewed and merged a good chunk of these

There are a handful that are still running CI or have unrelated CI errors that need investigation

jameslamb commented 3 months ago

Thanks for helping to move this forward @jakirkham !

I've checked the statuses this morning and all the remaining one haves unrelated CI failures. Will keep checking and updating them.

vyasr commented 3 months ago

@jameslamb is https://github.com/rapidsai/cuspatial/pull/1430 all that's left before we can close this?

jameslamb commented 3 months ago

is https://github.com/rapidsai/cuspatial/pull/1430 all that's left before we can close this?

yes

jameslamb commented 3 months ago

That cuspatial PR is now merged, so this is complete.

jakirkham commented 3 months ago

Thanks all! 🙏