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

disallow fallback to Make, add devcontainers CI #363

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

jameslamb
Copy link
Member

Contributes to rapidsai/build-planning#146

Closes #262

Proposes:

  • setting [tool.scikit-build].ninja.make-fallback = false, so scikit-build-core will not silently fallback to using GNU Make if ninja is not available
  • adding devcontainers CI jobs here

Notes for reviewers

Why add devcontainers CI jobs in this PR?

The devcontainers are useful for interactive development on this project (including for outside contributors).

ucxx building successfully in them is also important for other all-of-RAPIDS builds, like those from https://github.com/rapidsai/devcontainers and other builds based on that repo. Having CI jobs run in PRs (as most of RAPIDs does) allows us to catch issues right here in ucxx when they happen, instead of only finding out when ucxxx is built as part of a larger devcontainers-based build.

How did you set up the .devcontainers folder?

Copied https://github.com/rapidsai/cugraph/tree/branch-25.04/.devcontainer and changed all the cugraph references to ucxx.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 31, 2025
Copy link

copy-pr-bot bot commented Jan 31, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@jameslamb
Copy link
Member Author

/ok to test

@jameslamb jameslamb changed the title WIP: disallow fallback to Make, add devcontainers CI disallow fallback to Make, add devcontainers CI Jan 31, 2025
@jameslamb jameslamb marked this pull request as ready for review January 31, 2025 23:39
@jameslamb jameslamb requested review from a team as code owners January 31, 2025 23:39
@jameslamb jameslamb requested a review from bdice January 31, 2025 23:39
@wence-
Copy link
Contributor

wence- commented Feb 3, 2025

setting [tool.scikit-build].ninja.make-fallback = false, so scikit-build-core will not silently fallback to using GNU Make if ninja is not available

Why is falling back problematic?

@jameslamb
Copy link
Member Author

Why is falling back problematic?

Builds with GNU Make tend to be much slower... sometimes prohibitively so. We found that accidentally not providing Ninja in the build environment led to cuGraph wheel builds sometimes timing out after 6 hours, even with an OK sccache hit rate, because they were falling back to Make, fixed in rapidsai/cugraph@65e4592

Since ninja is pip-installable on many platforms (https://pypi.org/project/ninja/#files) and declared in the package metadata for ucxx / libucxx (and most RAPIDS libraries), I think we can defensibly always expect it to exist for wheel builds, and treat any case where it doesn't exist as a mistake. This PR turns that type of mistake into a loud and obvious error.

"features": {
"ghcr.io/rapidsai/devcontainers/features/cuda:25.4": {
"version": "11.8",
"installcuBLAS": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these math libraries for UCXX? I don't think we do.

cuDF doesn't have them: https://github.com/rapidsai/cudf/blob/branch-25.04/.devcontainer/cuda11.8-pip/devcontainer.json

Same for CUDA 12.8 pip devcontainers.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're totally right, those are unnecessary here. Removed in 7980f3c

@jameslamb jameslamb requested a review from bdice February 3, 2025 15:27
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
# .devcontainer files
find .devcontainer/ -type f -name devcontainer.json -print0 | while IFS= read -r -d '' filename; do
sed_runner "s@rapidsai/devcontainers:[0-9.]*@rapidsai/devcontainers:${NEXT_RAPIDS_SHORT_TAG}@g" "${filename}"
sed_runner "s@rapidsai/devcontainers/features/rapids-build-utils:[0-9.]*@rapidsai/devcontainers/features/rapids-build-utils:${NEXT_RAPIDS_SHORT_TAG_PEP4400}@" "${filename}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sed_runner "s@rapidsai/devcontainers/features/rapids-build-utils:[0-9.]*@rapidsai/devcontainers/features/rapids-build-utils:${NEXT_RAPIDS_SHORT_TAG_PEP4400}@" "${filename}"
sed_runner "s@rapidsai/devcontainers/features/rapids-build-utils:[0-9.]*@rapidsai/devcontainers/features/rapids-build-utils:${NEXT_RAPIDS_SHORT_TAG_PEP440}@" "${filename}"

Copy link
Member Author

Choose a reason for hiding this comment

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

blegh, this mistake shows that the way I tested in #363 (comment) wasn't great. There I was checking for "nothing was missed", should have also manually scanned the diff for "all the updates look right".

Fixed in 3c774c2

Sorry for making you catch these things in review, I know it's annoying :/

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! Thanks for your work on this PR. :)

@jameslamb jameslamb requested a review from bdice February 3, 2025 16:47
@bdice
Copy link
Contributor

bdice commented Feb 4, 2025

/merge

@rapids-bot rapids-bot bot merged commit 7462e3a into rapidsai:branch-0.43 Feb 4, 2025
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding devcontainers for UCXX
3 participants