feat(graph): add Spanner Graph support to LangChain GraphStore interface by mtyin · Pull Request #104 · googleapis/langchain-google-spanner-python

mtyin · 2024-10-22T20:20:15Z

Add SpannerGraphStore implementation of GraphStore;
Add an integration test;
Add a notebook to demonstrate the usage;
Misc updates: requirements, init

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

1) Add SpannerGraphStore implementation of GraphStore; 2) Add an integration test; 3) Add a notebook to demonstrate the usage; 4) Misc updates: requirements, __init__

1) same edge label between different types of nodes E.g. Company FOCUS_ON Product Company FOCUS_ON Technology 2) the same doc contains multiple nodes/edges with the same key fields. E.g. Merge Company(id='Google', properties={'p1':'a1', 'p3':'a3'}), Company(id='Google', properties={'p2':'a2', 'p3':'a4'}) into Company(id='Google', properties={'p1':'a1', 'p2':'a2', 'p3':'a4'}) This is due to Spanner DML doesn't support updating the same row twice in the same DML. We could also separate each node/edge into a separate DML which can be very slow.

gauravpurohit06

Thank you mtyin for raising the PR. The code looks well organized and modular.

I have reviewed the source code file and not test file. From the functionality point of view, it looks good but I need to do one more iteration.

Other than mentioned comments, few general things:

You can use isort and black to format and style the code.
Null Checks are missing at various places, can you please add it?

gauravpurohit06 · 2024-11-11T10:39:59Z

@@ -0,0 +1,903 @@
+{


nit: Styling of this file is super bright (ie. painful white background) and inconsistent with other docs. Please make it consistent with other docs.

I made some changes but from what I can see, the other notebooks are also using brightwhite as background color. LMK if I missed anything.

gauravpurohit06 · 2024-11-11T11:28:45Z

+            w = NodeWrapper(node)
+            if w in s:
+                # Combine the properties for nodes with the same id.
+                n = next(filter(lambda v: v == w, s))


This operation potentially be slow for big graphs, because it's iterating over a complete set which is not required as the NodeWrapper object is hashable & comparable based on their ids.

gauravpurohit06 · 2024-11-11T11:29:11Z

+            w = NodeWrapper(node)
+            if w in s:
+                # Combine the properties for nodes with the same id.
+                n = next(filter(lambda v: v == w, s))


if w.node.id in s: # Combine the properties for nodes with the same id. s[w.node.id].node.properties.update(node.properties)

Btw, I used the wrapper as the key to make the code looks more consistent between node and edge.

gauravpurohit06 · 2024-11-11T11:29:29Z

+            w = EdgeWrapper(edge)
+            if w in s:
+                # Combine the properties for edges with the same id.
+                e = next(filter(lambda v: v == w, s))


same as above.

gauravpurohit06 · 2024-11-11T11:29:52Z

+    }
+
+
+class TypeUtility(object):


Add class docs.

Done! Also moved the class to a separate file

gauravpurohit06 · 2024-11-11T11:32:09Z

