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.

Sign up for GitHub

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

Jump to bottom

add devcontainers #79

Merged
merged 10 commits into from
Dec 3, 2024
Merged

add devcontainers #79

merged 10 commits into from
Dec 3, 2024

Conversation

jameslamb
Copy link
Member

Follow-up to these PRs:

Proposes adding devcontainers and a devcontainers CI job to the repo.

Notes for Reviewers

Benefits of these changes

Similar to rapidsai/nx-cugraph#25

How I made these changes

Copied the .devcontainer/ directory from https://github.com/rapidsai/cugraph, then just changed cugraph references to cugraph-gnn.

How I tested this

Tested the update-version.sh changes like this:

./ci/release/update-version.sh '25.04.00'
git grep -E '25\.[0-9]+'

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Nov 25, 2024
@jameslamb
Copy link
Member Author

Think this will need rapidsai/devcontainers#423 before it can pass.

jameslamb
jameslamb commented Nov 26, 2024
View reviewed changes
dependencies.yaml
@@ -262,6 +262,33 @@ dependencies:
packages:
- &cmake_ver cmake>=3.26.4,!=3.30.0
- ninja
specific:
Copy link
Member Author

@jameslamb jameslamb Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Copied this compiler block directly from cugraph: https://github.com/rapidsai/cugraph/blob/3d681cc9e6d85336822ccb6faf0d1cba5f759ead/dependencies.yaml#L304-L330

To try to fix this error in the conda devcontainer build:

  CMake Error at /home/coder/.conda/envs/rapids/share/cmake-3.31/Modules/CMakeDetermineCXXCompiler.cmake:48 (message):
    Could not find compiler set in environment variable CXX:
  
    /usr/bin/g++.
  
  Call Stack (most recent call first):
    CMakeLists.txt:39 (project)

(build link)

Most other RAPIDS repos I checked have something like this, and I found offline conversations where it was suggested that the conda devcontainer builds expect the conda environment to provide compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a good fix.

@jameslamb jameslamb changed the title WIP: add devcontainers add devcontainers Nov 26, 2024
@jameslamb jameslamb marked this pull request as ready for review November 26, 2024 17:55
@jameslamb jameslamb requested review from a team as code owners November 26, 2024 17:55
@jameslamb jameslamb requested a review from bdice November 26, 2024 17:55
bdice
bdice approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

One question about ucx versions. Otherwise approved.

.devcontainer/cuda11.8-pip/devcontainer.json Outdated
"args": {
"CUDA": "11.8",
"PYTHON_PACKAGE_MANAGER": "pip",
"BASE": "rapidsai/devcontainers:25.02-cpp-cuda11.8-ucx1.15.0-openmpi-ubuntu22.04"
Copy link
Contributor

Choose a reason for hiding this comment

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

This uses ucx 1.15 but the CUDA 12 container uses ucx 1.17. Is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great catch! I think it should be 1.17 here too.

I copied these directly from cugraph, looks like that was an oversight in rapidsai/cugraph#4562 (comment).

Let's try 1.17 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to fix this in cugraph and nx-cugraph. https://github.com/search?q=org%3Arapidsai%20ucx1.15&type=code

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I'll do that alongside this other UCX-related devcontainers change I'm about to go make: rapidsai/devcontainers#421 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a commit adding those changes (setting RAPIDS_LIBUCX_PREFER_SYSTEM_LIBRARY=true in the pip devcontainers) here.

And manually cancelled CI because it'll fail until rapidsai/raft#2513 is done.

dependencies.yaml
@@ -262,6 +262,33 @@ dependencies:
packages:
- &cmake_ver cmake>=3.26.4,!=3.30.0
- ninja
specific:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a good fix.

@jameslamb
Copy link
Member Author

wholegraph wheel builds are failing like this:

  /__w/cugraph-gnn/cugraph-gnn/python/pylibwholegraph/build/cp311-cp311-linux_aarch64/_deps/raft-src/cpp/include/raft/core/detail/callback_sink.hpp:22:10: fatal error: spdlog/common.h: No such file or directory
     22 | #include <spdlog/common.h>
        |          ^~~~~~~~~~~~~~~~~

(build link)

I think that's a result of the ongoing logging changes, and should be fixed by rapidsai/raft#2513.

This was referenced Nov 27, 2024
Closed
Merged
Merged
Merged
rapids-bot bot pushed a commit to rapidsai/nx-cugraph that referenced this pull request Dec 2, 2024
@jameslamb
prefer system install of UCX in devcontainers, update outdated RAPIDS…
1cd7cc6 
… references (#53)

Contributes to rapidsai/build-planning#118

Proposes the following changes for pip devcontainers:

* use UCX 1.17 (ref: rapidsai/cugraph-gnn#79 (comment))
* prefer system installation of ucx to the one provided by the `libucx-cu{11,12}` wheels (ref: rapidsai/devcontainers#421 (comment))

And some other related changes noticed while doing that:

* update lingering `24.*` references to `25.02`
* fix `update-version.sh` so those will be correctly updated in future releases

Similar to rapidsai/cugraph#4792

## Notes for Reviewers

### How I tested this

Relying on CI for most things. But for `update-version.sh`, tested like this:

```shell
./ci/release/update-version.sh '25.02.00'
git grep -E '24\.'
```

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

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

URL: #53
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request Dec 2, 2024
@jameslamb
pip devcontainers: use UCX 1.17, prefer system installation (#4792)
94c6ee4 
Contributes to rapidsai/build-planning#118

Proposes the following changes for pip devcontainers:

* use UCX 1.17 (ref: rapidsai/cugraph-gnn#79 (comment))
* prefer system installation of ucx to the one provided by the `libucx-cu{11,12}` wheels (ref: rapidsai/devcontainers#421 (comment))

And one other related change:

* skip most CI when a PR only changes files in the `.devcontainer/` directory (this was incorrectly spelled `.devcontainers/` before)

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

Approvers:
  - Jake Awe (https://github.com/AyodeAwe)

URL: #4792
@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 42c16fe into rapidsai:branch-25.02 Dec 3, 2024
80 checks passed
@jameslamb jameslamb deleted the add-devcontainers branch December 3, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

Assignees
No one assigned
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jameslamb @bdice