make cugraph-ops optional for cugraph-gnn packages #99

tingyu66 · 2024-12-19T03:59:17Z

Address #81

copy-pr-bot · 2024-12-19T04:00:38Z

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.

tingyu66 · 2024-12-19T04:07:06Z

/ok to test

bdice · 2024-12-19T15:28:11Z

dependencies.yaml

            packages:
-              - pytorch-cuda=12.4
-          - matrix: {cuda: "11.8"}
+            - pytorch-gpu>=2.3=*cuda120*


This is already using conda-forge, I think? pytorch-gpu is a conda-forge package, not a pytorch channel package. Also, the latest conda-forge builds are built with CUDA 12.6. CUDA 12.0 is no longer used to build.

For compatibility reasons we may want to stick to older builds of pytorch-gpu (built with cuda120) for now. We will hopefully be able to relax this in the future.

Yes, this PR switches to conda-forge::pytorch-gpu since pytorch channel will discontinue.

Also, the latest conda-forge builds are built with CUDA 12.6. CUDA 12.0 is no longer used to build.

For compatibility reasons we may want to stick to older builds of pytorch-gpu (built with cuda120) for now. We will hopefully be able to relax this in the future.

Oh, I had not noticed that the most recent build (_306) is only against 12.6. I agree with keeping 12.0 for better backward compatibility. However, the CUDA 11 build seems missing. Do we have details on their build matrix?

CUDA 11 builds were dropped recently. You may need an older version for CUDA 11 compatibility. I also saw this while working on rapidsai/cudf#17475. mamba search -c conda-forge "pytorch=*=cuda118*" indicates the latest version with CUDA 11 support is 2.5.1 build 303. The latest is 2.5.1 build 306.

For completeness, the latest CUDA 12.0 build was also 2.5.1 build 303.

Got it, thanks. It shouldn't be a dealbreaker unless another test component ends up requiring a newer version of torch on CUDA 11 down the line.

pytorch-gpu requires __cuda, and is not installable on systems without a CUDA driver. This makes it impossible to resolve the conda environment needed for devcontainers jobs in CI, which are CPU-only.

Note: many CUDA packages, including RAPIDS, are explicitly designed not to have __cuda as a run requirement, because it makes it impossible to install on a CPU node before using that environment on another system with a GPU.

It looks like if we just use pytorch instead of pytorch-gpu, we still get GPU builds:

CUDA 11 driver present:

CONDA_OVERRIDE_CUDA="11.8" conda create -n test --dry-run pytorch

shows

pytorch 2.5.1 cuda118_py313h40cdc2d_303 conda-forge

CUDA 12 driver present:

CONDA_OVERRIDE_CUDA="12.5" conda create -n test --dry-run pytorch

shows

pytorch 2.5.1 cuda126_py313hae2543e_306 conda-forge

No CUDA driver present:

CONDA_OVERRIDE_CUDA="" mamba create -n test --dry-run pytorch

shows

pytorch 2.5.1 cpu_mkl_py313_h90df46e_108 conda-forge

This should be sufficient. Let's try using just pytorch instead of pytorch-gpu with specific CUDA build selectors.

There are two benefits here, if my proposal above works.

devcontainers CI job would get CPU-only builds, which should still be fine for builds

We don't need to specify CUDA versions, so this dependency doesn't have to be "specific" to CUDA 11/12

I agree, let's try with "pytorch" instead of "pytorch-gpu".

That opens up a risk that there may be situations where the solver chooses a CPU-only version because of some conflict, but hopefully cugraph-pyg can detect that with torch.cuda.is_available() or similar and raise an informative error saying something like "if using conda, try 'conda install cugraph-pyg pytorch-gpu'".

We don't need to specify CUDA versions, so this dependency doesn't have to be "specific" to CUDA 11/12

I looked into this today... we shouldn't have needed to specify CUDA versions in build strings for pytorch-gpu anyway, as long as we're pinning the cuda-version package somewhere (for example, in the run: dependencies of cugraph).

Looks like pytorch-gpu is == pinned to a specific pytorch.

And the pytorch CUDA builds all have run: dependencies on cuda-version.

So here in cugraph-pyg, just having cuda-version as a run: dependency would be enough to ensure a compatible pytorch-gpu / pytorch is pulled.

@jameslamb These simplifications to drop build string info are only possible now with conda-forge, iirc. I believe more complexity was required when we used the pytorch channel, and we probably just carried that over when switching to conda-forge.

copy-pr-bot · 2024-12-22T21:45:24Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

tingyu66 · 2024-12-22T21:46:06Z

/ok to test

jameslamb · 2025-01-06T19:52:19Z

/ok to test

jameslamb

Hey @tingyu66 , I'd like to help keep this moving forward. Left some questions for your consideration.

jameslamb · 2025-01-06T19:55:47Z

dependencies.yaml

