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 manifest entry for libcuspatial wheels #387

Merged
merged 4 commits into from
Sep 4, 2024
Merged

add manifest entry for libcuspatial wheels #387

merged 4 commits into from
Sep 4, 2024

Conversation

jameslamb
Copy link
Member

I'm working on adding libcuspatial wheels in rapidsai/cuspatial#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 #271 (comment).

Rebuilt the CUDA 12.5 pip devcontainer, with only cudf and cuspatial repos mounted in, and with cuspatial pointed at the branch from rapidsai/cuspatial#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 rapidsai/cuspatial#1450 is close to being merged, let's merge this PR and then restart the devcontainers CI job there.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Aug 30, 2024
@jameslamb jameslamb requested a review from a team as a code owner August 30, 2024 22:29
@jameslamb jameslamb requested review from KyleFromNVIDIA and removed request for a team August 30, 2024 22:29
@jameslamb jameslamb mentioned this pull request Aug 30, 2024
Merged
3 tasks
@trxcllnt
Copy link
Collaborator

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

@jameslamb
Copy link
Member Author

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.

@jameslamb jameslamb requested a review from bdice September 3, 2024 16:07
@trxcllnt
Copy link
Collaborator

trxcllnt commented Sep 3, 2024

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
Copy link
Member Author

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

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

@jameslamb
Copy link
Member Author

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 rapidsai/cuspatial#1450.

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

@jameslamb jameslamb merged commit f9155e8 into rapidsai:branch-24.10 Sep 4, 2024
22 of 26 checks passed
@jameslamb jameslamb deleted the libcuspatial-wheels branch September 4, 2024 23:13
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Sep 4, 2024
@jameslamb
Distribute libcuspatial wheels (#1450)
0e9d36c 
Contributes to rapidsai/build-planning#33

Adds `libcuspatial` wheels, and switches `cuspatial` wheels to using them.

## Notes for Reviewers

### Benefits of these changes

Faster CI runs and smaller total footprint on package repositories (because now `libcuspatial` no longer needs to be compiled once per Python version).

Smaller `cuspatial` wheels.

| whee.  l          | size (before) | size (this PR)    |
|:-------------:|-------------:|----------------:|
| `libcuspatial` |   ---              |      17.0M            |
| `cuspatial`.   |     21.0M        |        4.1M            |
| `cuproj `        |      0.9M        |         0.9M           |
|**TOTAL**      |  **21.9M**    |      **22.0M**     |

*NOTES: size = compressed, "before" = 2024-09-02 nightlies (1544e7b), CUDA = 12, Python = 3.11*

<details><summary>how I calculated those (click me)</summary>

```shell
docker run \
    --rm \
    -v $(pwd):/opt/work:ro \
    -w /opt/work \
    --network host \
    --env RAPIDS_NIGHTLY_DATE=2024-09-02 \
    --env RAPIDS_NIGHTLY_SHA=1544e7b \
    --env RAPIDS_PR_NUMBER=1450 \
    --env RAPIDS_PY_CUDA_SUFFIX=cu12 \
    --env RAPIDS_REPOSITORY=rapidsai/cuspatial \
    --env WHEEL_DIR_BEFORE=/tmp/wheels-before \
    --env WHEEL_DIR_AFTER=/tmp/wheels-after \
    -it rapidsai/ci-wheel:cuda12.5.1-rockylinux8-py3.11 \
    bash

mkdir -p "${WHEEL_DIR_BEFORE}"
mkdir -p "${WHEEL_DIR_AFTER}"

py_projects=(
    cuspatial
    cuproj
)

for project in "${py_projects[@]}"; do
    # before
    RAPIDS_BUILD_TYPE=nightly \
    RAPIDS_PY_WHEEL_NAME="${project}_${RAPIDS_PY_CUDA_SUFFIX}" \
    RAPIDS_REF_NAME="branch-24.10" \
    RAPIDS_SHA=${RAPIDS_NIGHTLY_SHA} \
        rapids-download-wheels-from-s3 python "${WHEEL_DIR_BEFORE}"

    # after
    RAPIDS_BUILD_TYPE=pull-request \
    RAPIDS_PY_WHEEL_NAME="${project}_${RAPIDS_PY_CUDA_SUFFIX}" \
    RAPIDS_REF_NAME="pull-request/${RAPIDS_PR_NUMBER}" \
        rapids-download-wheels-from-s3 python "${WHEEL_DIR_AFTER}"
done

# after
RAPIDS_BUILD_TYPE=pull-request \
RAPIDS_PY_WHEEL_NAME="libcuspatial_${RAPIDS_PY_CUDA_SUFFIX}" \
RAPIDS_REF_NAME="pull-request/${RAPIDS_PR_NUMBER}" \
    rapids-download-wheels-from-s3 cpp "${WHEEL_DIR_AFTER}"

du -sh ${WHEEL_DIR_BEFORE}/*
du -sh ${WHEEL_DIR_BEFORE}
du -sh ${WHEEL_DIR_AFTER}/*
du -sh ${WHEEL_DIR_AFTER}
```

</details>

### devcontainers job?

Once this PR is close to ready, let's merge the devcontainers PR and then re-run the devcontainers CI here.

devcontainers PR: rapidsai/devcontainers#387

### `rapids-metadata` changes?

Not necessary, `libcuspatial` is already there: https://github.com/rapidsai/rapids-metadata/blob/9b6307e708511cd9a1990d8bb36606df53bc9e1b/src/rapids_metadata/__init__.py#L89

#

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

Approvers:
  - Bradley Dice (https://github.com/bdice)
  - Mark Harris (https://github.com/harrism)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #1450
This was referenced Sep 5, 2024
Closed
Merged
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Sep 6, 2024
@jameslamb
re-enable mixing libcuspatial wheels with libcudf conda packages (#1456)
9385e49 
Fixes #1455

devcontainer conda CI jobs are failing in this project because of the following mix of characteristics for thoes jobs:

* all build and runtime dependencies for `libcuspatial`, `cuspatial`, `cuproj` are installed via conda
* `libcuspatial`, `cuspatial`, and `cuproj` wheels are then built with `pip install -e --no-deps --no-build-isolation`
* `import libcuspatial` results in unconditionally running `import libcudf`
* `libcudf` is provided by the `libcudf` **conda** package, which does not have any Python modules, so that import fails

This fixes that, and restores the ability to mix a `pip install`'d `cuspatial` / `cuproj` with a `conda`-installed `libcudf`.

## Notes for Reviewers

### How did CI not catch this before?

When rapidsai/devcontainers#387 was merged, I only re-ran the **pip** devcontainers CI job on #1450.

#

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

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

URL: #1456
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

@KyleFromNVIDIA KyleFromNVIDIA Awaiting requested review from KyleFromNVIDIA KyleFromNVIDIA is a code owner automatically assigned from rapidsai/packaging-codeowners

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

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.
3 participants
@jameslamb @trxcllnt @bdice