Skip to content
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

Add compatibility layer for PackedGraph #84

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

cthoyt
Copy link
Contributor

@cthoyt cthoyt commented Feb 9, 2022

Summary

This PR adds a compatibility layer for the packed graph class from torch drug while we're waiting for an upstream fix (DeepGraphLearning/torchdrug#70). This layer makes sure there's a to() function that takes a device, as we usually expect torch stuff to do.

  • Unit tests provided for these changes
  • Documentation and docstrings added for these changes using the sphinx style

Changes

  • Add a new module chemicalx.compat
  • Implement a subclass of torchdrug.data.PackedGraph that implements the to() function
  • Implement subclass of torchdrug.data.Graph that uses the monkey patched PackedGraph when called in chemicalx.data.drugfeatureset

@codecov-commenter
Copy link

Codecov Report

Merging #84 (081a117) into main (040fc7b) will decrease coverage by 0.83%.
The diff coverage is 58.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   95.84%   95.00%   -0.84%     
==========================================
  Files          32       33       +1     
  Lines        1419     1441      +22     
==========================================
+ Hits         1360     1369       +9     
- Misses         59       72      +13     
Impacted Files Coverage Δ
chemicalx/compat.py 38.09% <38.09%> (ø)
chemicalx/data/batchgenerator.py 98.30% <100.00%> (ø)
chemicalx/data/drugfeatureset.py 100.00% <100.00%> (ø)
chemicalx/data/drugpairbatch.py 100.00% <100.00%> (ø)
chemicalx/models/deepdds.py 97.43% <100.00%> (ø)
chemicalx/models/deepdrug.py 96.77% <100.00%> (ø)
chemicalx/models/epgcnds.py 100.00% <100.00%> (ø)
chemicalx/models/gcnbmp.py 97.61% <100.00%> (ø)
chemicalx/models/mrgnn.py 100.00% <100.00%> (ø)
chemicalx/models/ssiddi.py 98.73% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 040fc7b...081a117. Read the comment docs.

@benedekrozemberczki
Copy link
Contributor

A GPU equipped dedicated runner could work, let us look into this.

cthoyt added a commit to cthoyt/chemicalx that referenced this pull request Feb 10, 2022
Closes AstraZeneca#76. This PR first requires AstraZeneca#84 to be tested and merged.

## Blocked by

- [ ] AstraZeneca#84
@cthoyt
Copy link
Contributor Author

cthoyt commented Feb 10, 2022

@benedekrozemberczki I just added a unit test that should demonstrate that the transfer to GPU works (but it only runs if a GPU is available, so the tests will pass on the current runner regardless)

@cthoyt cthoyt marked this pull request as ready for review February 10, 2022 12:12
@cthoyt
Copy link
Contributor Author

cthoyt commented Feb 10, 2022

Good news! I tested the following on a google colab notebook with GPU embedded:

!python -m pip install torch
!python -m pip install "torch-scatter>=2.0.8"
!python -m pip install git+https://github.com/cthoyt/chemicalx.git@packed-graph-compat

import torch.cuda
from torchdrug.data import Molecule

from chemicalx.compat import Graph, PackedGraph

assert torch.cuda.is_available(), "can not test compatibility layer without a GPU available"

gpu_device = torch.device("cuda")

structures = ["C(=O)O", "CCO"]
molecules = [Molecule.from_smiles(smiles) for smiles in structures]
packed_graph = Graph.pack(molecules)
assert isinstance(packed_graph, PackedGraph)

assert "cpu" == packed_graph.edge_list.device.type
assert -1 == packed_graph.edge_list.get_device(), "Device should be -1 to represent it's on the CPU"

gpu_packed_graph = packed_graph.to(gpu_device)
assert "cuda" == gpu_packed_graph.edge_list.device.type
assert 0 <= gpu_packed_graph.edge_list.get_device(), "Device should 0 or higher for a cuda device"


assert packed_graph is not gpu_packed_graph, "The PackedGraph.cuda() creates a new object. There is no in-place versions of this, unfortunately"

so I am confident that this PR does what's intended.

@benedekrozemberczki
Copy link
Contributor

benedekrozemberczki commented Feb 11, 2022

@cthoyt this is amazing!

@benedekrozemberczki
Copy link
Contributor

@cthoyt Feel free to merge this!

@cthoyt cthoyt merged commit f48d70b into AstraZeneca:main Feb 11, 2022
@cthoyt cthoyt deleted the packed-graph-compat branch February 11, 2022 13:56
benedekrozemberczki pushed a commit that referenced this pull request Feb 11, 2022
* Begin adding compatibility layer for packed graph

* Fix import

* Add compatibility layer for packing

* Update gcnbmp.py

* Add GPU support to pipeline

Closes #76. This PR first requires #84 to be tested and merged.

## Blocked by

- [ ] #84

* Update pipeline.py

* Reduce code with better type hints

* Update pipeline.py
@cthoyt cthoyt mentioned this pull request Feb 12, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants