Skip to content

Conversation

@guanxingithub
Copy link
Contributor

We consolidate cugraph training with
Models: GAT, GCN, GraphSAGE
Datasets: ogbn-arxiv, ogbn-products, ogbn-papers100M
examples/ogbn_train_cugraph.py is the script for single gpu training
examples/multi_gpu/ogbn_train_cugraph_multigpu.py is the script for multiple gpu training.

@guanxingithub guanxingithub requested a review from wsad1 as a code owner January 15, 2025 19:21
Update cugraph examples
@puririshi98
Copy link
Contributor

@guanxingithub, as mentioned before please delete the old cugraph examples that you based these new files on, otherwise there will be duplicates.

@puririshi98
Copy link
Contributor

also please attach logs of these scripts running succesfully on your setup for review

@guanxingithub
Copy link
Contributor Author

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @guanxingithub! This is not blocking you from merging this PR, but what would be so nice before merging this PR would be to profile some parts of the scripts and make sure there's no basic inefficient code :)

akihironitta
akihironitta previously approved these changes Jan 21, 2025
Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work :)

@akihironitta akihironitta self-requested a review January 21, 2025 17:49
@akihironitta akihironitta dismissed their stale review January 21, 2025 17:50

accidentally pressed approve

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I'd like these cugraph examples to be as performant as possible :) Let's merge this once comments are addressed and this PR is cleaned up :)

@guanxingithub
Copy link
Contributor Author

@puririshi98
Copy link
Contributor

@akihironitta reviewing the profile by @guanxingithub, it does not seem like there are any bottlenecks, i will leave to you to decide if we can merge if this investigation satisfies your concerns

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also make sure README.md also gets updated?

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@puririshi98 I'm not sure you have already, but would you mind reviewing this PR as well?

@puririshi98
Copy link
Contributor

i have but ill do another omnday

@puririshi98
Copy link
Contributor

Can you also make sure README.md also gets updated?

good catch

Copy link
Contributor

@puririshi98 puririshi98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, was also reviewed by Alexandria, our cugraph expert. @akihironitta lmk if anything else needed to merge

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guanxingithub @puririshi98 @alexbarghi-nv Thanks for bearing with me ❤️ I'll test this out and get it merged tonight!

Copy link
Member

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't managed to run, but LGTM!

@puririshi98 puririshi98 merged commit aa6cf80 into pyg-team:master Jan 29, 2025
16 checks passed
@guanxingithub guanxingithub deleted the cugraph_examples branch February 28, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants