-
Notifications
You must be signed in to change notification settings - Fork 310
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 CMake/C++ references to cugraph-ops #4744
Remove CMake/C++ references to cugraph-ops #4744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to remove ALL references to cugraph-ops
here? If so, there are many more:
git grep -i 'cugraph.*ops'
For example, all the uses of those now-removed CMake options in build scripts.
CI configs (click me)
cugraph/.github/workflows/build.yaml
Lines 79 to 81 in 050d524
extra-repo: rapidsai/cugraph-ops | |
extra-repo-sha: branch-24.12 | |
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY |
cugraph/.github/workflows/build.yaml
Lines 103 to 105 in 050d524
extra-repo: rapidsai/cugraph-ops | |
extra-repo-sha: branch-24.12 | |
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY |
cugraph/.github/workflows/pr.yaml
Lines 140 to 142 in 050d524
extra-repo: rapidsai/cugraph-ops | |
extra-repo-sha: branch-24.12 | |
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY |
cugraph/.github/workflows/pr.yaml
Lines 159 to 161 in 050d524
extra-repo: rapidsai/cugraph-ops | |
extra-repo-sha: branch-24.12 | |
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY |
cugraph/.github/workflows/test.yaml
Line 26 in 050d524
symbol_exclusions: (cugraph::ops|hornet|void writeEdgeCountsKernel|void markUniqueOffsetsKernel) |
build scripts (click me)
Line 46 in 050d524
--without_cugraphops |
Line 110 in 050d524
BUILD_WITH_CUGRAPHOPS=ON |
Line 271 in 050d524
-DUSE_CUGRAPH_OPS=${BUILD_WITH_CUGRAPHOPS} \ |
many more in build.sh
cugraph/ci/build_wheel_cugraph.sh
Line 31 in 050d524
export SKBUILD_CMAKE_ARGS="-DDETECT_CONDA_ENV=OFF;-DFIND_CUGRAPH_CPP=OFF;-DCPM_cugraph-ops_SOURCE=${GITHUB_WORKSPACE}/cugraph-ops/${EXTRA_CMAKE_ARGS}" |
cugraph/ci/build_wheel_pylibcugraph.sh
Line 18 in 050d524
export SKBUILD_CMAKE_ARGS="-DDETECT_CONDA_ENV=OFF;-DFIND_CUGRAPH_CPP=OFF;-DCPM_cugraph-ops_SOURCE=${GITHUB_WORKSPACE}/cugraph-ops/${EXTRA_CMAKE_ARGS}" |
dependency on 'libcugraphops' and 'pylibcugraphops' (click me)
Lines 23 to 24 in 050d524
# Deprecate pylibcugraphops | |
- depends_on_pylibcugraphops |
Lines 921 to 944 in 050d524
depends_on_pylibcugraphops: | |
common: | |
- output_types: conda | |
packages: | |
- &pylibcugraphops_unsuffixed pylibcugraphops==24.12.*,>=0.0.0a0 | |
- output_types: requirements | |
packages: | |
# pip recognizes the index as a global option for the requirements.txt file | |
- --extra-index-url=https://pypi.nvidia.com | |
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple | |
specific: | |
- output_types: [requirements, pyproject] | |
matrices: | |
- matrix: | |
cuda: "12.*" | |
cuda_suffixed: "true" | |
packages: | |
- pylibcugraphops-cu12==24.12.*,>=0.0.0a0 | |
- matrix: | |
cuda: "11.*" | |
cuda_suffixed: "true" | |
packages: | |
- pylibcugraphops-cu11==24.12.*,>=0.0.0a0 | |
- {matrix: null, packages: [*pylibcugraphops_unsuffixed]} |
Lines 429 to 430 in 050d524
# Deprecate libcugraphops | |
- libcugraphops==24.12.*,>=0.0.0a0 |
- libcugraphops ={{ minor_version }} |
and several more, see that git grep
output
@jameslamb Since CI has already passed here I am inclined to just accept this as-is and then iteratively rip out more. Maybe we can limit this PR's scope to CMake / C++ removal, and then do the removals you suggested (mostly CI, build scripts, dependencies, etc.) in a follow-up PR. I started to look into this as well and noticed there are quite a few references in the GNN code, which is being split out, so there may be potentially duplicate work that we should defer until after that split. |
Ok yeah fair enough, let's stop here and do more in follow-ups. |
@@ -186,11 +186,4 @@ def test_docstring(self, docstring): | |||
f"{docstring.name}:\n{doctest_stdout.getvalue()}" | |||
) | |||
except AssertionError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is no longer using specialized logic in the exception clause, the "except: raise" pattern is unneeded. We can remove the try
entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This PR fixes a number of issues in the manifest: - cudf's dependency on rmm was accidentally removed in #221. Since rmm is header-only I guess nobody noticed the issue since it doesn't increase build times and only shows up if you attempt to make local changes to rmm and test them in cudf. - UCXX_ENABLE_PYTHON was removed in rapidsai/ucxx#257 - cuml depends on cuvs as of rapidsai/cuml#6085 - cugraph does not depend on cugraph-ops as of rapidsai/cugraph#4744 - Many packages were relying on transitively getting rmm or raft as dependencies, which causes problems when the intermediate dependency is removed. Extra `-D<package_name>_ROOT` variables in the CMake configure don't cause any problems so I made everything explicit. --------- Co-authored-by: Paul Taylor <[email protected]> Co-authored-by: Bradley Dice <[email protected]>
This repo should no longer need any build-time dependencies on cloning the `cugraph-ops` source code or pulling in its packages, as of these PRs: * #4744 * #4765 This PR proposes the following: * removing CI configuration related to cloning the `cugraph-ops` repo * removing CMake options related to `cugraph-ops` - `ALLOW_CLONE_CUGRAPH_OPS` - `CUGRAPH_EXCLUDE_CUGRAPH_OPS_FROM_ALL` - `CUGRAPH_USE_CUGRAPH_OPS_STATIC` - `USE_CUGRAPH_OPS` * removing dependencies on `pylibcugraphops` / `libcugraphops` in development scripts, conda recipes * removing the `cugraph-ops` docs, and references to them in other docs * removing unused source file `cpp/src/utilities/cugraph_ops_utils.hpp` * removing the `cugraph_ops` pytest marker ## Notes for Reviewers ### Benefits of these changes * faster CI * simplifies the pending work to introduce `libcugraph` wheel packages (rapidsai/rapids-wheels-planning#53) ### How I identified these changes ```shell git grep -i '_ops' git grep -i '\-ops' git grep -i 'cugraphops' ``` ### Why is this so big? I'd originally wanted to leave the docs-building stuff in place, but saw `docs-build` CI here failing because docs haven't been built from the `cugraph-ops` repo yet: https://github.com/rapidsai/cugraph/blob/c5d3d231c9cac361bf51246c13440a42eeda93b9/build.sh#L334-L341 Talked offline with @acostadon and @ChuckHastings , and we agreed that those docs could just be removed here. ### What is intentionally being left? * references in licenses (as some code from `cugraph-ops` is vendored here) * references in the docs for the GNN packages (`cugraph-dgl`, `cugraph-pyg`, `pylibwholegraph`), as those do still rely on `cugraph-ops` (their docs will eventually move out of this repo) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Kyle Edwards (https://github.com/KyleFromNVIDIA) - Alex Barghi (https://github.com/alexbarghi-nv) - Chuck Hastings (https://github.com/ChuckHastings) URL: #4784
Delete deprecated cugraph-ops functionality, clean up references to cugraph-ops.