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

LangGraph throws an error when adding edges that don't exist, but that makes writing the code a bit messy #88

Open
davidfant opened this issue Mar 19, 2024 · 5 comments

Comments

@davidfant
Copy link

davidfant commented Mar 19, 2024

Here LangGraph throws an error when trying to add an edge between nodes that don't exist:

if (!this.nodes[startKey]) {
throw new Error(`Need to addNode \`${startKey}\` first`);
}
if (!this.nodes[endKey] && endKey !== END) {
throw new Error(`Need to addNode \`${endKey}\` first`);
}

That makes sense but it makes my code that constructs the graph a bit confusing to read. This isn't the case for addConditionalEdge (unless providing a mapping).

Example for how I ideally want to set up the graph

agent.addNode("first", ...);
agent.addEdge("first", "second");

agent.addNode("second", ...);

Counter point: why can't I set up edges after all nodes. I can, but if a graph gets big, it's annoying and confusing to have to scroll a lot to figure out how everything is connected. I want to read the logic/paths from top to bottom, rather than just nodes and then just edges. Maybe this is just me and no one else having this problem.

What are your thoughts reg moving this validation to the compile step instead?

@jacoblee93
Copy link
Collaborator

@nfcampos to weigh in!

@nfcampos
Copy link
Contributor

@davidfant see improved typings added in #128 which would potentially make what you want harder? That said I'll be moving the runtime validation itself to compile() to allow you to build graphs "out of order" if you opt out of the static validation

@andrewnguonly
Copy link
Contributor

What are your thoughts reg moving this validation to the compile step instead?

@jacoblee93, what's your opinion?

I'm slightly against this, but I don't know why and I don't have a strong reason. Pushing business logic validation to a compile/static check does not seem desirable IMHO if it's at the expense of increased developer maintenance. The implementation may get more complex when supporting more complicated graph features (dynamic graphs).

I think a better solution would be to allow the developer to add nodes and edges in any order (i.e. just aggregate them in a list). Then build the graph internally in topological sorted order.

@hinthornw
Copy link
Contributor

It could be nice to have something where it creates the node and adds an edge at the same time (for simple edges), like node.to(other_node), however.

@jacoblee93
Copy link
Collaborator

jacoblee93 commented May 17, 2024

Hey sorry for missing the ping!

@andrewnguonly I don't feel super strongly but adjacency lists are a common way to declare graphs in general which naturally imply declaring edges before declaring all nodes:

1: 2, 3
2: 1
3: 1

It does increase the likelihood of something being misrouted if a name changes, but I lean towards @davidfant's comments here around convenience.

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

No branches or pull requests

5 participants