@@ -18,7 +18,6 @@ files:
      - depends_on_cugraph
      - depends_on_cudf
      - depends_on_dask_cudf
-      - depends_on_pylibcugraphops


Can the dependencies in the conda recipes be removed as well?

cugraph-gnn/conda/recipes/cugraph-pyg/meta.yaml

Line 38 in 71675d8

- pylibcugraphops ={{ minor_version }}

cugraph-gnn/conda/recipes/cugraph-dgl/meta.yaml

Line 32 in 71675d8

- pylibcugraphops ={{ minor_version }}

@tingyu66 could you please make this change (removing the dependencies on pylibcugraphops in conda recipes) in this PR?

@jameslamb Done, thank you!

jameslamb · 2025-01-06T20:14:48Z

python/cugraph-pyg/cugraph_pyg/nn/conv/__init__.py

+except Exception as e:
+    warnings.warn(f"Unexpected error while importing pylibcugraphops: {e}")
+
+if HAVE_CUGRAPH_OPS:


Why is this PR putting cugraph-ops stuff behind a conditional import instead of completely removing it?

Or, said another way.... how are you expecting that someone would have cugraph-pyg=25.2.* and pylibcugraphops=25.2.* installed together? Is this being left here so it's still possible to build and install pylibcugraphops from source?

I'm hoping we can completely stop publishing pylibcugraphops packages in the 25.02 release.

The conditional import is there because we planned to migrate cugraph-ops to a new location, and the new package will possibly retain the same name. We understand that rapids won't release cugraph-ops in 25.02. It's more of a placeholder for the migrated package.

Oh interesting, did not know that!

Ok thank you.

Since this new home for the code doesn't exist yet, can we remove the warning on line 24? It seems wrong to warn about something you know does not exist.

jameslamb · 2025-01-06T22:04:37Z

/ok to test

tingyu66 · 2025-01-06T22:10:27Z

/ok to test

tingyu66 · 2025-01-07T19:46:09Z

/ok to test

tingyu66 · 2025-01-08T23:20:56Z

/ok to test

tingyu66 · 2025-01-08T23:24:13Z

/ok to test

bdice

The packaging changes look mostly right to me. There are a few areas that may have been missed:

ci/release/update-version.sh has references to libcugraphops and pylibcugraphops
readme_pages/CONTRIBUTING.md says "The cugraph-ops package is the expection being a closed-source package." (Also probably a typo, expection -> exception)
python/cugraph-pyg/pytest.ini has references to cugraph_ops, and several tests are marked with cugraph_ops

I would like to see those addressed (or responded to here) before approving.

The cugraph-gnn library changes can probably go further than shown in this PR. I don't think it makes sense to keep so many references to a library that no longer exists, even if you expect a new package. Maybe it's better to drop all cugraph-ops references and then add those code paths back when the new home of that code exists.

bdice · 2025-01-08T23:54:15Z

python/cugraph-dgl/cugraph_dgl/tests/test_utils.py

@@ -179,6 +180,7 @@ def test_get_source_destination_range():
    assert output_d == expected_output


+@pytest.mark.skip(reason="Skipping due to missing cugraph-ops backend.")
 def test__create_homogeneous_cugraph_dgl_nn_sparse_graph():


nit: Looks like a typo with the double underscore here.

Suggested change

def test__create_homogeneous_cugraph_dgl_nn_sparse_graph():

def test_create_homogeneous_cugraph_dgl_nn_sparse_graph():

bdice · 2025-01-08T23:56:22Z

python/cugraph-pyg/cugraph_pyg/nn/conv/__init__.py

+except Exception as e:
+    warnings.warn(f"Unexpected error while importing pylibcugraphops: {e}")
+
+if HAVE_CUGRAPH_OPS:


Since this new home for the code doesn't exist yet, can we remove the warning on line 24? It seems wrong to warn about something you know does not exist.

@alexbarghi-nv

Fixes #55 RAPIDS will stop shipping `cugraph-ops` in 25.02 (ref: rapidsai/cugraph-gnn#99) This proposes removing all references to it in docs here. ## Notes for Reviewers In addition to auto-assigned CODEOWNERS, let's please not merge this until @alexbarghi-nv approves. ### How I found these references ```shell git grep -i cugraphops git grep -i -E 'cugraph.*ops' git grep -i ops ``` Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Don Acosta (https://github.com/acostadon) - Bradley Dice (https://github.com/bdice) - Alex Barghi (https://github.com/alexbarghi-nv) URL: #72

jameslamb · 2025-01-10T18:30:17Z

I don't think it makes sense to keep so many references to a library that no longer exists, even if you expect a new package. Maybe it's better to drop all cugraph-ops references and then add those code paths back when the new home of that code exists.

I'm + to this. I think it'd be better to fully remove the effectively-unreachable code here and then re-introduce it when the successor to cugraph-ops is available.

