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

remove broken/unsupported rapids_cpm_find usages in favor of rapids_find_package #5

Merged
3 commits merged into from
Jan 27, 2023

Conversation

cwharris
Copy link
Contributor

@cwharris cwharris commented Jan 24, 2023

By using rapids_cpm_find we're implicitly stating that we support download and building packages. Excluding the CPM arguments from the call doesn't make it any better, since the error message produced by rapids_cpm_find when it does not find the package installed indicates that the CPM arguments should be added. In short, rapids_cpm_find doesn't do us any favors if we don't support downloading and building the package from source. Instead it just acts as a confusing wrapper around find_package. This PR replaces rapids_cpm_find with rapids_find_package and removes the broken build code for cudf, ucx, and boost.

If/when we decide to support building these packages from source, we can revisit rapids_cpm_find, but for now we require installing the packages ahead of time (this hasn't changed).

@cwharris cwharris requested a review from a team as a code owner January 24, 2023 17:20
Copy link
Contributor

@drobison00 drobison00 left a comment

Choose a reason for hiding this comment

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

Looks fine to me, assuming all the tests pass, and that it works fails with an appropriate message when we have to pull and build UCX.

@mdemoret-nv Do you remember why this was originally such a manual process?

@drobison00 drobison00 added bug Something isn't working non-breaking Non-breaking change labels Jan 24, 2023
@cwharris
Copy link
Contributor Author

@drobison00 the error produced by find_package is very informative for anyone who's used to cmake and understands where/how packages should be installed (conda).

CMake Error at external/utilities/cmake/morpheus_utils/package_config/ucx/Configure_ucx.cmake:25 (find_package):
  By not providing "Finducx.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "ucx", but
  CMake did not find one.

  Could not find a package configuration file provided by "ucx" (requested
  version 1.13) with any of the following names:

    ucxConfig.cmake
    ucx-config.cmake

  Add the installation prefix of "ucx" to CMAKE_PREFIX_PATH or set "ucx_DIR"
  to a directory containing one of the above files.  If "ucx" provides a
  separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
  cmake/dependencies.cmake:39 (morpheus_utils_configure_ucx)
  CMakeLists.txt:135 (include)

@cwharris
Copy link
Contributor Author

Still need to test this with Morpheus now that the utilities changes have been merged there.

@cwharris
Copy link
Contributor Author

I've kept the old code as a comment, per @mdemoret-nv 's request, and used rapids_find_package instead of find_package so we have proper export sets.

@cwharris cwharris requested a review from drobison00 January 25, 2023 16:28
@cwharris cwharris changed the title remove broken/unsupported rapids_cpm_find usages in favor of find_package remove broken/unsupported rapids_cpm_find usages in favor of rapids_find_package Jan 25, 2023
@cwharris
Copy link
Contributor Author

tested with Morpheus and it works. Need to update one line in MRC to get this working, since I renamed the boost configuration function.

@cwharris
Copy link
Contributor Author

@drobison00 I don't have permissions to change labels, but because of the boost configuration function rename this is a breaking change now.

@drobison00 drobison00 added breaking Breaking change and removed non-breaking Non-breaking change labels Jan 26, 2023
@cwharris
Copy link
Contributor Author

/merge

@drobison00
Copy link
Contributor

/merge

@ghost ghost merged commit 6518302 into nv-morpheus:branch-23.01 Jan 27, 2023
ghost pushed a commit to nv-morpheus/MRC that referenced this pull request Jan 27, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants