[cugraph-pyg] Add TransformerConv and support for bipartite node features in GATConv #3489
Conversation
tingyu66
added
feature request
| super().__init__() | ||
|
|
||
| if HAS_PYLIBCUGRAPHOPS is False: | ||
| raise ModuleNotFoundError( |
There was a problem hiding this comment.
You should also actually check for >= 23.04
There was a problem hiding this comment.
We did check this implicitly via
try: # pragma: no cover
from pylibcugraphops.pytorch import (
BipartiteCSC,
SampledCSC,
SampledHeteroCSC,
StaticCSC,
StaticHeteroCSC,
)
as these structures are not available <23.04.
Do you prefer to explicitly check the version?
| def forward( | ||
| self, | ||
| x: Union[torch.Tensor, Tuple[torch.Tensor, torch.Tensor]], | ||
| csc: Tuple[torch.Tensor, torch.Tensor, int], |
There was a problem hiding this comment.
I know that it is like that in upstream but in the longer term, we should have our own graph structure. I am in the process of defining a single class which can represent all our cases. This should make things less opaque and later allow things like using the (optional) "reverse graph structure".
There was a problem hiding this comment.
Yes, we can update all modules in one go after finalizing the god class that you have been working on.
| representation to the desired format. | ||
| edge_attr: (torch.Tensor, optional) The edge features. | ||
| """ | ||
| bipartite = not isinstance(x, torch.Tensor) |
There was a problem hiding this comment.
| bipartite = not isinstance(x, torch.Tensor) | |
| bipartite = isinstance(x, Tuple) |
There was a problem hiding this comment.
This was intended for also accepting a pair in list.
| @@ -179,6 +179,7 @@ if [[ "${RAPIDS_CUDA_VERSION}" == "11.8.0" ]]; then | |||
| --channel "${PYTHON_CHANNEL}" \ | |||
| libcugraph \ | |||
| pylibcugraph \ | |||
| pylibcugraphops \ | |||
There was a problem hiding this comment.
| pylibcugraphops \ |
This list is reserved for only packages that are built as part of the cugraph repository workflow.
If this is a run time dependency, it should be added to the relevant conda recipe and the relevant dependencies.yaml section.
If it's strictly a test dependency, it should only be added to the relevant dependencies.yaml section so it can be used in the conda environment that's generated earlier in this script.
There was a problem hiding this comment.
@ajschmidt8 I would still need to add pylibcugraphops to this file to enable relevant tests in cugraph_pyg.
I understand that this code block:
https://github.com/rapidsai/cugraph/blob/branch-23.06/ci/test_python.sh#L31-L38
is reserved for cugraph packages.
The one that I modified is in test_cugraph_pyg, which is intended for including additional test dependencies (like pytorch) without polluting the dependencies.yaml. @alexbarghi-nv, correct me if I am wrong, since I am not involved in the original discussion of this design.
If you look at the code block for test_cugraph_dgl, pylibcugraphops is already there.
Co-authored-by: AJ Schmidt <ajschmidt8@users.noreply.github.com>
|
This PR now also adds GATv2Conv model. |
|
/merge |
Fixes https://github.com/rapidsai/graph_dl/issues/180, fixes https://github.com/rapidsai/graph_dl/issues/182, fixes https://github.com/rapidsai/graph_dl/issues/184.
CC: @MatthiasKohl @stadlmax