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

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

Merged
merged 6 commits into from
Aug 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c3f4f9
add dependency on libcudf
jameslamb Aug 28, 2024
b429e24
remove unnecessary nvcomp installation
jameslamb Aug 28, 2024
5a81fdb
revert inadvertent cuproj changes
jameslamb Aug 28, 2024
18f7dbf
one more cuproj reversion
jameslamb Aug 28, 2024
a5b6e4e
add package-type to wheels-publish jobs
jameslamb Aug 28, 2024
fe7df5d
remove WheelHelpers, add cudf import
jameslamb Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ jobs:
sha: ${{ inputs.sha }}
date: ${{ inputs.date }}
package-name: cuspatial
package-type: python
wheel-build-cuproj:
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/wheels-build.yaml@branch-24.10
Expand All @@ -104,3 +105,4 @@ jobs:
sha: ${{ inputs.sha }}
date: ${{ inputs.date }}
package-name: cuproj
package-type: python
8 changes: 6 additions & 2 deletions ci/build_wheel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


source rapids-configure-sccache
source rapids-date-string
Expand All @@ -18,6 +19,9 @@ cd "${package_dir}"
python -m pip wheel . -w dist -vvv --no-deps --disable-pip-version-check

mkdir -p final_dist
python -m auditwheel repair -w final_dist dist/*
python -m auditwheel repair \
--exclude libcudf.so \
-w final_dist \
dist/*

RAPIDS_PY_WHEEL_NAME="${package_name}_${RAPIDS_PY_CUDA_SUFFIX}" rapids-upload-wheels-to-s3 final_dist
RAPIDS_PY_WHEEL_NAME="${package_name}_${RAPIDS_PY_CUDA_SUFFIX}" rapids-upload-wheels-to-s3 "${package_type}" final_dist
4 changes: 2 additions & 2 deletions ci/build_wheel_cuproj.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/bin/bash
# Copyright (c) 2023, NVIDIA CORPORATION.
# Copyright (c) 2023-2024, NVIDIA CORPORATION.

set -euo pipefail

ci/build_wheel.sh cuproj python/cuproj
ci/build_wheel.sh cuproj python/cuproj python
6 changes: 2 additions & 4 deletions ci/build_wheel_cuspatial.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
#!/bin/bash
# Copyright (c) 2023, NVIDIA CORPORATION.
# Copyright (c) 2023-2024, NVIDIA CORPORATION.

set -euo pipefail

export SKBUILD_CMAKE_ARGS="-DUSE_LIBARROW_FROM_PYARROW=ON"

ci/build_wheel.sh cuspatial python/cuspatial
ci/build_wheel.sh cuspatial python/cuspatial python
16 changes: 9 additions & 7 deletions ci/test_wheel_cuproj.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,21 @@ set -eou pipefail

mkdir -p ./dist
RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})"
RAPIDS_PY_WHEEL_NAME="cuproj_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 ./dist

# Install additional dependencies
# install build dependencies for fiona
apt update
DEBIAN_FRONTEND=noninteractive apt install -y --no-install-recommends libgdal-dev
python -m pip install --no-binary fiona 'fiona>=1.8.19,<1.9'

# Download the cuspatial built in the previous step
RAPIDS_PY_WHEEL_NAME="cuspatial_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 ./local-cuspatial-dep
python -m pip install --no-deps ./local-cuspatial-dep/cuspatial*.whl
# Download the cuproj and cuspatial built in the previous step
RAPIDS_PY_WHEEL_NAME="cuproj_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 python ./dist
RAPIDS_PY_WHEEL_NAME="cuspatial_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 python ./dist

# echo to expand wildcard before adding `[extra]` requires for pip
python -m pip install $(echo ./dist/cuproj*.whl)[test]
python -m pip install \
--no-binary 'fiona' \
"$(echo ./dist/cuspatial*.whl)" \
"$(echo ./dist/cuproj*.whl)[test]" \
'fiona>=1.8.19,<1.9'

rapids-logger "pytest cuproj"
pushd python/cuproj/cuproj
Expand Down
12 changes: 8 additions & 4 deletions ci/test_wheel_cuspatial.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@ set -eou pipefail

mkdir -p ./dist
RAPIDS_PY_CUDA_SUFFIX="$(rapids-wheel-ctk-name-gen ${RAPIDS_CUDA_VERSION})"
RAPIDS_PY_WHEEL_NAME="cuspatial_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 ./dist

# 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 •
edited
Loading

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.

python -m pip install --no-binary fiona 'fiona>=1.8.19,<1.9'

# Download the cuspatial built in the previous step
RAPIDS_PY_WHEEL_NAME="cuspatial_${RAPIDS_PY_CUDA_SUFFIX}" rapids-download-wheels-from-s3 python ./dist

# echo to expand wildcard before adding `[extra]` requires for pip
python -m pip install $(echo ./dist/cuspatial*.whl)[test]
python -m pip install \
--no-binary 'fiona' \
"$(echo ./dist/cuspatial*.whl)[test]" \
'fiona>=1.8.19,<1.9'

rapids-logger "pytest cuspatial"
pushd python/cuspatial/cuspatial
Expand Down
69 changes: 61 additions & 8 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ files:
- depends_on_cudf
- depends_on_cuml
- depends_on_cupy
- depends_on_libcudf
- depends_on_librmm
- rapids_build_skbuild
- run_python_cuspatial
- test_libcuspatial
Expand Down Expand Up @@ -77,14 +79,17 @@ files:
- build_cpp
- build_python
- build_wheels
- depends_on_libcudf
- depends_on_librmm
py_run_cuspatial:
output: [pyproject]
pyproject_dir: python/cuspatial
extras:
table: project
includes:
- depends_on_rmm
- depends_on_cudf
- depends_on_libcudf
- depends_on_rmm
- run_python_cuspatial
py_test_cuspatial:
output: [pyproject]
Expand Down Expand Up @@ -141,14 +146,12 @@ dependencies:
common:
- output_types: [conda, requirements, pyproject]
packages:
- ninja
- cmake>=3.26.4,!=3.30.0
- &ninja ninja
- &cmake cmake>=3.26.4,!=3.30.0
- output_types: conda
packages:
- c-compiler
- cxx-compiler
- libcudf==24.10.*,>=0.0.0a0
- librmm==24.10.*,>=0.0.0a0
- proj
- sqlite
specific:
Expand Down Expand Up @@ -184,13 +187,13 @@ dependencies:
common:
- output_types: [conda, requirements, pyproject]
packages:
- ninja
- cmake>=3.26.4,!=3.30.0
- *ninja
- *cmake
- output_types: conda
packages:
- c-compiler
- cxx-compiler
- librmm==24.10.*,>=0.0.0a0
- &librmm_unsuffixed librmm==24.10.*,>=0.0.0a0
- proj
- sqlite
specific:
Expand Down Expand Up @@ -443,6 +446,31 @@ dependencies:
- pylibcudf-cu11==24.10.*,>=0.0.0a0
- {matrix: null, packages: [*cudf_unsuffixed]}

depends_on_libcudf:
common:
- output_types: conda
packages:
- &libcudf_unsuffixed libcudf==24.10.*,>=0.0.0a0
- output_types: requirements
packages:
# pip recognizes the index as a global option for the requirements.txt file
- --extra-index-url=https://pypi.nvidia.com
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
specific:
- output_types: [requirements, pyproject]
matrices:
- matrix:
cuda: "12.*"
cuda_suffixed: "true"
packages:
- libcudf-cu12==24.10.*,>=0.0.0a0
- matrix:
cuda: "11.*"
cuda_suffixed: "true"
packages:
- libcudf-cu11==24.10.*,>=0.0.0a0
- {matrix: null, packages: [*libcudf_unsuffixed]}

depends_on_cuml:
common:
- output_types: conda
Expand Down Expand Up @@ -523,3 +551,28 @@ dependencies:
- libcuspatial==24.10.*,>=0.0.0a0
- cuspatial==24.10.*,>=0.0.0a0
- cuproj==24.10.*,>=0.0.0a0

depends_on_librmm:
common:
- output_types: conda
packages:
- *librmm_unsuffixed
- output_types: requirements
packages:
# pip recognizes the index as a global option for the requirements.txt file
- --extra-index-url=https://pypi.nvidia.com
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
specific:
- output_types: [requirements, pyproject]
matrices:
- matrix:
cuda: "12.*"
cuda_suffixed: "true"
packages:
- librmm-cu12==24.10.*,>=0.0.0a0
- matrix:
cuda: "11.*"
cuda_suffixed: "true"
packages:
- librmm-cu11==24.10.*,>=0.0.0a0
- {matrix: null, packages: [*librmm_unsuffixed]}
6 changes: 1 addition & 5 deletions python/cuspatial/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,8 @@ if(NOT cuspatial_FOUND)

set(cython_lib_dir cuspatial)
include(cmake/Modules/WheelHelpers.cmake)
# TODO: This install is currently overzealous. We should only install the libraries that are
# downloaded by CPM during the build, not libraries that were found on the system. However, in
# practice this would only be a problem if libcudf was not found but some of the
# dependencies were, and we have no real use cases where that happens.
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

DESTINATION ${cython_lib_dir}
)
endif()
Expand Down
4 changes: 4 additions & 0 deletions python/cuspatial/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ requires-python = ">=3.10"
dependencies = [
"cudf==24.10.*,>=0.0.0a0",
"geopandas>=0.11.0",
"libcudf==24.10.*,>=0.0.0a0",
"numpy>=1.23,<2.0a0",
"rmm==24.10.*,>=0.0.0a0",
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit ../../dependencies.yaml and run `rapids-dependency-file-generator`.
Expand Down Expand Up @@ -71,6 +72,7 @@ known_dask = [
known_rapids = [
"rmm",
"cudf",
"libcudf",
"pylibcudf",
]
known_first_party = [
Expand Down Expand Up @@ -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.

"librmm==24.10.*,>=0.0.0a0",
"ninja",
"rmm==24.10.*,>=0.0.0a0",
"wheel",
Expand Down
Loading