add PR CI for cugraph-pyg and cugraph-dgl #59
Conversation
jameslamb
added
improvement
…ey should express them
jameslamb
changed the title
|
🥳 this is now ready for review! Thanks very much for all the help @alexbarghi-nv . |
… into add-pyg-and-dgl-ci
There was a problem hiding this comment.
Just looked this over one last time; I think this is ready to be merged. Thanks for all your hard work @jameslamb
.gitignore
Outdated
| dataset/ | ||
| datasets/ |
There was a problem hiding this comment.
Do we really need both of these paths? Why?
There was a problem hiding this comment.
Good point, there's an opportunity to simplify here.
I added dataset/ because I saw files created at python/cugraph-pyg/cugraph_pyg/dataset/ when running the cugraph-pyg examples locally.
Looks like that comes from here: https://github.com/snap-stanford/ogb/blob/f631af76359c9687b2fe60905557bbb241916258/ogb/nodeproppred/dataset.py#L13
I just pushed changes writing all those datasets to datasets/ instead, so we don't need to have both rules in gitignore.
ci/build_python.sh
Outdated
| RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \ | ||
| --no-test \ | ||
| --channel "${CPP_CHANNEL}" \ | ||
| --channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \ |
There was a problem hiding this comment.
Do we need ${RAPIDS_CONDA_BLD_OUTPUT_DIR} here? What packages is it supplying? I don't see pylibwholegraph in the dependencies of cugraph-pyg or cugraph-dgl.
Same for cugraph-dgl below.
There was a problem hiding this comment.
You're totally right. pylibwholegraph is a testing dependency of both cugraph-pyg and cugraph-dgl, but not a build-time dependency, so --channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" is unnecessary. I just pushed 1202c36 removing it.
ci/test_wheel_cugraph-pyg.sh
Outdated
| # Used to skip certain examples in CI due to memory limitations | ||
| export CI_RUN=1 |
There was a problem hiding this comment.
Typically the environment variable CI is set to true in CI contexts. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
We could use that to avoid introducing a separate variable.
There was a problem hiding this comment.
Yeah great point! Looks to me like this was carried over from cugraph, but there it's only used for the narrow purpose of cugraph-pyg, so we can just simplify it here.
Did that in 1202c36
|
It seems like there are opportunities to move from CUDA 12.1 to 12.4 and drop the |
Yep absolutely! That's a good next step. Thanks for reviewing, I do think we should move it forward once you feel like all your questions have been addressed. The sooner we get CI fully working in this repo, the sooner we can cut all this stuff out of the |
| @@ -9,7 +9,7 @@ cd "$(dirname "$(realpath "${BASH_SOURCE[0]}")")"/../python/cugraph-pyg/cugraph_ | |||
| pytest --cache-clear --benchmark-disable "$@" . | |||
|
|
|||
| # Used to skip certain examples in CI due to memory limitations | |||
| export CI_RUN=1 | |||
| export CI=true | |||
There was a problem hiding this comment.
If we export this, it means that local executions of this script will also skip those tests. I'm not sure if that is really intended or not.
There was a problem hiding this comment.
I think the cugraph developers intended that. @alexbarghi-nv introduced that in rapidsai/cugraph#4384
|
/merge |
Closes #53 This pulls over the remaining changes from that PR... thought that was easier than fixing the merge conflicts between that and recent PRs. * adds notebook testing * adds all the remaining `build.sh` changes from #53 (mostly for interactive development) * adds `cugraph-pyg` and `cugraph-dgl` tests in `conda-python-tests` (missed in #59) ## Notes for Reviewers I left other comments explaining these changes inline. Authors: - James Lamb (https://github.com/jameslamb) - Tingyu Wang (https://github.com/tingyu66) Approvers: - Tingyu Wang (https://github.com/tingyu66) - Ray Douglass (https://github.com/raydouglass) URL: #62
Another steps towards completing the work started in #53
Fixes #15
Contributes to rapidsai/build-planning#111
Proposes changes to get CI running on pull requests for
cugraph-pygandcugraph-dglNotes for Reviewers
Workflows for nightly builds and publishing nightly packages are intentionally not included here. See #58 (comment)
Notebook tests are intentionally not added here... they'll be added in the next PR.
Pulls in changes from these other upstream PRs that had not been ported over to this repo: