-
Notifications
You must be signed in to change notification settings - Fork 28
fix: refactor runtime execute #227
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
Conversation
b08df2f to
643531d
Compare
| yield self.context.memory | ||
| else: | ||
| # Create new memory and dispose at the end | ||
| async with AsyncSqliteSaver.from_conn_string( |
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.
This line closes the memory object once it goes out of scope so I wouldn't recommend caching it. It's better to just instantiate it during object construction
self.context.memory = AsyncSqliteSaver.from_conn_string(self.state_file_path)
Then, in the cleanup method of LangGraphRuntime, we will need to invoke self.context.memory.close() or something along these lines. This will also fix the parallel-evaluations issue that was reported last week.
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.
removed the caching. Yes, the idea is to use the memory externally provided, otherwise create a new local scope
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 see what you mean.
I was thinking of getting removing memory from UiPathRuntimeContext and moving it to the runtime object. IMO this will simplify how the cache objects are created. I currently don't see why the caller would invoke execute with different memory objects.
Since the cache is removed, the concern of maintaining the cache object is moved to the caller. If a called does not provide a cache object, this line will create new objects for every single execution. Caching the sqlite object is a good idea. However, IMO we should not be closing it unless the runtime exists (i.e; via cleanup).
We could change this later too and decouple it from this PR.
| message.pretty_print() | ||
| else: | ||
| # No messages, print node name and state | ||
| logger.info("%s", event.payload) |
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.
Nit: should this be logger.verbose?
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.
doesn't seem like a built-in method
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.
Sorry, I meant debug. Basically, my concern is that event may contain sensitive information, and should be only shown during dev time.
6a5d481 to
b70e9ab
Compare
b70e9ab to
015d49b
Compare
Description
Refactors the runtime execution flow to simplify input/output handling, the suspension/resume support, and streamline streaming execution, in preparation for debug and conversational agents.
Development Package