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

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

Open
jameslamb opened this issue May 2, 2024 · 2 comments

Comments

@jameslamb
Copy link
Member

jameslamb commented May 2, 2024

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:

  • pin of scikit-build-core[pyproject]>=0.7.0 (raft pyproject.toml example)
  • no minimum-version set in the [tool.scikit-build-core] table in pyproject.toml

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

@vyasr
Copy link
Contributor

vyasr commented May 3, 2024

  1. The cmake.minimum-version (and ninja.minimum-version, for that matter) changes happened at our request, see Exception raised during wheel build. scikit-build/scikit-build-core#571 (comment) and Support complete version specification syntax for CMake and ninja versions scikit-build/scikit-build-core#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
Copy link
Contributor

vyasr commented May 16, 2024

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

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

2 participants