Skip to content

Conversation

@alexbarghi-nv
Copy link
Contributor

@alexbarghi-nv alexbarghi-nv commented Feb 27, 2025

Removes unbuffered sampling (dumping data to disk) since it is about to be deprecated and it has numerous bugs that we do not intend to fix. Buffered sampling does not require writing to disk, and is significantly faster as shown in the benchmarks below.

Buffered sampling was introduced in rapidsai/cugraph-gnn#48 as part of the overhaul to add support for negative sampling and general link prediction workflows in cuGraph-PyG.

Performance Analysis (w/ GraphSAGE):

Dataset GPUs Epoch Time (unbuffered) Epoch Time (buffered) Speedup
ogbn-arxiv 1x A100 80GB 14 s 5s 2.8x
ogbn-arxiv 2x A100 80GB 14 s 4s 3.5x
ogbn-products 1x A100 80GB 69 s 45 s 1.5x
ogbn-products 2x A100 80GB 43 s 25 s 1.7x
ogbn-products 4x A100 80GB 42 s 19 s 2.2x
ogbn-papers100M 1x A100 80GB 479 s 375 s 1.3x
ogbn-papers100M 2x A100 80GB 435 s 305 s 1.4x
ogbn-papers100M 4x A100 80GB 207 s 120 s 1.7x
ogbn-papers100M 8x A100 80GB 169 s 100 s 1.7x

alexbarghi-nv and others added 22 commits June 1, 2022 17:27
remove use of __dict__

Co-authored-by: Matthias Fey <[email protected]>
remove use of __dict__

Co-authored-by: Matthias Fey <[email protected]>
simplify isinstance syntax

Co-authored-by: Matthias Fey <[email protected]>
simplify isinstance call

Co-authored-by: Matthias Fey <[email protected]>
remove explicit inheritance from object

Co-authored-by: Matthias Fey <[email protected]>
remove direct __dict__ access

Co-authored-by: Matthias Fey <[email protected]>
remove direct __dict__ access

Co-authored-by: Matthias Fey <[email protected]>
Create Initial Version of cuGraph Storage and Data Classes
@alexbarghi-nv
Copy link
Contributor Author

@puririshi98 @drivanov

@puririshi98
Copy link
Contributor

ty will review by eod

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.

code LGTM but can you share logs of running this w/ master branch and your branch to show the diff in outputs and speed. also please share the device specs for context

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.

It'd be nice to have some reference to details on buffered/unbuffered sampling in the PR description, but LGTM!

@alexbarghi-nv
Copy link
Contributor Author

Just added single-GPU benchmarks, working on multi-GPU now...

@alexbarghi-nv
Copy link
Contributor Author

@puririshi98 could you please re-review? I have added benchmarks indicating the speedup from buffered sampling and linked to the PR that introduced the feature in cugraph-pyg.

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.

good to go

@puririshi98 puririshi98 merged commit 4845c6c into pyg-team:master Mar 5, 2025
16 checks passed
michaelfortunato pushed a commit to michaelfortunato/pytorch_geometric that referenced this pull request Mar 30, 2025
Removes unbuffered sampling (dumping data to disk) since it is about to
be deprecated and it has numerous bugs that we do not intend to fix.
Buffered sampling does not require writing to disk, and is significantly
faster as shown in the benchmarks below.

Buffered sampling was introduced in
rapidsai/cugraph-gnn#48 as part of the overhaul
to add support for negative sampling and general link prediction
workflows in cuGraph-PyG.

Performance Analysis (w/ GraphSAGE):
| Dataset | GPUs | Epoch Time (unbuffered) | Epoch Time (buffered) |
Speedup |

|----------|-------|------------------------|---------------------|------------|
| ogbn-arxiv | 1x A100 80GB | 14 s | 5s | 2.8x
| ogbn-arxiv | 2x A100 80GB | 14 s | 4s | 3.5x
| ogbn-products | 1x A100 80GB | 69 s | 45 s | 1.5x
| ogbn-products | 2x A100 80GB | 43 s | 25 s | 1.7x
| ogbn-products | 4x A100 80GB | 42 s | 19 s | 2.2x
| ogbn-papers100M | 1x A100 80GB | 479 s | 375 s | 1.3x
| ogbn-papers100M | 2x A100 80GB | 435 s | 305 s | 1.4x
| ogbn-papers100M | 4x A100 80GB | 207 s | 120 s | 1.7x
| ogbn-papers100M | 8x A100 80GB | 169 s | 100 s | 1.7x

---------

Co-authored-by: Matthias Fey <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Rishi Puri <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>
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