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

Create and promote library aliases in libcudf installations #7734

Merged
merged 12 commits into from
Mar 29, 2021

Conversation

trxcllnt
Copy link
Contributor

@trxcllnt trxcllnt commented Mar 26, 2021

This PR ensures all cudf::* library aliases are created and promoted to IMPORTED_GLOBAL when find_package(cudf) finds cudf in a local build directory.

This PR shouldn't affect CI or the targets you'd see when libcudf is installed (e.g. by conda), only local source builds.

edit: This now fixes cudf::* alias targets for the libcudf installations too, needed by rapidsai/cuspatial#365.

Validation method:

$ docker run --rm -it \
  -w /tmp/findpackagecudf \
  -v "/tmp/findpackagecudf:/tmp/findpackagecudf" \
  gpuci/miniconda-cuda:10.2-devel-ubuntu18.04 bash

# Set up mamba environment
conda install -y -n base -c conda-forge mamba
mamba update -y -n base -c defaults conda && mamba update -y -n base -c conda-forge mamba
mamba install -y -n base -c conda-forge -c rapidsai-nightly \
    git gtest gmock ninja cmake=3.18 gdal=3.0.2 boost-cpp=1.72.0 cudatoolkit=10.2 libcudf=0.19

# Copy changes in this PR (from the host) to container's /opt/conda/lib/cmake/cudf
# cmake --install $CUDF_ROOT --prefix $CUDF_ROOT/local-install
# docker cp $CUDF_ROOT/local-install/lib/cmake/cudf frosty_agnesi:/opt/conda/lib/cmake/

# Clone cuspatial
git clone https://github.com/trxcllnt/cuspatial.git && cd cuspatial && git checkout fix/cmake-exports

# Configure cuspatial
rm -rf cpp/build && mkdir -p cpp/build \
 && cmake -GNinja -B cpp/build -S cpp \
    -DBUILD_TESTS=ON -DBUILD_BENCHMARKS=ON -DCMAKE_CUDA_ARCHITECTURES=

@trxcllnt trxcllnt requested a review from robertmaynard March 26, 2021 01:14
@trxcllnt trxcllnt requested a review from a team as a code owner March 26, 2021 01:14
@trxcllnt trxcllnt added the non-breaking Non-breaking change label Mar 26, 2021
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Mar 26, 2021
@trxcllnt trxcllnt added improvement Improvement / enhancement to an existing function and removed libcudf Affects libcudf (C++/CUDA) code. labels Mar 26, 2021
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 26, 2021
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #7734 (285f3c3) into branch-0.19 (7871e7a) will increase coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7734      +/-   ##
===============================================
+ Coverage        81.86%   82.52%   +0.65%     
===============================================
  Files              101      101              
  Lines            16884    17458     +574     
===============================================
+ Hits             13822    14407     +585     
+ Misses            3062     3051      -11     
Impacted Files Coverage Δ
python/cudf/cudf/utils/gpu_utils.py 53.65% <0.00%> (-4.88%) ⬇️
python/cudf/cudf/core/column/lists.py 87.68% <0.00%> (-3.72%) ⬇️
python/cudf/cudf/core/column/decimal.py 92.95% <0.00%> (-1.92%) ⬇️
python/cudf/cudf/core/abc.py 87.23% <0.00%> (-1.14%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.83% <0.00%> (-0.20%) ⬇️
python/cudf/cudf/core/column/column.py 87.61% <0.00%> (-0.15%) ⬇️
python/cudf/cudf/utils/utils.py 85.36% <0.00%> (-0.07%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/ioutils.py 78.71% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a1bd66...285f3c3. Read the comment docs.

@trxcllnt trxcllnt changed the title Create and promote library aliases when linking libcudf locally Create and promote library aliases in libcudf installations Mar 26, 2021
@trxcllnt trxcllnt requested a review from kkraus14 March 26, 2021 05:14
cpp/cmake/cudf-build-config.cmake.in Outdated Show resolved Hide resolved
cpp/cmake/cudf-build-config.cmake.in Outdated Show resolved Hide resolved
@github-actions github-actions bot added the conda label Mar 26, 2021
@trxcllnt trxcllnt requested a review from a team as a code owner March 26, 2021 16:34
@trxcllnt trxcllnt requested a review from robertmaynard March 26, 2021 17:32
Comment on lines 44 to 45
- gtest
- gmock
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given these are only required if someone uses cudftestutil, how would we feel about not including these and if someone uses cudftestutil they're required to have gtest / gmock installed in their environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine with me, just means we have to install them into the gpuCI containers and block merging until the next nightly release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change was in reference to rapidsai/cuspatial#365 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we had a run on gpuCI where these didn't work? We install the rapids-build-env in all gpuCI images that we build with so both gtest/gmock should be there

Copy link
Contributor Author

@trxcllnt trxcllnt Mar 29, 2021

Choose a reason for hiding this comment

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

This cuSpatial build failed because CMake couldn't find GTest:

cudf could not be found because dependency GTest could not be found.

But the real reason could due to cuDF not having the changes in this PR. I'll remove these here and in cuSpatial and see what happens.

@kkraus14 kkraus14 added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Mar 29, 2021
@kkraus14
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 54dfaaa into rapidsai:branch-0.19 Mar 29, 2021
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Mar 30, 2021
This PR includes many of the same changes as were made in [cudf/pull/7107](rapidsai/cudf#7107).

* Export `cuspatial::cuspatial` CMake alias targets.
* Fixes -Wall errors discovered when changing compile flags.
* Use `CPMFindPackage` to find `libcudf` installed on the system or build `libcudf` from source.

edit: Depends on rapidsai/cudf#7574 and rapidsai/cudf#7734

Authors:
  - Paul Taylor (@trxcllnt)

Approvers:
  - AJ Schmidt (@ajschmidt8)
  - Mark Harris (@harrism)
  - Keith Kraus (@kkraus14)

URL: #365
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants