aplbrain / grand

Your favorite Python graph libraries, scalable and interoperable. Graph databases in memory, and familiar graph APIs for cloud databases.
Apache License 2.0
80 stars 6 forks source link

Update SQL backend to support SQLAlchemy 2.x #45

Closed davidmezzetti closed 4 months ago

davidmezzetti commented 4 months ago

Hi @j6k4m8

Once again thank you for this compelling series of graph-related libraries!

I am working to use grand as a NetworkX compatible backend with Postgres. The main issue I ran into is compatibility with SQLAlchemy 2.x. This PR adds support for that.

This PR also adds a few performance improvements.

A couple other performance improvements were possible using what's already provided in the library (i.e. caching). So overall, with these changes, I'm add to get solid performance with large graphs stored in Postgres.

j6k4m8 commented 4 months ago

https://github.com/aplbrain/grand/actions/runs/8649244221/job/23717215080?pr=45#step:6:22

@davidmezzetti Some failing tests — looks like it's a matter of deduplicating edges?

davidmezzetti commented 4 months ago

Let me take a look. I didn't see that tests were available. I'll reproduce locally and push a new commit with the fix.

j6k4m8 commented 4 months ago

Awesome, let me know if you run into any trouble! SHOULD be as simple as running pytest (and it should know to run sql queries in-memory during testing, etc)

davidmezzetti commented 4 months ago

Ok, I just checked in a change that fixes the unit tests. One of the errors didn't properly convert the logic to SQLAlchemy 2.x.

The other one is related to the new edge source and target indexes. Previously, when that index wasn't there, the unit tests used the default sort order of the table when pulling edges, which typically uses the primary key. An order_by clause was added to sort by the primary key.

Alternatively, the bfs_successors calls could have a sort_neighbors argument applied in the tests to guarantee a consistent ordering. This might be a better way to ensure the test always works regardless of the edge ordering.

davidmezzetti commented 4 months ago

Looks like this failure is due to not being able to build networkit for 3.7. Perhaps 3.7 should be removed given it's long been EOL?

j6k4m8 commented 4 months ago

My thoughts exactly — want to remove 3.7 and add 3.11?

On Thu, Apr 11, 2024 at 3:37 PM David Mezzetti @.***> wrote:

Looks like this failure is due to not being able to build networkit for 3.7. Perhaps 3.7 should be removed given it's long been EOL?

— Reply to this email directly, view it on GitHub https://github.com/aplbrain/grand/pull/45#issuecomment-2050388455, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFJKBZOJRP2WYOXUMZ5QHTY43RBBAVCNFSM6AAAAABGCQVQUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJQGM4DQNBVGU . You are receiving this because you were mentioned.Message ID: @.***>

davidmezzetti commented 4 months ago

Just made that change to the build script.

j6k4m8 commented 4 months ago

Whew — thank you!! Looking good, good to merge once these pass?

davidmezzetti commented 4 months ago

Good from my end.

I'm putting together an example project for txtai that shows how to store all components in Postgres. This will help with the Graph piece and it's really cool how this integrates with the previous work with openCypher!

davidmezzetti commented 4 months ago

Hi @j6k4m8 - I wanted to check and see if there were plans to push a new release anytime soon. I'm leaning towards integrating the database graph component as a core feature but would need this change to add the dependency.

j6k4m8 commented 4 months ago

~Sorry, have been traveling — will roll it today!~

[edit] @davidmezzetti I just pushed it, thanks for the bump!

davidmezzetti commented 4 months ago

Great, thank you!