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

Use rapids-build-backend. #1502

Merged
merged 81 commits into from
May 24, 2024
Merged

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Mar 13, 2024

Description

This PR uses rapids-build-backend to simplify wheel builds and reduce the complexity of various CI/build scripts.

See also:

Checklist

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

@github-actions github-actions bot added Python Related to RMM Python API conda ci labels Mar 13, 2024
ci/build_wheel.sh Outdated Show resolved Hide resolved
@bdice bdice added non-breaking Non-breaking change 5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function labels Mar 15, 2024
@bdice bdice changed the base branch from branch-24.04 to branch-24.06 March 18, 2024 23:35
@jameslamb
Copy link
Member

Yep that worked (build link), thanks @vyasr

@jameslamb jameslamb removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label May 21, 2024
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple of small things, but approving since I don't need to look at this again unless something comes up.

ci/build_python.sh Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Just noticed this in the raft PR and it reminded me: do we also need to add a disable-cuda in the build.sh invocations?

@KyleFromNVIDIA
Copy link
Contributor

do we also need to add a disable-cuda in the build.sh invocations?

Yes, but it needs to be rapidsai.disable-cuda once rapidsai/rapids-build-backend#37 is merged.

@jameslamb
Copy link
Member

Alright I've updated this with the following changes:

  • moved rapids-build-backend floor to 0.3.0
  • passed --config-settings rapidsai.disable-cude=true in build.sh
  • consolidated rapids-generate-version calls based on Use rapids-build-backend. #1502 (comment)
  • changed librmm/_version.py to also use the __package__ pattern (instead of string literal "librmm")
  • added __all__ lists to _version.py

That's enough changes that I think this should get another review if you have time @vyasr .

@jameslamb jameslamb requested a review from vyasr May 23, 2024 20:02
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple more small notes, but approving assuming that they're addressed.

ci/build_cpp.sh Outdated Show resolved Hide resolved
dependencies.yaml Outdated Show resolved Hide resolved
SKBUILD_CMAKE_ARGS="${EXTRA_CMAKE_ARGS}" python -m pip install \
--no-build-isolation \
--no-deps \
--config-settings rapidsai.disable-cuda=true \
Copy link
Member

Choose a reason for hiding this comment

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

I just pushed one more change... remembered that pip accepts --config-settings (with an s), not --config-setting.

https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-C

Having --config-setting (no s) didn't break anything in CI or even display any warnings, so maybe newer versions of pip have code to capture both. But either way, I think --config-settings (with the s) is the right thing for pip install, given that's what is in that tool's docs.

I'll apply that change to the cudf PR. cc @KyleFromNVIDIA that should be applied to the other rapids-build-backend PRs as well.

@jameslamb
Copy link
Member

I just pushed one more tiny change (#1502 (comment)).

I think all the review comments have been addressed, so if CI passes, I'll merge this.

@jameslamb
Copy link
Member

/merge

@rapids-bot rapids-bot bot merged commit 15c0466 into rapidsai:branch-24.08 May 24, 2024
57 checks passed
rapids-bot bot pushed a commit that referenced this pull request May 30, 2024
Contributes to rapidsai/build-planning#31

Follow-up to #1502

* ensures that `update-version.sh` does not remove alpha specs like `,>=0.0.0a0` in `pyproject.toml` and conda environment files
* consolidates `rapids-build-backend` versions in `dependencies.yaml`
   - *since I was pushing a new commit here anyway, figured I'd take the opportunity to include that simplification recommended in rapidsai/cudf#15245 (comment)

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - Bradley Dice (https://github.com/bdice)

URL: #1572
rapids-bot bot pushed a commit that referenced this pull request Jun 12, 2024
…build-backend configuration (#1582)

Contributes to rapidsai/build-planning#31
Contributes to rapidsai/dependency-file-generator#89

#1502 was one of the first `rapids-build-backend` PRs merged across RAPIDS. Since it was merged, we've made some small adjustments to the approach for `rapids-build-backend`. This catches `rmm` up with those changes:

* consolidates version-handling in `ci/build_cpp.sh`
* removes `commit-file` configuration in `pyproject.toml`
  - *as of rapidsai/rapids-build-backend#30, this is no longer necessary if the project's top-level directory is `{project_name}.replace("-", "_")*
  - *and anyway, it was changed from `commit-file` to `commit-files` in that PR, so `commit-file` was being silently ignored here*
* uses `--file-key` instead of `--file_key` in `rapids-dependency-file-generator` calls

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #1582
copy-pr-bot bot pushed a commit that referenced this pull request Jun 17, 2024
…build-backend configuration (#1582)

Contributes to rapidsai/build-planning#31
Contributes to rapidsai/dependency-file-generator#89

#1502 was one of the first `rapids-build-backend` PRs merged across RAPIDS. Since it was merged, we've made some small adjustments to the approach for `rapids-build-backend`. This catches `rmm` up with those changes:

* consolidates version-handling in `ci/build_cpp.sh`
* removes `commit-file` configuration in `pyproject.toml`
  - *as of rapidsai/rapids-build-backend#30, this is no longer necessary if the project's top-level directory is `{project_name}.replace("-", "_")*
  - *and anyway, it was changed from `commit-file` to `commit-files` in that PR, so `commit-file` was being silently ignored here*
* uses `--file-key` instead of `--file_key` in `rapids-dependency-file-generator` calls

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #1582
@trivialfis
Copy link
Member

I think the README file needs to be updated.

@jakirkham
Copy link
Member

Could you please raise this in an issue or PR?

@trivialfis
Copy link
Member

@jakirkham Opened an issue here: #1609 .

@jameslamb jameslamb mentioned this pull request Jul 16, 2024
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Jul 16, 2024
Fixes #1609
Follow-up to #1502

Adds `rapids-build-backend` to the documentation on how to build `rmm` from source.

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)
  - Jiaming Yuan (https://github.com/trivialfis)

URL: #1614
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci conda improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Related to RMM Python API
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants