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

RMM now leverages rapids-cmake to reduce CMake boilerplate #800

Merged
merged 9 commits into from
Jun 22, 2021

Conversation

robertmaynard
Copy link
Contributor

rapids-cmake provides all the functionality of RMM existing CMake modules.
This will allow RAPIDS to deploy build-system fixes easily across all projects.

rapids-cmake provides all the functionality of RMM existing
CMake modules. This allow RAPIDS to deploy build-system fixes quickly
and across all projects.
@robertmaynard robertmaynard requested review from a team as code owners June 10, 2021 13:24
@github-actions github-actions bot added CMake cpp Pertains to C++ code gpuCI labels Jun 10, 2021
@robertmaynard robertmaynard added non-breaking Non-breaking change cpp Pertains to C++ code gpuCI and removed cpp Pertains to C++ code gpuCI labels Jun 10, 2021
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@ajschmidt8 ajschmidt8 added the improvement Improvement / enhancement to an existing function label Jun 10, 2021
@github-actions github-actions bot added the conda label Jun 10, 2021
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.

Various questions.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
cmake/rapids-cmake.json Outdated Show resolved Hide resolved
cmake/thirdparty/get_gbench.cmake Show resolved Hide resolved
@@ -0,0 +1,43 @@
# =============================================================================
Copy link
Member

Choose a reason for hiding this comment

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

Should the get_gtest.cmake and get_gbench.cmake be promoted to the rapids-cmake repo? I'd think they would be duplicated across multiple RAPIDS repos if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that gtest, gbench, and spdlog are all very resonable candiates to getting
moved over to rapids-cmake. In general I am hestitant to make rapids-cmake a package
manager but for core common dependencies it does make sense. I think thrust could also be included but we need to discuss that a bit more, since cudf uses a patched version of thrust ( do we want all of rapids to also do that? )

Most of the questions will be around what does the API look like for this. I think we would want to offer an API that offers very little customization ( around which version, flags, etc ) since rapids_cpm_find offers that. Instead we would be looking at a fairly closed/narrow API like the following:

  rapids_cpm_<PackageName>( [BUILD_EXPORT_SET <export-name>]
                            [INSTALL_EXPORT_SET <export-name>]                  
                          )

Which would simplify rmm usage to:

rapids_cpm_gtest()
rapids_cpm_benchmark()
rapids_com_spdlog(BUILD_EXPORT_SET rmm-exports INSTALL_EXPORT_SET rmm-exports)

Copy link
Member

Choose a reason for hiding this comment

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

That looks good. However I guess this shouldn't block the present PR. Agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

I have added a feature request to rapids-cmake for this: rapidsai/rapids-cmake#32

cmake/thirdparty/get_spdlog.cmake Show resolved Hide resolved
cmake/thirdparty/get_thrust.cmake Show resolved Hide resolved
The file should always be downloaded by CI to get the latest
API changes.
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Jul 2, 2021
…ing (#430)

This PR contains three distinct changes required to get cuspatial builds working and tests passing again:
1. RMM switched to rapids-cmake (rapidsai/rmm#800), which requires CMake 3.20.1, so this PR includes the required updates for that.
2. The Arrow upgrade in cudf also moved the location of testing utilities (rapidsai/cudf#7495). Long term cuspatial needs to move away from use of the testing utilities, which are not part of cudf's public API, but we are currently blocked by rapidsai/cudf#8646, so this PR just imports the internal `assert_eq` method as a stopgap to get tests passing.
3. The changes in rapidsai/cudf#8373 altered the way that metadata was propagated to libcudf outputs from previously existing cuDF Python objects. The new code paths require cuspatial to override metadata copying at the GeoDataFrame rather than the GeoColumn level in order to ensure that information about column types is lost in the libcudf round trip and the metadata copying functions are now called on the output DataFrame rather than the input one.

This PR supersedes #427, #428, and #429, all of which can now be closed.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Christopher Harris (https://github.com/cwharris)

URL: #430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake conda cpp Pertains to C++ code gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants