-
Notifications
You must be signed in to change notification settings - Fork 304
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
Update cugraph-pyg Recipe and CI Script #3288
Update cugraph-pyg Recipe and CI Script #3288
Conversation
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.
LGTM . Thanks for working on this Alex. This resolves a important P0 for this release.
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.
We might need to brainstorm on how to get this properly working in CI.
Although the tests are currently passing, I'm seeing both the cudatoolkit
and cuda-toolkit
packages being installed in our CI test environments now, which is not something that we want.
Additionally, the cuda-toolkit
package version that is being installed doesn't match the cudatoolkit
version. See the links below for an example on the 11.2.2
test job:
cudatoolkit
11.2.2
is installed (which is what we want) - https://github.com/rapidsai/cugraph/actions/runs/4200038972/jobs/7286563545#step:6:87cuda-toolkit
11.7.1
is installed shortly after - https://github.com/rapidsai/cugraph/actions/runs/4200038972/jobs/7286563545#step:6:513
I'm not very familiar with the cugraph
packages that are being changed here.
If they have dependencies that conflict with the other RAPIDS libraries, we might want to consider creating an entirely new conda environment in this test job to accommodate testing these packages.
Happy to hop on a call to chat about this further if needed.
I want to mention that we could also move the Instead, it would only test against a single CUDA version (e.g. To make this work, you could adapt the
The only changes you would need to make are:
|
…to update-ci-for-pyg
@rlratzel and @ajschmidt8 could you please re-review this PR? |
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.
LGTM
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.
Look good!
Looks like the wrong cugraph-pyg
version is getting installed though. We'll want to get that sorted out.
ci/test_python.sh
Outdated
rapids-print-env | ||
|
||
rapids-mamba-retry install \ | ||
--channel "${CPP_CHANNEL}" \ | ||
--channel "${PYTHON_CHANNEL}" \ | ||
libcugraph \ | ||
pylibcugraph \ | ||
cugraph \ | ||
cugraph-pyg | ||
|
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.
Two quick notes:
rapids-print-env
does aconda list
(src). so it might be worth placing that command after all the mamba install commands.- I notice that CI is installing the wrong
cugraph-pyg
version (see here). This is probably from a dependency issue somewhere. It might take a bit of iterating to figure out why this is happening. I wrote some docs about reproducing CI locally here that might help. Happy to hop on a call to help or discuss this more if needed.
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, just possibly one change request.
conda/recipes/cugraph-pyg/meta.yaml
Outdated
- pytorch >=2.0 | ||
- cupy >=9.5.0,<12.0.0a0 | ||
- cugraph ={{ version }} | ||
- pyg-nightly |
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.
just want to confirm that we're putting a nightly dependency in our release packages, or are we planning on changing this before code freeze? If not, can we put an upper-bound pin on this so users installing this package some time later don't pull in a possibly incompatible newer nightly version?
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 just changed this now to resolve the issue of pulling the wrong version of cugraph-pyg. We will need to bind to pyg 2.3 once it is released, which is supposed to be next week.
/merge |
Updates tests to support presence of PyTorch and PyG in the environment. Updates cugraph-pyg to include support for loading from samples on disk. Makes CuGraphStore serializable and allows creating CuGraphStore and BulkSampleLoader instances without the graph structure. Merge after #3288 Closes #3287 Closes #3176 Authors: - Alex Barghi (https://github.com/alexbarghi-nv) Approvers: - Vibhu Jawa (https://github.com/VibhuJawa) - Rick Ratzel (https://github.com/rlratzel) URL: #3289
- Adds new examples for cugraph-pyg. - Removes outdated examples. - Moves MG scripts to top-level directory. - Makes the input to `_get_vertex_groups_from_sample` a tensor instead of Series - Adds `is_sorted` arg to `_get_vertex_groups_from_sample` to skip sorting if tensor already sorted - Some fixes to `CuGraphStore` for running multi-GPU workflows Merge after #3288 - merged Merge after #3289 - merged Merge after #3382 - merged Closes #3316 Closes #3226 Closes #3072 Authors: - Alex Barghi (https://github.com/alexbarghi-nv) Approvers: - Vibhu Jawa (https://github.com/VibhuJawa) - Rick Ratzel (https://github.com/rlratzel) URL: #3326
Properly installs torch in test script; skips PyG tests on CUDA 11.8. Removes OGB from dependencies since it is no longer needed and created a version conflict. Properly adds PyG and PyTorch as dependencies of cugraph-pyg.
Closes #3083
Closes #3225