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

Open
wants to merge 4 commits into
base: branch-24.12
Choose a base branch
from

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

3 participants