+        """
+        if s == "BOOL":
+            return param_types.BOOL
+        if s in ["INT64", "INT32"]:


why would the schema have INT32 ?

Good catch!

gauravpurohit06 · 2024-11-11T11:34:15Z

+    }
+
+
+class TypeUtility(object):


This is very generic class and not limited just to GraphStore. Can we create a utility module and move it there? It would be useful for future development in this repo.

gauravpurohit06 · 2024-11-11T11:37:53Z

+from requests.structures import CaseInsensitiveDict
+
+
+def to_identifier(s: str) -> str:


Can we move this method in GraphDocumentUtility? as we are keeping fixup_identifier method there as well, both methods looks similar to me.

gauravpurohit06 · 2024-11-11T11:37:59Z

+    return "`" + s + "`"
+
+
+def to_identifiers(s: List[str]) -> str:


same as above.

1) Fix style of python notebook; 2) GraphStore: - improve a corner case by avoiding full set iteration; - add some null checking; - move utilities around.

mtyin · 2024-11-15T17:20:12Z

/gcbrun

averikitsch · 2024-11-19T23:15:31Z

/gcbrun

averikitsch · 2024-11-19T23:40:39Z

/gcbrun

mtyin · 2024-11-20T03:12:19Z

/gcbrun

mtyin · 2024-11-20T18:36:22Z

/gcbrun

mtyin · 2024-11-20T18:38:06Z

/gcbrun

averikitsch · 2024-11-22T20:27:41Z

 langchain-core==0.3.9
 langchain-community==0.3.1
 google-cloud-spanner==3.49.1
+langchain-experimental==0.3.2


Since this dep is only needed for the colab, please remove from here.

averikitsch · 2024-11-22T20:28:53Z

+    "colab_type": "text"
+   },
+   "source": [
+    "<a href=\"https://colab.research.google.com/gist/mtyin/3f1e7d8e4c6b59edc6e7858c52247465/copy-of-graph_store.ipynb\" target=\"_parent\"><img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/></a>"


please remove duplicate colab button

Good catch! Done.

mtyin · 2024-11-22T22:07:05Z

Thank you mtyin for raising the PR. The code looks well organized and modular.

I have reviewed the source code file and not test file. From the functionality point of view, it looks good but I need to do one more iteration.

Other than mentioned comments, few general things:

You can use isort and black to format and style the code.

Null Checks are missing at various places, can you please add it?

Resolve this.

gauravpurohit06

LGTM

feat(graph): add Spanner Graph support to LangChain GraphStore interface

943462d

1) Add SpannerGraphStore implementation of GraphStore; 2) Add an integration test; 3) Add a notebook to demonstrate the usage; 4) Misc updates: requirements, __init__

product-auto-label Bot added the api: spanner Issues related to the googleapis/langchain-google-spanner-python API. label Oct 22, 2024

gauravpurohit06 marked this pull request as ready for review November 11, 2024 11:13

gauravpurohit06 requested review from a team November 11, 2024 11:13

blunderbuss-gcf Bot assigned rahul2393 Nov 11, 2024

gauravpurohit06 and others added 3 commits November 11, 2024 16:43

Merge branch 'main' into graph

2374d66

style: formatting

9ae8467

style: formatting

0a941bb

gauravpurohit06 suggested changes Nov 11, 2024

View reviewed changes

mtyin added 2 commits November 11, 2024 17:34

Address various review feedbacks

7548c65

1) Fix style of python notebook; 2) GraphStore: - improve a corner case by avoiding full set iteration; - add some null checking; - move utilities around.

Format the notebook

9b44836

Fix linter error

fc49cf3

Fix type annotation

aa009e1

Use batch table insertion API instead of transaction DML API

d160b7f

averikitsch approved these changes Nov 22, 2024

View reviewed changes

Minor fixes & cleanups: colab, requirements, tests, schema corner cases

60a0af7

gauravpurohit06 approved these changes Nov 25, 2024

View reviewed changes

averikitsch merged commit 98c2f8f into googleapis:main Nov 25, 2024

release-please Bot mentioned this pull request Nov 25, 2024

chore(main): release 0.5.0 #109

Merged

		from requests.structures import CaseInsensitiveDict


		def to_identifier(s: str) -> str:

		return "`" + s + "`"


		def to_identifiers(s: List[str]) -> str:

Conversation

mtyin commented Oct 22, 2024

Uh oh!

gauravpurohit06 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtyin commented Nov 15, 2024

Uh oh!

averikitsch commented Nov 19, 2024

Uh oh!

averikitsch commented Nov 19, 2024

Uh oh!

mtyin commented Nov 20, 2024

Uh oh!

mtyin commented Nov 20, 2024

Uh oh!

mtyin commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtyin commented Nov 22, 2024

Uh oh!

gauravpurohit06 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants