-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
community[minor]: Implement addGraphDocuments method for neo4j graph #4761
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -1,19 +1,37 @@ | |||
/* eslint-disable no-process-env */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! 👋 This is a friendly flag to highlight that the recent change in the PR adds, accesses, and requires environment variables via process.env
. It's important for maintainers to review this change to ensure proper handling of environment variables. Keep up the great work! 🚀
@@ -1,4 +1,6 @@ | |||
import neo4j, { RoutingControl } from "neo4j-driver"; | |||
import { createHash } from "crypto"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is intended to work in web environments (e.g. Next.js) but I would suggest using this:
import { insecureHash } from "@langchain/core/utils/hash"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use insecureHash
} | ||
} | ||
|
||
export class GraphDocument extends Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this extend Document
instead? It will be a bit more interchangeable with other RAG pipeline things in the framework if we do it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated - please review
LGTM |
|
||
relationships: Relationship[]; | ||
|
||
source: Document; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnest this since we're now extending Document
directly? Otherwise there's no point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically implement pageContent
and metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would consisteny between python and JS be a higher priority? IMO, there is not much value in extending the doc for rag, as we never use the graph doc in retrieval setting, it's only used for ingestion atm. Even if we wanted to use it in retrieval, it would need custom components since none use nodes or rels atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fine by me.
Really sorry for the delay - see above. |
Thank you for your patience! |
Porting over add_graph_documents method from python.