But I'm also ok with deferring that to later if it means we could get this PR merged sooner, as that would unblock a lot of other work for removing cugraph-ops across RAPIDS.

make cugraph-ops optional for cugraph-pyg

4f4c82e

tingyu66 requested review from a team as code owners December 19, 2024 03:59

tingyu66 requested a review from KyleFromNVIDIA December 19, 2024 03:59

tingyu66 marked this pull request as draft December 19, 2024 04:00

Merge branch 'branch-25.02' into rm-cugraph-ops

d3f975a

tingyu66 added breaking Introduces a breaking change improvement Improves an existing functionality labels Dec 19, 2024

bdice reviewed Dec 19, 2024

View reviewed changes

tingyu66 added 2 commits December 22, 2024 16:42

Merge branch 'branch-25.02' into rm-cugraph-ops

eddabef

format

bafd813

Merge branch 'branch-25.02' into rm-cugraph-ops

a111a09

jameslamb self-requested a review January 6, 2025 20:15

jameslamb reviewed Jan 6, 2025

View reviewed changes

pre-commit

8e5a0f8

use just pytorch instead of pytorch-gpu

4992680

tingyu66 added 3 commits January 6, 2025 23:13

update -pyg examples

c48941a

skip importing convs in cugraph-dgl

1120cdb

Merge branch 'branch-25.02' into rm-cugraph-ops

0d285e0

tingyu66 marked this pull request as ready for review January 7, 2025 22:36

jameslamb mentioned this pull request Jan 8, 2025

remove references to cugraph-ops rapidsai/cugraph-docs#72

Merged

modify conda recipes

c4019df

Merge branch 'branch-25.02' into rm-cugraph-ops

be8996f

style

ec9598e

bdice reviewed Jan 9, 2025

View reviewed changes

bdice mentioned this pull request Jan 9, 2025

fix: Direct users to the CUDA metapackage guides for compilation conda-forge/conda-forge.github.io#2416

Open

1 task

This was referenced Jan 10, 2025

WIP: remove cugraph-ops from nightly pipeline rapidsai/workflows#70

Draft

WIP: remove cugraph-ops rapidsai/rapids-metadata#37

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make cugraph-ops optional for cugraph-gnn packages #99

make cugraph-ops optional for cugraph-gnn packages #99

tingyu66 commented Dec 19, 2024

copy-pr-bot bot commented Dec 19, 2024

tingyu66 commented Dec 19, 2024

bdice Dec 19, 2024 •

edited

Loading

bdice Dec 19, 2024

tingyu66 Dec 20, 2024

bdice Dec 20, 2024

bdice Dec 20, 2024

tingyu66 Dec 20, 2024

bdice Jan 6, 2025 •

edited

Loading

bdice Jan 6, 2025

jameslamb Jan 7, 2025

bdice Jan 7, 2025

copy-pr-bot bot commented Dec 22, 2024

tingyu66 commented Dec 22, 2024

jameslamb commented Jan 6, 2025

jameslamb left a comment

jameslamb Jan 6, 2025

tingyu66 Jan 6, 2025

jameslamb Jan 8, 2025

tingyu66 Jan 8, 2025

jameslamb Jan 6, 2025

tingyu66 Jan 6, 2025

jameslamb Jan 8, 2025

bdice Jan 8, 2025

jameslamb commented Jan 6, 2025

tingyu66 commented Jan 6, 2025

tingyu66 commented Jan 7, 2025

tingyu66 commented Jan 8, 2025

tingyu66 commented Jan 8, 2025

bdice left a comment

bdice Jan 8, 2025 •

edited

Loading

bdice Jan 8, 2025

jameslamb commented Jan 10, 2025

	def test__create_homogeneous_cugraph_dgl_nn_sparse_graph():
	def test_create_homogeneous_cugraph_dgl_nn_sparse_graph():

make cugraph-ops optional for cugraph-gnn packages #99

Are you sure you want to change the base?

make cugraph-ops optional for cugraph-gnn packages #99

Conversation

tingyu66 commented Dec 19, 2024

copy-pr-bot bot commented Dec 19, 2024

tingyu66 commented Dec 19, 2024

bdice Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

CUDA 11 driver present:

CUDA 12 driver present:

No CUDA driver present:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

copy-pr-bot bot commented Dec 22, 2024

tingyu66 commented Dec 22, 2024

jameslamb commented Jan 6, 2025

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Jan 6, 2025

tingyu66 commented Jan 6, 2025

tingyu66 commented Jan 7, 2025

tingyu66 commented Jan 8, 2025

tingyu66 commented Jan 8, 2025

bdice left a comment

Choose a reason for hiding this comment

bdice Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Jan 10, 2025

bdice Dec 19, 2024 •

edited

Loading

bdice Jan 6, 2025 •

edited

Loading

bdice Jan 8, 2025 •

edited

Loading