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

Properly support dynamic inheritance in Batch #5051

Merged
merged 9 commits into from
Jul 26, 2022
Merged

Conversation

mananshah99
Copy link
Contributor

@mananshah99 mananshah99 commented Jul 25, 2022

This diff properly supports dynamic inheritance with the DynamicInheritance metaclass in Batch. The initial issue was that of conflicting metaclasses: abc.ABCMeta and DynamicInheritance for Data and HeteroData objects. The resolution required two steps:

  • Metaclass conflict resolution of the aforementioned metaclasses
  • Patching pin_memory in PyTorch which makes assumptions about MutableMapping before checking whether pin_memory exists in the class. This in turn had adverse effects of calling the metaclass of the class again when constructing the new class as part of pin_memory, which resulted in the DataDataBatch problem observed in the filed issue.

Hopefully the latter fix can be deprecated in the future, with a patch to PyTorch (see pytorch/pytorch#82185)

Closes #4848

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #5051 (5b4027e) into master (0e2d987) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

❗ Current head 5b4027e differs from pull request most recent head c4507eb. Consider uploading reports for the commit c4507eb to get more accurate results

@@            Coverage Diff             @@
##           master    #5051      +/-   ##
==========================================
- Coverage   82.90%   82.89%   -0.02%     
==========================================
  Files         331      331              
  Lines       18150    18160      +10     
==========================================
+ Hits        15048    15053       +5     
- Misses       3102     3107       +5     
Impacted Files Coverage Δ
torch_geometric/loader/dataloader.py 74.46% <50.00%> (-6.62%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, didn’t know it is due to pin_memory. Can we add a test which might have failed without this patch?

torch_geometric/loader/dataloader.py Outdated Show resolved Hide resolved
torch_geometric/loader/dataloader.py Outdated Show resolved Hide resolved
@mananshah99
Copy link
Contributor Author

Can we add a test which might have failed without this patch?

Since this is an issue with pin_memory, it will be hard to verify this on CPU since torch _BaseDataLoaderIter internally sets pin_memory to False without CUDA. We could define a custom dataloader iterator that overrides this functionality and write a test for this, but that might be overkill :)

@mananshah99 mananshah99 enabled auto-merge (squash) July 26, 2022 18:29
@mananshah99 mananshah99 merged commit ac6119b into master Jul 26, 2022
@mananshah99 mananshah99 deleted the fix_data_batch branch July 26, 2022 18:31
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.

Data Batch problem in PyG
2 participants