simplify wholegraph CMake, other small building and testing changes #102
Conversation
jameslamb
added
improvement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
|
||
| set -euo pipefail | ||
|
|
||
| rapids-logger "Create test conda environment" | ||
| . /opt/conda/etc/profile.d/conda.sh | ||
|
|
||
| RAPIDS_VERSION="$(rapids-version)" | ||
| export RAPIDS_VERSION_MAJOR_MINOR="$(rapids-version-major-minor)" |
There was a problem hiding this comment.
For re-use in cpp/Doxyfile
| @@ -38,7 +38,7 @@ PROJECT_NAME = "WholeGraph C API" | |||
| # could be handy for archiving the generated documentation or if some version | |||
| # control system is used. | |||
|
|
|||
| PROJECT_NUMBER = 25.02 | |||
| PROJECT_NUMBER = $(RAPIDS_VERSION_MAJOR_MINOR) | |||
There was a problem hiding this comment.
This is what other RAPIDS projects are doing, e.g. rmm: https://github.com/rapidsai/rmm/blob/8275ba8dc94a7fc63d9513acd81e3a2cf30a85d1/doxygen/Doxyfile#L41
One less hard-coded version is nice for:
- smaller diffs on updates when development switches to new versions
- simpler
update-version.sh - reduced risk of inconsistencies between docs and code
jameslamb
changed the title
| include(rapids-cmake) | ||
| include(rapids-cuda) | ||
| include(rapids-cpm) | ||
|
|
||
| rapids_cuda_init_architectures(PYLIBWHOLEGRAPH) | ||
|
|
||
| project(PYLIBWHOLEGRAPH VERSION ${WHOLEGRAPH_VERSION} LANGUAGES CXX CUDA) | ||
| project( | ||
| PYLIBWHOLEGRAPH |
There was a problem hiding this comment.
We're a little inconsistent about this across repos but it might be nice to use lowercase pylibwholegraph here. However, it's fine if that is out-of-scope for this PR (and/or not important enough to change at all).
e.g. cuDF has project(CUDF...) but also project(pylibcudf...), so our casing is not consistent even within a repo.
There was a problem hiding this comment.
Thanks, yeah I'd noticed that. Let's treat it as out-of-scope for this PR. I'd like to merge this to simplify things ahead of setting up libwholegraph wheels.
The project name flows into a bunch of other variables that CMake generates (see https://cmake.org/cmake/help/latest/command/project.html#command:project) and I'm not sure if those are case-sensitive, so changing the case could require other changes to CMake code here (and maybe in other places that build wholegraph from source, like RAPIDS devcontainers). I'd rather not work through that right now.
There was a problem hiding this comment.
Yes, all those downstream uses are case sensitive. At some point we may want to strive for consistency across RAPIDS but it’s low priority - lots of work, risk of breakage, and not a lot of value.
|
/merge |
Proposes some miscellaneous packaging cleanup:
RAPIDS_VERSIONfromcugraph-gnn/rapids_config.cmake
Line 20 in af22a12
pre-commithooks to their latest versions