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

wheels: dynamically load libcudf.so from libcudf wheel #1447

Merged
merged 6 commits into from
Aug 29, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Aug 28, 2024

Description

Contributes to rapidsai/build-planning#33.

Proposes the following for cuspatial wheels:

  • add build and runtime dependencies on libcudf wheels
  • stop vendoring copies of libcudf.so, libnvcomp.so, libnvcomp_bitcomp.so, and libnvcomp_gdeflate.so
    • (load libcudf.so dynamically at runtime instead)

And other related changes for development/CI:

  • combine all pip install calls into 1 in wheel-testing scripts
  • dependencies.yaml changes:
    • more use of YAML anchors = less duplication
    • use dedicated depends_on_librmm and depends_on_libcudf groups
  • explicitly pass a package type to gha-tools wheel uploading/downloading scripts

Notes for Reviewers

Benefits of these changes

Unblocks CI in this repo (ref: #1444 (comment), #1441 (comment)).

Reduces wheel sizes for cuspatial wheels by about 125MB 😁

wheel size (before) size (this PR)
cuspatial 146.0M 21M
cuproj 0.9M 0.9M
TOTAL 146.9M 21.9M

NOTES: size = compressed, "before" = 2024-08-21 nightlies (c60bd4d), CUDA = 12, Python = 3.11

how I calculated those (click me)
# note: 2024-08-21 because that was the most recent date with
#           successfully-built cuspatial nightlies
#
docker run \
    --rm \
    -v $(pwd):/opt/work:ro \
    -w /opt/work \
    --network host \
    --env RAPIDS_NIGHTLY_DATE=2024-08-21 \
    --env RAPIDS_NIGHTLY_SHA=c60bd4d \
    --env RAPIDS_PR_NUMBER=1447 \
    --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

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

Reduces the amount of additional work required to start shipping libcuspatial wheels.

Background

This is part of ongoing work towards packaging libcuspatial as a wheel.

relevant prior work:

How I tested this

Confirmed in local builds and CI logs that cudf is being found, not built, in cuspatial builds.

-- CPM: Using local package cudf@24.10.0

(build link)

Built cuspatial wheels locally and ran all the unit tests, without issue.

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 improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 28, 2024
@github-actions github-actions bot added cmake Related to CMake code or build configuration Python Related to Python code ci labels Aug 28, 2024
@jameslamb jameslamb changed the title WIP: [NOT READY FOR REVIEW] wheels: dynamically load libcudf.so from libcudf wheel wheels: dynamically load libcudf.so from libcudf wheel Aug 28, 2024
@jameslamb jameslamb added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Aug 28, 2024
@jameslamb jameslamb marked this pull request as ready for review August 28, 2024 20:37
@jameslamb jameslamb requested review from a team as code owners August 28, 2024 20:37
@@ -134,6 +136,8 @@ requires = [
"cmake>=3.26.4,!=3.30.0",
"cudf==24.10.*,>=0.0.0a0",
"cython>=3.0.0",
"libcudf==24.10.*,>=0.0.0a0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Putting this somewhere on the diff so it can be threaded.

Just realized I may have missed one detail that wouldn't be caught in CI.

@vyasr do we want something like https://github.com/rapidsai/cudf/blob/925530afe8178b7e788ea1a8d4df4c0eb4d042dc/python/cudf/cudf/__init__.py#L3-L11 near the top of cuspatial's top-level __init__.py?

I think the answer is yes, so that other libcudf.so lying around in the environment aren't loaded first by some other import.

Copy link
Member

Choose a reason for hiding this comment

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

Reading the linked code (and where it lives), it seems just doing import cudf would be sufficient

Though please let me know if I'm missing something

Copy link
Contributor

Choose a reason for hiding this comment

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

John is right that the import should be sufficient for now. When you create the libcuspatial wheel in the follow-up, though, you'll end up needing to load libcudf there. You can see how I did that in the cugraph PR for raft/cugraph-ops.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, added an import cudf in fe7df5d and will adjust when I add libcuspatial.

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.

All seems fine to me. Please double-check ci/release/update-version.sh. It may need libcudf and librmm listings.

@jameslamb
Copy link
Member Author

jameslamb commented Aug 28, 2024

Please double-check ci/release/update-version.sh. It may need libcudf and librmm listings.

libcudf
libcuspatial
libcuspatial-tests
librmm

Yep I don't think any changes are needed there. It already had them because the conda recipes reference those packages.


# Install additional dependencies
# install build dependencies for fiona
apt update
DEBIAN_FRONTEND=noninteractive apt install -y --no-install-recommends libgdal-dev
Copy link
Member

@jakirkham jakirkham Aug 28, 2024

Choose a reason for hiding this comment

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

Fiona appears to ship binary wheels

Wonder if this is only needed for some specific case (didn't see Linux ARM packages for example)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the question why we install fiona with --no-binary? If so, I do think arm is one of the reasons, but it may not be the only one. @trxcllnt might remember more.

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 don't know the original reason, not having ARM packages seems like a likely candidate.

This PR doesn't change anything about it (it's just showing up in the diff because I'm moving other install-related code around), so I'm going to treat this discussion as non-blocking.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Couple of notes, but I don't need to review again.

@@ -5,6 +5,7 @@ set -euo pipefail

package_name=$1
package_dir=$2
package_type=$3
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is in anticipation of the C++ wheel? I have a very minor preference for sticking that change in the subsequent PR to make the reason more obvious, but now that it's done let's leave it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes, but I think it'd be desirable even if we weren't adding a C++ wheel in this repo.

In my mind, rapidsai/shared-workflows#209 and rapidsai/gha-tools#105 shipped with package-type defaulting to python just to avoid having to do updates across all the repos to make progress there.

But that it'd be better to be explicit everywhere (like @bdice mentioned in rapidsai/cudf#15483 (comment)), to avoid issues like rapidsai/cudf#16650.

Anyway yeah, we are adding C++ wheels here very soon so I'll leave this change in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: I wasn't meaning the passing of python to the GHA tool, just the passing of an argument to our script, i.e. we could have hardcoded python in this script and added the package_type parameter in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

ohhhh ok yeah got it, fair


# Install additional dependencies
# install build dependencies for fiona
apt update
DEBIAN_FRONTEND=noninteractive apt install -y --no-install-recommends libgdal-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the question why we install fiona with --no-binary? If so, I do think arm is one of the reasons, but it may not be the only one. @trxcllnt might remember more.

install_aliased_imported_targets(
TARGETS cuspatial arrow_shared nvcomp::nvcomp nvcomp::nvcomp_gdeflate nvcomp::nvcomp_bitcomp
TARGETS cuspatial
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, now that we've removed all of these targets we should just remove the WheelHelpers altogether and just do a normal CMake install.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! Done in fe7df5d

@jameslamb jameslamb requested a review from a team as a code owner August 29, 2024 15:14
Copy link
Contributor

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Based on the above discussions about importing cudf, Python changes LGTM

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit b43a32e into rapidsai:branch-24.10 Aug 29, 2024
58 checks passed
@jameslamb jameslamb deleted the use-libcudf branch August 29, 2024 15:55
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 non-breaking Non-breaking change Python Related to Python code
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants