Update Pregel files to match Python implementation: `debug.ts`, `io.ts`, `read.ts`, `types.ts`, `write.ts` by andrewnguonly · Pull Request #120 · langchain-ai/langgraphjs

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:30:33Z

Summary

The following files are updated (as much as possible) to match the corresponding Python implementation: debug.ts, io.ts, read.ts, types.ts, write.ts. The goal is to update the files without breaking existing tests or adding new functionality that would require significant changes to the Pregel class.

The implementation does not match exactly 1:1 with the Python implementation. I'll leave comments directly in the PR to note any differences.

Implementation

debug.ts: Small updates.
io.ts: Moved readChannel() function to this file. Added the following unused functions readChannels(), mapOutputValues(), and mapOutputUpdates().
read.ts: Renamed ChannelInvoke to PregelNode. Added more fields to PregelNode. Remove PregelNode.when. Update PregelNode.pipe() to support checking ChannelWrite.
types.ts: New file created to match Python implementation.
write.ts: Add ChannelWrite.isWriter() and ChannelWrite.registerWriter().
Add RunnableCallable class as part of fixing bug in Fix bug where saving a runnable or function in state object would misbehave langgraph#345. I haven't migrated all instances of RunnableLambda to RunnableCallable yet. Will do this in a subsequent PR.

To Do

Clean up the code. Improve readability.
Add unit tests for new io.ts. functions.
Add this bug fix from this PR: Fix bug where saving a runnable or function in state object would misbehave langgraph#345

Next Steps

Update pregel/index.ts (Pregel and Channel classes).
Migrate RunnableLambda to RunnableCallable.
Update pregel/validate.ts (I think).

…arguments.

… string[].

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:43:53Z

langgraph/src/pregel/types.ts

+  input: any;
+  proc: Runnable;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  writes: Array<[string, any]>; // TODO: Array type may need to be changed


The Python implementation uses collections.deque and the implementation of Pregel depends on the deque APIs. I need to figure out the appropriate type later.

we use deque in python for thread safety. in js fine to use array

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:47:45Z

langgraph/src/pregel/types.ts

+  /**
+   * Nodes to execute in the next step, if any
+   */
+  next: [string];


Double check: This needs to be Array<string>.

yes current type is wrong

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:54:51Z

langgraph/src/pregel/write.ts

        default: null,
        annotation: "TYPE_SEND",
-        isShared: true,
+        isShared: false,


The default value is false in the Python implementation.

LangGraph: https://github.com/langchain-ai/langgraph/blob/main/langgraph/pregel/write.py#L55

LangChain: https://github.com/langchain-ai/langchain/blob/master/libs/core/langchain_core/runnables/utils.py#L419

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:56:21Z

langgraph/src/pregel/write.ts

+  }
+
+  static registerWriter(runnable: Runnable): ChannelWrite {
+    return runnable as ChannelWrite;


Double check: Not used at the moment, but need to double check that this is the desired implementation.

It's not, but you can fix it later

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:56:47Z

langgraph/src/pregel/write.ts

+
+  static isWriter(runnable: RunnableLike): boolean {
+    // eslint-disable-next-line no-instanceof/no-instanceof
+    return runnable instanceof ChannelWrite;


Double check: Currently used in PregelNode.pipe(). Need to double check that this is the desired implementation.

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:58:21Z

langgraph/src/pregel/index.ts

  }
 }

-function _readChannel(


This is moved to io.ts.

Andrew Nguonly (andrewnguonly) · 2024-04-24T23:59:00Z

langgraph/src/pregel/index.ts

-        if (proc.when === undefined || proc.when(val)) {
-          tasks.push([proc, val, name]);
-        }
+        tasks.push([proc, val, name]);


I don't know if this is valid, but all of the existing tests are passing.

why would it not be valid?

I was just comparing with the current Python implementation which relies on the for_execution API, which isn't implemented yet in JS.

https://github.com/langchain-ai/langgraph/blob/main/langgraph/pregel/__init__.py#L1287

Andrew Nguonly (andrewnguonly) · 2024-04-25T00:02:38Z

langgraph/src/pregel/read.ts

-  lc_graph_name = "ChannelInvoke";
+  lc_graph_name = "PregelNode";

  channels: Record<string, string> | string;


channels is supposed to be type Record<string, string> | string[], but I couldn't make this change without breaking a significant amount of tests. I'll make this update when I update the Pregel class.

Andrew Nguonly (andrewnguonly) · 2024-04-25T00:04:38Z

langgraph/src/pregel/read.ts

+  writers?: Runnable[];
  tags?: string[];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  bound?: Runnable<any, any>;


Should this be Runnable<RunInput, RunOutput>?

Andrew Nguonly (andrewnguonly) · 2024-04-25T00:06:21Z

langgraph/src/pregel/read.ts

-  when?: (args: any) => boolean;
-  config?: RunnableConfig;
+  mapper?: (args: any) => any;
+  writers?: Runnable[];


Should this be Runnable<RunInput, RunOutput>[]?

This one should be Runnable<RunOutput, RunOutput>[]

Andrew Nguonly (andrewnguonly) · 2024-04-25T00:07:10Z

langgraph/src/pregel/read.ts

  triggers: string[] = [];

  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  when?: (args: any) => boolean;


Removed all instances and usages of when. I think this was a deprecated feature?

Yep this not used anymore

Andrew Nguonly (andrewnguonly) · 2024-04-25T00:09:58Z

langgraph/src/pregel/read.ts

+        triggers: this.triggers,
+        mapper: this.mapper,
+        writers: [...this.writers, coerceable as ChannelWrite],
+        bound: this.bound.pipe(coerceable),


This diverges from the Python implementation. I couldn't get the existing tests to pass without piping coerceable. Any advice?

Python implementation: https://github.com/langchain-ai/langgraph/blob/main/langgraph/pregel/read.py#L201

That's because you need other changes in Pregel class I think

Is it ok to leave this change in for now?

yes, but make a note to change this

…hannels().

Andrew Nguonly (andrewnguonly) · 2024-04-25T01:01:30Z

langgraph/src/pregel/io.ts

 ): Generator<[string, any]> {
-  if (typeof inputChannels === "string") {
-    yield [inputChannels, chunk];
+  if (chunk) {


This check was added in the Python implementation: https://github.com/andrewnguonly/langgraph/blob/main/langgraph/pregel/io.py#L52

Nuno Campos (nfcampos) · 2024-04-25T22:51:17Z

langgraph/src/pregel/write.ts

      .join(",")}>`;
    super({
-      ...{ channels, name },
+      ...{ channels: writes, name },


this should bewrites in super call too?

Unexpected change by VSCode when applying the Rename Symbol action. I changed to ...{ writes, name } and everything seems to work the same.

Nuno Campos (nfcampos)

mostly lgtm, some minor comments

Andrew Nguonly (andrewnguonly) · 2024-04-25T23:31:27Z

langgraph/src/pregel/write.ts

-
-    const valuesAwaited = await Promise.all(values);
-
+  async _getWriteValues(


Nuno Campos (@nfcampos), the implementation of _getWriteValues() (and _write()) matches the Python implementation as specified in langchain-ai/langgraph#345. However, I only made the bare minimum changes in Pregel and StateGraph to make the existing tests pass.

I'll add a new test to match this one: https://github.com/langchain-ai/langgraph/pull/345/files#diff-9f99beb90b66284c77ac257b4c0dfc06dcb998302f7c6740d84759460f704f53

Andrew Nguonly (andrewnguonly) · 2024-04-26T07:20:40Z

langgraph/src/tests/pregel.test.ts

  });
+
+  // TODO: fix this test
+  xit("can invoke a nested graph", async () => {


Nuno Campos (@nfcampos), I copied this test, but I haven't figured out where the fix needs to be in the existing implementation (which will probably be changed later). Ideally, I'd like to get this fixed/working before merging. I'll ask you for help in the morning.

…of Runnable if it is defined in graph state.

…hat exception is raised in method call.

… in generator.

Andrew Nguonly (andrewnguonly) added 11 commits April 22, 2024 14:33

Implement Pregel types and update debug print methods.

80c2e9f

Merge branch 'main' into migrate-pregel

4aa4d54

Rename ChannelInvoke to PregelNode. Remove field. Update constructor …

34b5251

…arguments.

Change type of PregelNodes.channels back to support string instead of…

61fe2dd

… string[].

Update ChannelRead class.

a53ceed

Update ChannelWrite.

c25077a

Update PregelNode pipe() function.

6dea09e

Update mapInput() and readChannels() in io.ts.

229b40e

Add mapOutputValues() and mapOutputUpdates() to io.ts.

8532bf8

Fix lint error.

4381301

Remove when parameter from Channel.subscribeTo() function.

2ed1110

Andrew Nguonly (andrewnguonly) commented Apr 24, 2024

View reviewed changes

langgraph/src/pregel/index.ts

}

}

function _readChannel(

Copy link
Copy Markdown

Contributor Author

Andrew Nguonly (andrewnguonly) Apr 24, 2024

Choose a reason for hiding this comment

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

This is moved to io.ts.

Andrew Nguonly (andrewnguonly) commented Apr 24, 2024

View reviewed changes

Andrew Nguonly (andrewnguonly) commented Apr 25, 2024

View reviewed changes

Andrew Nguonly (andrewnguonly) requested a review from Nuno Campos (nfcampos) April 25, 2024 00:10

Fix bug in readChannels(). Add unit tests for readChannel() and readC…

915e233

…hannels().

Andrew Nguonly (andrewnguonly) commented Apr 25, 2024

View reviewed changes

Add unit tests for mapInput(), mapOutputValues(), mapOutputUpdates().

fb74f00

Nuno Campos (nfcampos) reviewed Apr 25, 2024

View reviewed changes

Nuno Campos (nfcampos) previously approved these changes Apr 25, 2024

View reviewed changes

Update _write() to match implementation in langchain-ai/langgraph#345.

8736ce5

Andrew Nguonly (andrewnguonly) dismissed Nuno Campos (nfcampos)’s stale review via 8736ce5 April 25, 2024 23:22

Fix lint errors.

e594c1d

Andrew Nguonly (andrewnguonly) commented Apr 25, 2024

View reviewed changes

Andrew Nguonly (andrewnguonly) added 2 commits April 25, 2024 23:37

Add (failing) unit test for invoking nested graph.

3b99228

Fix minor typing issues.

5fbf72e

Andrew Nguonly (andrewnguonly) commented Apr 26, 2024

View reviewed changes

Andrew Nguonly (andrewnguonly) requested a review from Nuno Campos (nfcampos) April 26, 2024 07:20

Andrew Nguonly (andrewnguonly) added 3 commits April 26, 2024 12:52

Fix skipped unit test. Implement RunnableCallable to skip invocation …

ba84592

…of Runnable if it is defined in graph state.

Fix unit test: Throw error immediately after calling method to test t…

f64ef02

…hat exception is raised in method call.

Fix unit test that wasn't implemented correctly to catch error thrown…

57b2cce

… in generator.

Nuno Campos (nfcampos) approved these changes Apr 30, 2024

View reviewed changes

Nuno Campos (nfcampos) merged commit 099fafa into langchain-ai:main Apr 30, 2024

Andrew Nguonly (andrewnguonly) deleted the migrate-pregel branch April 30, 2024 20:21


		const valuesAwaited = await Promise.all(values);

		async _getWriteValues(

Conversation

Andrew Nguonly (andrewnguonly) commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation

To Do

Next Steps

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!

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!

Nuno Campos (nfcampos) Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nuno Campos (nfcampos) 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Andrew Nguonly (andrewnguonly) commented Apr 24, 2024 •

edited

Loading

Nuno Campos (nfcampos) Apr 25, 2024 •

edited

Loading