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 CMake/C++ references to cugraph-ops #4744

Merged

Conversation

ChuckHastings
Copy link
Collaborator

Delete deprecated cugraph-ops functionality, clean up references to cugraph-ops.

@ChuckHastings ChuckHastings self-assigned this Nov 1, 2024
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function breaking Breaking change and removed cuGraph CMake python labels Nov 1, 2024
@ChuckHastings ChuckHastings marked this pull request as ready for review November 4, 2024 04:25
@ChuckHastings ChuckHastings requested review from a team as code owners November 4, 2024 04:25
@jameslamb jameslamb self-requested a review November 4, 2024 14:42
Copy link
Member

@jameslamb jameslamb left a 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)

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

symbol_exclusions: (cugraph::ops|hornet|void writeEdgeCountsKernel|void markUniqueOffsetsKernel)

build scripts (click me)

--without_cugraphops

BUILD_WITH_CUGRAPHOPS=ON

-DUSE_CUGRAPH_OPS=${BUILD_WITH_CUGRAPHOPS} \

many more in build.sh

export SKBUILD_CMAKE_ARGS="-DDETECT_CONDA_ENV=OFF;-DFIND_CUGRAPH_CPP=OFF;-DCPM_cugraph-ops_SOURCE=${GITHUB_WORKSPACE}/cugraph-ops/${EXTRA_CMAKE_ARGS}"

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)

# Deprecate pylibcugraphops
- depends_on_pylibcugraphops

cugraph/dependencies.yaml

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]}

cugraph/dependencies.yaml

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

cpp/CMakeLists.txt Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Nov 4, 2024

@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.

@jameslamb
Copy link
Member

Ok yeah fair enough, let's stop here and do more in follow-ups.

@ChuckHastings ChuckHastings requested a review from a team as a code owner November 4, 2024 15:38
@ChuckHastings ChuckHastings requested a review from bdice November 4, 2024 15:38
@@ -186,11 +186,4 @@ def test_docstring(self, docstring):
f"{docstring.name}:\n{doctest_stdout.getvalue()}"
)
except AssertionError:
Copy link
Contributor

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.

@ChuckHastings ChuckHastings changed the title Remove references to cugraph-ops Remove CMake/C++ references to cugraph-ops Nov 4, 2024
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

LGTM

@ChuckHastings
Copy link
Collaborator Author

/merge

@rapids-bot rapids-bot bot merged commit 2d1189b into rapidsai:branch-24.12 Nov 8, 2024
140 checks passed
vyasr added a commit to rapidsai/devcontainers that referenced this pull request Dec 2, 2024
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]>
rapids-bot bot pushed a commit that referenced this pull request Dec 4, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change CMake cuGraph improvement Improvement / enhancement to an existing function python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants