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

Distribute libcuspatial wheels #1450

Merged
merged 16 commits into from
Sep 4, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Aug 30, 2024

Description

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

how I calculated those (click me)
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}

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

Checklist

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

@jameslamb jameslamb added 2 - In Progress Currenty a work in progress 5 - DO NOT MERGE Hold off on merging; see PR for details improvement Improvement / enhancement to an existing function non-breaking Non-breaking change libcuspatial Relates to the cuSpatial C++ library labels Aug 30, 2024
@github-actions github-actions bot added cmake Related to CMake code or build configuration Python Related to Python code ci labels Aug 30, 2024
@jameslamb jameslamb removed the 2 - In Progress Currenty a work in progress label Aug 30, 2024
@jameslamb jameslamb changed the title WIP: [NOT READY FOR REVIEW] Distribute libcuspatial wheels Distribute libcuspatial wheels Sep 3, 2024
@jameslamb jameslamb added 3 - Ready for Review Ready for review by team and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Sep 3, 2024
@jameslamb jameslamb marked this pull request as ready for review September 3, 2024 14:15
ci/build_wheel.sh Show resolved Hide resolved
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.

Re-approving.

@harrism
Copy link
Member

harrism commented Sep 3, 2024

My review is requested but I have no expertise on wheels/pip.

@harrism harrism removed their request for review September 3, 2024 23:53
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Approving cmake changes.

ci/build_wheel.sh Show resolved Hide resolved
cpp/cmake/thirdparty/get_ranger.cmake Show resolved Hide resolved
python/libcuspatial/CMakeLists.txt Outdated Show resolved Hide resolved
python/libcuspatial/CMakeLists.txt Show resolved Hide resolved
set(CUSPATIAL_USE_CUDF_STATIC OFF)
set(CUSPATIAL_EXCLUDE_CUDF_FROM_ALL ON)

set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/lib)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remind me why we're setting this? Is it strictly necessary? Why aren't the install rules that come from add_subdirectory sufficient? If we do have to set this variable, why do we have to set it but also do a separate install below? It seems like either one or the other should be sufficient. I know this code is in the libcudf wheel now as well, but I don't understand why.

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 copied this in here from libcudf because I think I misunderstood the purpose there.

There, this is used to ensure all the nvcomp libraries and libcudf.so all get put together into the same place: https://github.com/rapidsai/cudf/blob/26091a44b3dbf0f56fc0dfc5f081077f2d00681f/python/libcudf/CMakeLists.txt#L50-L53

That isn't necessary here, and you're right that the install rules from the add_subdirectory() are sufficient. Removed this stuff in fb307ba

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that the libcudf install rules are also a little overspecified now. Simultaneously setting both the CMake var and doing the install of cudf in particular seems redundant. The nvcomp installation might be fine as is. We can revisit those later, though.

@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

Mostly confused by the CMake installation, otherwise everything else looks OK to me.

@jameslamb jameslamb requested a review from vyasr September 4, 2024 16:37
@jameslamb
Copy link
Member Author

I've merged rapidsai/devcontainers#387 and restarted the pip devcontainer CI job here. Will merge this if it passes.

build link: https://github.com/rapidsai/cuspatial/actions/runs/10705830747/job/29698846144?pr=1450

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 0e9d36c into rapidsai:branch-24.10 Sep 4, 2024
64 checks passed
@jameslamb jameslamb deleted the libcuspatial-wheel branch September 4, 2024 23:40
rapids-bot bot pushed a commit that referenced this pull request Sep 6, 2024
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
rapids-bot bot pushed a commit that referenced this pull request Sep 9, 2024
Follow-up to #1448. This proposes some `dependencies.yaml` changes that I noticed while working on adding `libcuspatial` wheels in #1450.

I've left inline comments with more details, but in short:

* not including any cuspatial projects in the conda environment files checked into this repo (which are intended to be used to set up a development environment)
* removing an unnecessary conda-only list of dependencies from pyproject.toml-specific config for `cuproj`
* introduction of `depends_on_libcuspatial` and `depends_on_cuproj` lists in `dependencies.yaml`, to reduce duplication

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

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

URL: #1451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team ci cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change Python Related to Python code
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants