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

updated dgl dependency #268

Merged
merged 3 commits into from
Nov 18, 2022
Merged

Conversation

guillaume-be
Copy link
Contributor

Issue #, if available: #256

Description of changes:

  • Update the Graph structure to leverage the (deprecated) dgl._deprecate.graph.DGLGraph. Effectively uses a deprecated class in a recent version of DGL instead of using a deprecated version of the DGL package.
  • Updates dgl dependency from ==0.4.3post2 to >~0.5, <~0.9, allowing sharing a recent DGL package installation with other applications, and allowing use of Python binaries for 3.9+

Integration tests have been updated and are running on my local environment.
I have re-ran training on FB15k to validate that the results are as expected, here are the output of the script:

TransE_l1_FB15k_0
Test average MRR : 0.6469265924837936
Test average MR : 47.05067630478577
Test average HITS@1 : 0.5219143065125019
Test average HITS@3 : 0.7437236545851602
Test average HITS@10 : 0.8403954563152816

TransE_l2_FB15k_0
Test average MRR : 0.6384227261417044
Test average MR : 39.936754075603936
Test average HITS@1 : 0.5046384858898614
Test average HITS@3 : 0.743528973608031
Test average HITS@10 : 0.8484196983291293

DistMult_FB15k_0
Test average MRR : 0.6508987590985638
Test average MR : 55.72816610519544
Test average HITS@1 : 0.5342046012425725
Test average HITS@3 : 0.7367828545309881
Test average HITS@10 : 0.84824194613262

ComplEx_FB15k_0
Test average MRR : 0.7075530292466494
Test average MR : 60.922652401347534
Test average HITS@1 : 0.6077686174264868
Test average HITS@3 : 0.7832100353811515
Test average HITS@10 : 0.8677015794552319

The full training logs are available on this gist

else
pip3 uninstall dgl
pip3 install dgl-cu102==0.4.3post2
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment about what is the latest dgl version that works with this patch. As we are using dgl._deprecate.graph.DGLGraph, this interface may be deleted in some date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @classicsong , I have added a minimum and maximum dependency for the tests

Copy link
Contributor

@lroberts7 lroberts7 Dec 7, 2022

Choose a reason for hiding this comment

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

This breaks the partition method (here: https://github.com/awslabs/dgl-ke/blob/master/python/dglke/partition.py#L119 ) because dgl switched the attribute name from dgl.transform to dgl.transforms in version 0.8.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/dmlc/dgl/releases/tag/0.8.0 under Graph Dataset and Transforms section

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm using version 0.9.1 of dgl and got this error. A reasonable work around for this would be to switch using the Version class in packaging.version.Version on version 0.8. I'll open a CR for this, (unless someone from this project indicates a preferred alternate way or says this shouldn't be done).

@guillaume-be
Copy link
Contributor Author

Hello @zheng-da @classicsong -
could you please let me know if there is anything further that needs to be addressed?

@guillaume-be
Copy link
Contributor Author

Fixes #267, #256,

@@ -34,10 +34,10 @@ conda activate ${DGLBACKEND}-ci
# test
if [ "$2" == "cpu" ]; then
pip3 uninstall dgl
pip3 install dgl==0.4.3post2
pip3 install dgl
Copy link
Contributor

Choose a reason for hiding this comment

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

why CPU version doesn't define the version number, but GPU version does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this @zheng-da . My mistake - I added the version requirements to the wrong line - it should be correct now

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.

4 participants