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

Introduce rapids_cpm_<preset> #52

Merged

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Jul 30, 2021

Requires #48 and #51

Adds rapids_cpm_ for common packages

Fixes #32

The following packages are now are easier to user for RAPIDS projects
as rapids-cmake offers a pre-configured setup for each project.

  • GTest
  • NVBench
  • RMM
  • SpdLog
  • Thrust

On top of providing a consistent version of these packages to all RAPIDS projects, rapids-cmake now is able to deduce when these projects should also be installed.

rapids_cpm_gtest(BUILD_EXPORT_SET myproject)
rapdis_cpm_rmm(BUILD_EXPORT_SET myproject
               INSTALL_EXPORT_SET myproject)

Given the above snippet when RMM is built as a subcomponent of myproject it will be installed as well. This is done since
RMM is part of the INSTALL export set, and therefore must be available from an installed version of myproject.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Introduces a non-breaking change 3 - Ready for Review Ready for review by team labels Jul 30, 2021
@robertmaynard robertmaynard force-pushed the fea/rapids_offer_presetup_pkgs branch 3 times, most recently from 1078a88 to 6af51f9 Compare August 2, 2021 16:21
endif()

include("${rapids-cmake-dir}/cpm/find.cmake")
rapids_cpm_find(Thrust 1.12.0 ${ARGN}
Copy link
Member

Choose a reason for hiding this comment

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

It would be tricky to have a central config file to control the versions of non rapids branch naming projects, but it'd be useful to have a readme with a list of the dependencies and versions, particularly if we put more of them here, to be sure they are synced with integration, conda recipes, etc. What do you think?

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

How about something like a json file in the cpm directory that listed all the project url and branch?

rapids-cmake and the docs could both reference it to maintain a single source of truth

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a great solution!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I am thinking is something like:

{
  "GTest" : {
    "version" : "1.10",
    "git_url" : "https://github.com/google/googletest.git",
    "git_tag" : "release-{version}",
  },
  "nvbench" : {
    "version" : "0.0",
    "git_url" : "https://github.com/NVIDIA/nvbench.git",
    "git_tag" : "main",
  },
  "rmm" : {
    "version" : "rapids-cmake-version",
    "git_url" : "https://github.com/rapidsai/rmm.git",
    "git_tag" : "branch-{version}",
  },
  "spdlog" : {
    "version" : "1.8.5",
    "git_url" : "https://github.com/gabime/spdlog.git",
    "git_tag" : "v{version}",
  },
  "Thrust" : {
    "version" : "1.12.0",
    "git_url" : "https://github.com/NVIDIA/thrust.git",
    "git_tag" : "{version}",
  },
}

I am still trying to figure out how best to match rmm version to the current rapids calver, so for now I have used a placeholder ( rapids-cmake-version ).

Any thoughts?

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 have updated the PR with an initial implementation that has everything working based on a json file ( https://github.com/rapidsai/rapids-cmake/pull/52/files#diff-695faffc4b8bc9bc2b2fb2f4c3958c279c2d01744eeead1a30dedd1ead81eb6c )

I still have to do the following:

  • Reference the json versions in each packages documentation
  • Formally document the versions.json file and what magic placeholders are supported

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

The JSON file approach looks really good! Had one question about the fetching

CPM_ARGS
GIT_REPOSITORY ${repository}
GIT_TAG ${tag}
GIT_SHALLOW TRUE
Copy link
Member

Choose a reason for hiding this comment

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

won't GIT_SHALLOW shallow being TRUE interfere with the ability of setting any commit hash in the json file? Setting it to false would give us more flexibility, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it is always combined with a tag it will do a shallow checkout at the tip of that tag/branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh I see what you mean, any arbitrary hash value for the GIT_TAG.

Yes in those case we need a shallow equals false. I think the best approach is to extend the json spec to include
git_shallow boolean field with the default being true

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 have updated the PR with the optional git_shallow field

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Looks great with the added field for the json file!


An optional boolean value that represents if we should do a shallow git clone
or not. If no such field exists the default is `git_shallow : true`

Copy link
Contributor

Choose a reason for hiding this comment

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

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 is worth doing as a followup Issue/Feature discussion and PR.

As you are aware since cudf patches thrust it doesn't install it in the default location, which is what this change does. I am hesitant to install a modified Thrust in the 'default' location as that could cause confusion to users ( potential compile time or performance differences ). But once we resolve where patched versions of projects should be installed, this will be required.

@robertmaynard
Copy link
Contributor Author

rerun tests

Fixes rapidsai#32

The following packages are now are easier to user for RAPIDS projects
as rapids-cmake offers a pre-configured setup for each project.

  GTest
  NVBench
  RMM
  SpdLog
  Thrust

On top of providing a consistent version of these packages to
all RAPIDS projects, rapids-cmake now is able to deduce when these
projects should also be installed.

```cmake

rapids_cpm_gtest(BUILD_EXPORT_SET myproject)
rapdis_cpm_rmm(BUILD_EXPORT_SET myproject
               INSTALL_EXPORT_SET myproject)
```
Given the above snippet when RMM is built as a subcomponent of
`myproject` it will be installed as well. This is done since
RMM is part of the INSTALL export set, and therefore must
be available from an installed version of `myproject`.
…ency

The generated output file from rapids_export_write_dependencies can't
depend on rapid-cmake being reachable. This behavior has been restored
by refactoring out the cpm download logic to `cpm/detail/download.cmake'
@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e73ceff into rapidsai:branch-21.10 Aug 18, 2021
@robertmaynard robertmaynard deleted the fea/rapids_offer_presetup_pkgs branch August 18, 2021 20:56
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 feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Add rapids_cpm_<PackageName> for common packages
3 participants