-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[Model] Update CuGraphRelGraphConv
to use pylibcugraphops=23.02
#5217
Conversation
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
CuGraphRelGraphConv
to use pylibcugraphops=23.02
CuGraphRelGraphConv
to use pylibcugraphops=23.02
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
Are there breaking changes? |
@@ -214,87 +87,68 @@ def __init__( | |||
regularizer=None, | |||
num_bases=None, | |||
bias=True, | |||
activation=None, |
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.
Is this a breaking change?
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. I saw the this comment from RelGraphConv:
dgl/python/dgl/nn/pytorch/conv/relgraphconv.py
Lines 128 to 129 in 76bb540
# TODO(minjie): consider remove those options in the future to make | |
# the module only about graph convolution. |
I also prefer to apply activation outside of the conv layer.
): | ||
if has_pylibcugraphops is False: | ||
raise ModuleNotFoundError( | ||
"dgl.nn.CuGraphRelGraphConv requires pylibcugraphops " | ||
"dgl.nn.CuGraphRelGraphConv requires pylibcugraphops >= 23.02 " |
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.
Is there a way to check the version number of the package?
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.
Any version <23.02 does not have pylibcugraphops.torch.autograd
API, and thus will throw an exception here.
self_loop : bool, optional | ||
True to include self loop message. Default: ``True``. | ||
dropout : float, optional | ||
Dropout rate. Default: ``0.0``. | ||
layer_norm : bool, optional |
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.
Is this a breaking change?
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, same as the "activation" argument above.
@@ -309,57 +163,58 @@ def forward(self, g, feat, etypes, norm=None): | |||
so any input of other integer types will be casted into int32, | |||
thus introducing some overhead. Pass in int32 tensors directly | |||
for best performance. | |||
norm : torch.Tensor, optional |
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.
Is this replaced by apply_norm=True
?
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. This is an additional feature from CuGraphRelGraphConv: supporting the normalized aggregation presented in the RGCN paper.
Hence, users no longer need to compute the norm during training:
dgl/examples/pytorch/rgcn/entity_sample.py
Lines 60 to 61 in 465828c
for block in blocks: | |
block.edata['norm'] = dgl.norm_by_dst(block).unsqueeze(1) |
A 1D tensor of edge norm value. Shape: :math:`(|E|,)`. | ||
max_in_degree : int, optional | ||
Maximum in-degree of destination nodes. It is only effective when | ||
:attr:`g` is a :class:`DGLBlock`, i.e., bipartite graph. When |
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.
Is this still only valid for DGLBlock
?
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
import dgl | ||
from dgl.nn import CuGraphRelGraphConv | ||
from dgl.nn import RelGraphConv | ||
from dgl.nn import CuGraphRelGraphConv, RelGraphConv | ||
|
||
# TODO(tingyu66): Re-enable the following tests after updating cuGraph CI image. |
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.
Is this now re-enabled?
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.
Not yet, I will create a new image after our 23.02 release and update these pytest markers in a separate PR.
@@ -8,19 +8,20 @@ | |||
code changes from the current `entity_sample.py` example. |
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.
Did you see similar performance numbers after running this script?
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, performance is the same as before.
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.
done a pass
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
Apologies for the late reply. The |
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
Not authorized to trigger CI. Please ask core developer to help trigger via issuing comment:
|
…mlc#5217) * update cugraph_relgraphconv * update equality test * update cugraph rgcn example * update RelGraphConvAgg based on latest API changes * enable fallback option to fg when fanout is large --------- Co-authored-by: Mufei Li <[email protected]>
…mlc#5217) * update cugraph_relgraphconv * update equality test * update cugraph rgcn example * update RelGraphConvAgg based on latest API changes * enable fallback option to fg when fanout is large --------- Co-authored-by: Mufei Li <[email protected]>
Description
This PR update
CuGraphRelGraphConv
module to usepylibcugraphops
23.02.With
pylibcugraphops
now offering aggregation autograd functions, we tidy up the source file to only include the nn.Module and make a few improvements.Detailed changes include:
apply_norm
option that enables normalized aggregationW
for better performancemax_in_degree
toforward()
, since it is a property of the graph not the modelChecklist
Please feel free to remove inapplicable items for your PR.
Changes