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

Rework libcuspatial CMakeLists.txt to export targets for CPM #365

Merged
merged 58 commits into from
Mar 30, 2021

Conversation

trxcllnt
Copy link
Contributor

@trxcllnt trxcllnt commented Mar 2, 2021

This PR includes many of the same changes as were made in cudf/pull/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

@trxcllnt trxcllnt requested review from a team as code owners March 2, 2021 16:14
@trxcllnt trxcllnt added 3 - Ready for Review Ready for review by team cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 2, 2021
@trxcllnt trxcllnt added 5 - DO NOT MERGE Hold off on merging; see PR for details 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels Mar 26, 2021
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Mar 29, 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:
```shell
$ 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=

```

Authors:
  - Paul Taylor (@trxcllnt)
  - Robert Maynard (@robertmaynard)

Approvers:
  - Robert Maynard (@robertmaynard)
  - Keith Kraus (@kkraus14)
  - Ray Douglass (@raydouglass)

URL: #7734
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.

Just some tiny C++ things.

cpp/benchmarks/hausdorff_benchmark.cpp Outdated Show resolved Hide resolved
Comment on lines +52 to +56
virtual void SetUp(const ::benchmark::State& state) override
{
mr = std::make_shared<rmm::mr::cuda_memory_resource>();
rmm::mr::set_current_device_resource(mr.get()); // set default resource to cuda
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? The default resource is the cuda_memory_resource.

Copy link
Contributor Author

@trxcllnt trxcllnt Mar 30, 2021

Choose a reason for hiding this comment

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

The benchmark base class instantiates an RMM pool resource in the SetUp function. It's assigned to a local variable instead of the member var, so it segfaults. When I fix that, I get "Maximum pool size exceeded" on the first bench run.

I'm not sure why the second error is happening, and this PR is primarily about CMake. So this change just makes the benchmark code compile and run without crashing.

cpp/src/io/shp/polygon_shapefile_reader.cpp Show resolved Hide resolved
cpp/benchmarks/hausdorff_benchmark.cpp Show resolved Hide resolved
cpp/tests/spatial/haversine_test.cpp Show resolved Hide resolved
@trxcllnt trxcllnt requested a review from a team as a code owner March 30, 2021 05:35
@github-actions github-actions bot added the Python Related to Python code label Mar 30, 2021
@kkraus14 kkraus14 removed 5 - DO NOT MERGE Hold off on merging; see PR for details 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels Mar 30, 2021
@kkraus14
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 09817f0 into rapidsai:branch-0.19 Mar 30, 2021
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 cmake Related to CMake code or build configuration conda Related to conda and conda 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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants