-
Notifications
You must be signed in to change notification settings - Fork 34
Remove cugraph Python library as a dependency #271
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 cugraph Python library as a dependency #271
Conversation
|
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. |
alexbarghi-nv
left a comment
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.
Looks good, I think we can move this out of draft.
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.
Came here looking for this because I was excited to see the note in @alexbarghi-nv's T5T saying the cugraph dependency had been removed from cugraph-pyg.
I can help you with a packaging-codeowners review and finishing this up.
Please also remove the dependency from the cugraph-pyg conda packages:
| - cugraph =${{ minor_version }} |
It also looks to me like there are more places where depends_on_cugraph could be removed in dependencies.yaml. If my comment about the use of import_optional and MissingModule in tests is correct, then it looks to me like depends_on_cugraph should just fully be removed from dependencies.yaml.
Otherwise, I see at least 2 specific cases that should be removed.
It looks to me like maybe the entire test_python group is unused and could be removed:
Lines 58 to 67 in 0f1f838
| test_python: | |
| output: none | |
| includes: | |
| - cuda_version | |
| - depends_on_cugraph | |
| - depends_on_cudf | |
| - depends_on_pytorch | |
| - depends_on_cuml | |
| - py_version | |
| - test_python_common |
And I don't think pylibwholegraph needs cugraph for its tests:
Line 87 in 0f1f838
| - depends_on_cugraph |
…om/rlratzel/cugraph-gnn into branch-25.10-decouple_from_cugraph
|
@jameslamb can you re-review? I removed the unused dependencies as well. |
|
@jameslamb I think I may have removed some of the dependency lists that are actually being used. The problem is it's not clear which are being used when I look at |
jameslamb
left a comment
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.
Thanks for the changes! The dependencies.yaml changes break the link between that file and wheels (i.e. remove automation of pyproject.toml updates), so some of them should be reverted.
I also think it's worth taking another pass over this branch looking for other places to remove. I pulled Rick's branch and ran this from the root:
git grep -E 'cugraph\.gnn`And found more cases that should be updated. For example, there are still some lingering cugraph.gnn.cugraph_comms_init uses in tests and examples that I think you want to move to pylibcugraph.comms.cugraph_comms_init.
| "cugraph==25.10.*,>=0.0.0a0", | ||
| "cuml==25.10.*,>=0.0.0a0", | ||
| "ogb", | ||
| "pylibcugraph==25.10.*,>=0.0.0a0", |
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.
Generally, these extras like [test] should not contain anything that's already a hard runtime dependency of the project (in [project].dependencies).
Having overlap here means we can miss packaging issues like "hard runtime dependency not listed as such", which might not be caught in CI (where we always install the [test] extra).
But this issue of duplicates looks like it predates this PR (pylibwholegraph and torch-geometric pins are also duplicated across these lists). If you agree with my assessment, I can push a follow-up PR after this to fix that.
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.
Yes, we should definitely fix that.
| - test_python_pylibwholegraph | ||
| py_build_libwholegraph: | ||
| output: pyproject | ||
| pyproject_dir: python/libwholegraph |
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.
This entry may look unused if you do something like git grep py_build_libwholegraph, but it's not... the rapids-dependency-file-generator pre-commit hook uses this to populate python/libwholegraph/pyproject.toml, and rapids-build-backend uses it at build time to populate dependency metadata for wheels.
Everything in the diff here with a non-empty pyproject_dir: entry should be put back, please.
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.
I think this is fixed.
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
| - depends_on_mkl | ||
| - depends_on_pylibwholegraph | ||
| - depends_on_cugraph_pyg | ||
| test_pylibwholegraph: |
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.
It looks like this group is used right here:
Line 74 in 9ecbc66
| --file-key test_pylibwholegraph \ |
Removing this is why the tests are failing like this:
./ci/run_pylibwholegraph_pytests.sh: line 9: pytest: command not found
I think that test_pylibwholegraph and test_cugraph_pyg should be restored (but depends_on_cugraph should be removed from test_pylibwholegraph because it looks like cugraph is not needed when testing pylibwholegraph).
git grep cugraph python/pylibwholegraph/
# (empty)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.
I'll look into why this didn't cause the build to fail earlier, at this part:
Traceback (most recent call last):
File "/opt/conda/bin/rapids-dependency-file-generator", line 10, in <module>
sys.exit(main())
File "/opt/conda/lib/python3.10/site-packages/rapids_dependency_file_generator/_cli.py", line 136, in main
make_dependency_files(
File "/opt/conda/lib/python3.10/site-packages/rapids_dependency_file_generator/_rapids_dependency_file_generator.py", line 410, in make_dependency_files
file_config = parsed_config.files[file_key]
KeyError: 'test_cugraph_pyg'
[rapids-conda-retry] timeout for conda operations: '45m'
Empty environment created at prefix: /opt/conda/envs/test_cugraph_pyg
That might be a bug in rapids-dependency-file-generator.
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.
Ah, not a bug, this trapping prevents an early exit:
Line 28 in 9ecbc66
| EXITCODE=0 |
And looks like that's there intentionally so the job runs all test stuff instead of failing immediately. So nothing needs to be changes about rapids-dependency-file-generator ... but yes these groups need to be restored in dependencies.yaml.
…om/rlratzel/cugraph-gnn into branch-25.10-decouple_from_cugraph
|
@jameslamb I think we are good to go now. |
jameslamb
left a comment
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.
Looks good! Thanks for doing this, really nice simplification to the RAPIDS dependency graph.
I'll put up a follow-up PR with the changes mentioned in https://github.com/rapidsai/cugraph-gnn/pull/271/files#r2274062952
|
/merge |
…cies from 'test' extra (#277) Follow-up from #271 Explicitly declares runtime dependencies on libraries that `cugraph-pyg` unconditionally imports. `cupy`: https://github.com/rapidsai/cugraph-gnn/blob/b10f279cb855b2cfc52239d2d7973c01c92c2e5f/python/cugraph-pyg/cugraph_pyg/data/graph_store.py#L17 `packaging`: https://github.com/rapidsai/cugraph-gnn/blob/b10f279cb855b2cfc52239d2d7973c01c92c2e5f/python/cugraph-pyg/cugraph_pyg/utils/imports.py#L14 Removes `pylibcugraph`, `pylibwholegraph`, and `torch-geometric` from `[test]` extra (these are already in `[project].dependencies`). Fixes homepage URL in `cugraph-pyg` project metadata. ## Notes for Reviewers ### What should we do with `torch`? ~There are a mix of conditional and unconditional imports of `torch` in `cugraph-pyg`:~ ~https://github.com/rapidsai/cugraph-gnn/blob/b10f279cb855b2cfc52239d2d7973c01c92c2e5f/python/cugraph-pyg/cugraph_pyg/tensor/dist_tensor.py#L25~ ~https://github.com/rapidsai/cugraph-gnn/blob/b10f279cb855b2cfc52239d2d7973c01c92c2e5f/python/cugraph-pyg/cugraph_pyg/loader/utils.py#L14~ ~Unconditional imports really should mean declaring a hard runtime dependency, but I vaguely recall that in the past, we intentionally did not declare a `torch` dependency in `cugraph-pyg` wheels. Am I right about that? @alexbarghi-nv can you remind me why we don't declare that dependency? If I'm right, should those unconditional imports be made conditional with `import_optional()`?~ We don't want to declare it: #277 (comment) ### Benefits of this Reduces the risk of import issues when users install `cugraph-pyg` in environments that don't happen to have these dependencies from other things (similar to rapidsai/cuml#6420). Improves the chance of catching packaging issues in CI. Authors: - James Lamb (https://github.com/jameslamb) - Alex Barghi (https://github.com/alexbarghi-nv) Approvers: - Bradley Dice (https://github.com/bdice) URL: #277
Closes #249