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.

Sign up for GitHub

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

Jump to bottom

new: adbdgl refactor #30

Merged
merged 37 commits into from
Oct 26, 2023
Merged

new: adbdgl refactor #30

merged 37 commits into from
Oct 26, 2023

Conversation

aMahanna
Copy link
Member

@aMahanna aMahanna commented Aug 5, 2022 •
edited
Loading

Warrants a 3.0.0 release (the large diff is mostly due to the notebook updates)

See the new README

Closes #29

Note: For notebook testing purposes:

  • Use !pip install git+https://github.com/arangoml/dgl-adapter.git@feature/adapter-refactor
  • Use !git clone -b feature/adapter-refactor --single-branch https://github.com/arangoml/dgl-adapter.git

Open In Collab

What's new

  • Repo-wide refactor to reflect the behaviour of the ArangoDB-PyG adapter (up to Code Cleanup pyg-adapter#23) at the exception of:
    • The preserve_adb_keys logic (DGL does not support lists of strings as a property)
    • Ambiguity check for homogeneous graphs (DGL stores its node & edge data separately, even if the graph is homogeneous)
    • The strict parameter of Arangodb to PyG (for now)
  • Official support for python 3.10 and 3.11

@aMahanna aMahanna self-assigned this Aug 5, 2022
joerg84
joerg84 reviewed Oct 17, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
joerg84
joerg84 previously approved these changes Oct 17, 2022
View reviewed changes
Copy link

@joerg84 joerg84 left a comment

Choose a reason for hiding this comment

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

Overall lgtm.
a) would this PR require a new major version release given backwards compatibility? Do we need a migration guide?
b) what are the resulting doc changes? Do we need to sync with the doc team?

geenen124
geenen124 reviewed Oct 18, 2022
View reviewed changes
adbdgl_adapter/abc.py Outdated Show resolved Hide resolved
@aMahanna
Copy link
Member Author

aMahanna commented Oct 18, 2022 •
edited
Loading

Overall lgtm. a) would this PR require a new major version release given backwards compatibility? Do we need a migration guide? b) what are the resulting doc changes? Do we need to sync with the doc team?

a) No need for a migration guide since the API convention/usability remains the same, but this will warrant a major version release (from 2.1.0 to 3.0.0)

b) Right, the doc team would have to replace the contents of https://www.arangodb.com/docs/stable/data-science-dgl-adapter.html#quickstart with https://github.com/arangoml/dgl-adapter/tree/feature/adapter-refactor#quickstart

comments have been addressed via a40896b

@aMahanna aMahanna requested review from geenen124 and joerg84 October 19, 2022 21:11
@aMahanna
Copy link
Member Author

@geenen124 @joerg84 any other thoughts on moving forward with this PR?

aMahanna added 2 commits July 20, 2023 23:53
@aMahanna
revive PR
7070d18 
improvements based on pyg-adapter, general code cleanup
@aMahanna
swap python 3.7 support for 3.11
bca8d5a 
3.7 has reached eol
aMahanna
aMahanna commented Oct 5, 2023
View reviewed changes
setup.py Outdated Show resolved Hide resolved
geenen124
geenen124 suggested changes Oct 10, 2023
View reviewed changes
Copy link

@geenen124 geenen124 left a comment

Choose a reason for hiding this comment

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

It looks like the colab link is out of date w.r.t. the notebook on this branch?

setup.py Outdated Show resolved Hide resolved
examples/ArangoDB_DGL_Adapter.ipynb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@aMahanna aMahanna requested a review from geenen124 October 11, 2023 14:54
geenen124
geenen124 previously approved these changes Oct 11, 2023
View reviewed changes
@aMahanna aMahanna mentioned this pull request Oct 15, 2023
Merged
@aMahanna
DGL Refactor Updates (#31)
2178ac0 
* initial commit

* bump

* cleanup workflows

* Update build.yml

* Update build.yml

* parameter renaming, new `use_async` param

* update release action
@aMahanna aMahanna requested a review from geenen124 October 26, 2023 12:12
@aMahanna
Copy link
Member Author

re-requesting review after having merged #31

geenen124
geenen124 previously approved these changes Oct 26, 2023
View reviewed changes
@aMahanna aMahanna requested a review from geenen124 October 26, 2023 14:11
@aMahanna
Copy link
Member Author

Sorry @geenen124 had to make another readme update, one last review when you get the chance

@aMahanna aMahanna merged commit 0f40b1d into master Oct 26, 2023
6 checks passed
@aMahanna aMahanna deleted the feature/adapter-refactor branch October 26, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geenen124 geenen124 geenen124 approved these changes

@joerg84 joerg84 Awaiting requested review from joerg84

Assignees

@aMahanna aMahanna

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor adbdgl-adapter to mirror adbpyg-adapter
3 participants
@aMahanna @geenen124 @joerg84