feat(aws): Enable Buffering and Flushing Writes to Checkpoint Savers#892
feat(aws): Enable Buffering and Flushing Writes to Checkpoint Savers#892Farouk Boukil (f4roukb) wants to merge 12 commits intolangchain-ai:mainfrom
Conversation
f34da40 to
18165e8
Compare
|
Michael Chin (@michaelnchin), since you addressed the corresponding issue, could you please have a look? Happy to address your comments. Thank you. |
Michael Chin (michaelnchin)
left a comment
There was a problem hiding this comment.
Thank you for the submission Farouk Boukil (@f4roukb) !
Added some feedback below. Please also add additional unit/integration tests to validate these edge cases where needed.
libs/langgraph-checkpoint-aws/langgraph_checkpoint_aws/buffered_saver.py
Outdated
Show resolved
Hide resolved
libs/langgraph-checkpoint-aws/langgraph_checkpoint_aws/buffered_saver.py
Outdated
Show resolved
Hide resolved
| try: | ||
| yield self | ||
| finally: | ||
| self.flush() |
There was a problem hiding this comment.
Even if there's no danger to flushing the existing buffer on exceptions, we should still log them for visibility given that the resumed checkpoint may not behave as expected due to the ignored failure.
There was a problem hiding this comment.
I added some log messages there. Let me know what you think.
libs/langgraph-checkpoint-aws/langgraph_checkpoint_aws/buffered_saver.py
Outdated
Show resolved
Hide resolved
libs/langgraph-checkpoint-aws/langgraph_checkpoint_aws/buffered_saver.py
Outdated
Show resolved
Hide resolved
libs/langgraph-checkpoint-aws/langgraph_checkpoint_aws/buffered_saver.py
Outdated
Show resolved
Hide resolved
|
I missed quite a few things working on this one, huh 😅 Thanks a lot for the spot-on observations. I addressed your comments and will be working on additional unit/integration tests. Should be re-reviewable by EOW. |
9b4cd7d to
2e4486e
Compare
|
I think it's a good milestone to get some more feedback please. I updated the PR description with more details. I can add component tests for the Please note that I don't have access to the necessary infrastructure to actually run and debug the component tests. I'd really appreciate it if you could help push this. Thank you. |
Issue: #806
Description: Enable buffering the write ops of any checkpoint saver and flushing them manually where needed (e.g., inside the graph execution), and automatically at the end of graph execution.
Changes:
BufferedCheckpointSaverclass (decorator pattern with both sync and async API). It buffers writes and flushes them:a) After the context is exit when a context manager is used:
aflush_on_exit(async) orflush_on_exit(sync).b) When the
aflush(async) orflush(sync) method is called (potentially inside the graph).MemorySaver, including tests for edge cases.I essentially picked the existing component tests back up and adapted them by wrapping the instances in the buffering decorator and adapting the assertions.
I also added more fundamental component tests in each of these test suites to ensure behavior is tested on different kinds of graphs, with streaming, with parallelism, with error mid-execution, and with flush mid-execution.