rapidsai / devcontainers

18 stars 28 forks source link

add manifest entry for libcuspatial wheels #387

Closed jameslamb closed 2 months ago

jameslamb commented 2 months ago

I'm working on adding libcuspatial wheels in https://github.com/rapidsai/cuspatial/pull/1450.

This proposes adding a manifest entry for rapids-build-utils, to build them in devcontainers.

Notes for Reviewers

How I tested this

Similar to https://github.com/rapidsai/devcontainers/pull/271#issuecomment-2307290687.

Rebuilt the CUDA 12.5 pip devcontainer, with only cudf and cuspatial repos mounted in, and with cuspatial pointed at the branch from https://github.com/rapidsai/cuspatial/pull/1450.

# build libcudf.so + all the cudf Python wheels
build-cudf -j

# build libcuspatial.so + all the cuspatial Python wheels
build-cuspatial -j

# test that 'libcuspatial' is importable
python -c "import libcuspatial; l = libcuspatial.load_library(); print(l)"
# None

# test that 'cuspatial' and 'cuproj' are importable
python -c "import cuspatial;import cuproj"

# test that cudf actually works
pushd ./cuspatial/python/cuspatial/cuspatial
python -m pytest \
  --cache-clear \
  --numprocesses=8 \
  --dist=worksteal \
  tests/

All worked 😁

Merge order?

Once https://github.com/rapidsai/cuspatial/pull/1450 is close to being merged, let's merge this PR and then restart the devcontainers CI job there.

trxcllnt commented 2 months ago

Need to bump the feature version, but other than that LGTM

jameslamb commented 2 months ago

Ah ok, bumped the feature version.

I was thinking that because I didn't see a version bump in #271, maybe matrix-only changes didn't need a version bump. Guess it was just an oversight there.

trxcllnt commented 2 months ago

Yeah changes to matrix.yaml don't require a feature bump, because they're not part of any feature. The manifest.yaml is part of the rapidsai/devcontainers/features/rapids-build-utils feature though, so that's why that feature version needs to be bumped.

jameslamb commented 2 months ago

🙈 I meant to say "manifest", not "matrix".

But ok yes, it sounds like not bumping the feature version in #271 was an oversight.

jameslamb commented 2 months ago

Just pushed one more change... I realized that -DFIND_CUSPATIAL_CPP no longer needs to be passed through to cuspatial wheel builds. That option is removed entirely in https://github.com/rapidsai/cuspatial/pull/1450.

Going to merge this and then hopefully https://github.com/rapidsai/cuspatial/pull/1450. One has to be admin-merged for the other's CI to pass.