Skip to content

Minor fix for pyg-lib usage in HeteroLinear and RGCNConv#5510

Merged
rusty1s merged 17 commits into
masterfrom
minor_fix_for_CI
Oct 12, 2022
Merged

Minor fix for pyg-lib usage in HeteroLinear and RGCNConv#5510
rusty1s merged 17 commits into
masterfrom
minor_fix_for_CI

Conversation

@puririshi98
Copy link
Copy Markdown
Contributor

@puririshi98 puririshi98 commented Sep 22, 2022

pyg_lib pathway is chosen if cuda is available and pyg_lib is available. but if the input is not cuda we should not be using the pyg_lib pathway. simple fix

without this a ton of CI fails for testing rgcnconv and heterolinear w/ cpu inputs

errors.txt

@github-actions github-actions Bot added the nn label Sep 22, 2022
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 22, 2022

Codecov Report

Merging #5510 (38f2a49) into master (3733006) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5510      +/-   ##
==========================================
+ Coverage   83.84%   83.86%   +0.02%     
==========================================
  Files         349      349              
  Lines       19231    19231              
==========================================
+ Hits        16124    16128       +4     
+ Misses       3107     3103       -4     
Impacted Files Coverage Δ
torch_geometric/nn/conv/rgcn_conv.py 95.39% <100.00%> (+1.31%) ⬆️
torch_geometric/nn/dense/linear.py 87.21% <100.00%> (+3.75%) ⬆️
torch_geometric/utils/scatter.py 66.66% <0.00%> (-33.34%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EdisonLeeeee
Copy link
Copy Markdown
Contributor

Use x.is_cuda for device check?

@rusty1s rusty1s changed the title minor fix for pyg_lib heteroconv and rgcnconv Minor fix for pyg-lib usage in HeteroConv and RGCNConv Sep 23, 2022
@rusty1s
Copy link
Copy Markdown
Member

rusty1s commented Sep 23, 2022

This PR will currently break TorchScript functionality since we cannot check for device within a TorchScript program (that's why we have this awkward

self._WITH_PYG_LIB = torch.cuda.is_available() and _WITH_PYG_LIB

check as a workaround in __init__. I suggest that we simply integrate CPU-based segment_matmul support in pyg-lib via the classic for-loop instead.

@rusty1s
Copy link
Copy Markdown
Member

rusty1s commented Sep 23, 2022

cc @dszwicht

@DamianSzwichtenberg
Copy link
Copy Markdown
Member

@rusty1s I have had a minimal implementation of [segment | grouped]_matmul ready for some time. Was working on optimization, but in such a case I'll create a PR with basic solution later today 😉

@puririshi98 puririshi98 changed the title Minor fix for pyg-lib usage in HeteroConv and RGCNConv Minor fix for pyg-lib usage in HeteroLinear and RGCNConv Sep 23, 2022
@puririshi98
Copy link
Copy Markdown
Contributor Author

okay id assume @dszwicht will PR that to pyg_lib. in that case then
self._WITH_PYG_LIB = torch.cuda.is_available() and _WITH_PYG_LIB
should change to
self._WITH_PYG_LIB = _WITH_PYG_LIB

then once pyg_lib is updated all pathways should work

@rusty1s
Copy link
Copy Markdown
Member

rusty1s commented Sep 25, 2022

Indeed. PR to pyg-lib just landed, so this should be resolved soon.

@puririshi98 puririshi98 enabled auto-merge (squash) September 26, 2022 17:47
@puririshi98
Copy link
Copy Markdown
Contributor Author

pyg-team/pyg-lib#111
this should be merged now since the associated PR from intel is in

@puririshi98 puririshi98 requested a review from rusty1s October 3, 2022 17:45
@rusty1s
Copy link
Copy Markdown
Member

rusty1s commented Oct 3, 2022

Thanks for the update @puririshi98. I noticed that segment_matmul still does not support TorchScript because the Python implementation via torch.autograd.Function does not come with TorchScript support. Now that the dispatching of segment_matmul is resolved in pyg-lib, can we move its backward implementation from Python to C++? Let me know if you have time working on this!

@rusty1s rusty1s disabled auto-merge October 12, 2022 13:26
@rusty1s rusty1s merged commit 6bda075 into master Oct 12, 2022
@rusty1s rusty1s deleted the minor_fix_for_CI branch October 12, 2022 13:39
jjpietrak pushed a commit to jjpietrak/pytorch_geometric that referenced this pull request Nov 25, 2022
…eam#5510)

pyg_lib pathway is chosen if cuda is available and pyg_lib is available.
but if the input is not cuda we should not be using the pyg_lib pathway.
simple fix

without this a ton of CI fails for testing rgcnconv and heterolinear w/
cpu inputs

[errors.txt](https://github.com/pyg-team/pytorch_geometric/files/9629902/errors.txt)

Co-authored-by: Matthias Fey <matthias.fey@tu-dortmund.de>
marekdedic pushed a commit to marekdedic/pytorch_geometric that referenced this pull request Apr 30, 2026
…eam#5510)

pyg_lib pathway is chosen if cuda is available and pyg_lib is available.
but if the input is not cuda we should not be using the pyg_lib pathway.
simple fix

without this a ton of CI fails for testing rgcnconv and heterolinear w/
cpu inputs


[errors.txt](https://github.com/pyg-team/pytorch_geometric/files/9629902/errors.txt)

Co-authored-by: Matthias Fey <matthias.fey@tu-dortmund.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants