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

remove py2neo as primary database driver #235

Merged
merged 21 commits into from
Feb 22, 2024
Merged

remove py2neo as primary database driver #235

merged 21 commits into from
Feb 22, 2024

Conversation

ianmkenney
Copy link
Collaborator

@ianmkenney ianmkenney commented Feb 13, 2024

We currently use the EOL py2neo library for handling communication between alchemiscale and the Neo4j database. This PR removes this responsibility from py2neo and instead shifts it to the official Neo4j Python driver. This PR does not remove py2neo entirely since we rely heavily on the py2neo Subgraph data structure and query generation.

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2024

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (28140bf) 81.65% compared to head (c8d3142) 81.66%.

Files Patch % Lines
alchemiscale/storage/subgraph.py 82.27% 14 Missing ⚠️
alchemiscale/storage/statestore.py 92.59% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   81.65%   81.66%   +0.01%     
==========================================
  Files          24       25       +1     
  Lines        2943     3049     +106     
==========================================
+ Hits         2403     2490      +87     
- Misses        540      559      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ianmkenney ianmkenney mentioned this pull request Feb 19, 2024
@dotsdl dotsdl marked this pull request as ready for review February 20, 2024 16:54
@dotsdl dotsdl self-requested a review February 21, 2024 05:26
@ianmkenney ianmkenney mentioned this pull request Feb 21, 2024
Copy link
Member

@dotsdl dotsdl left a comment

Choose a reason for hiding this comment

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

Amazing work @ianmkenney! Merge if you're satisfied!

graph = Graph(
settings.NEO4J_URL,
auth=(settings.NEO4J_USER, settings.NEO4J_PASS),
name=settings.NEO4J_DBNAME,
Copy link
Member

Choose a reason for hiding this comment

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

Let's also drop NEO4J_DBNAME from the settings, since we don't actually use it anywhere, such as in our execute_query calls.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, added it back in a commit I've pushed, but in transaction instead. Also added Neo4jStore.execute_query method that includes use of database_ kwarg to ensure this gets passed through, and adjusted all uses of self.graph.execute_query to use this instead.

def __init__(self, graph: "py2neo.Graph"):
self.graph: Graph = graph
def __init__(self, graph: Driver):
self.graph: Driver = graph
self.gufe_nodes = weakref.WeakValueDictionary()

@contextmanager
def transaction(self, readonly=False, ignore_exceptions=False) -> Transaction:
Copy link
Member

Choose a reason for hiding this comment

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

Removed readonly kwarg, since this doesn't exist in new driver.

alchemiscale/storage/statestore.py Outdated Show resolved Hide resolved
alchemiscale/storage/statestore.py Outdated Show resolved Hide resolved
@ianmkenney ianmkenney merged commit faedfe5 into main Feb 22, 2024
4 checks passed
@dotsdl dotsdl deleted the 144-remove-py2neo branch February 22, 2024 17:38
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