-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat #2: a step towards async exec + memory persistence #10
Conversation
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.
Thanks for the PR! Leaving some changes that I think would be better suited for our current scenario. It is still not defined, so I am leaving this open for discussion!
src/index.ts
Outdated
const task = await getNextTask(messages) | ||
if (!task) { | ||
return messages.at(-1)!.content as string | ||
} | ||
|
||
console.log('🚀 Next task:', task) | ||
if (execOptions.maxIterations && context.iteration >= execOptions.maxIterations) { |
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 would implement this the following:
- let workflow settings object take 'maxIterations' with a default value of 25
- update prompt to say you must produce final answer in maximum 25 steps
- if the tool fails to produce final answer in 25 steps, execute a follow-up prompt called "produce_final_answer" that will tell it that time is up and it must wrap it up.
I have seen this done in other agent frameworks too.
src/index.ts
Outdated
content: task, | ||
}) | ||
this.nextTask(task, workflow, context); | ||
this.context.iteration++; |
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: We don't have to count iterations, we can just use messages.length
as we push every iteration anyway.
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.
Fixed
src/index.ts
Outdated
}) | ||
} | ||
|
||
if (execOptions.async) // exec one step at time, tbd: save the context in a persistence layer; |
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: like discussed on the chat, I would recommend to have a subclass or something, or maybe entirely separate Team setup (called RemoteTeam 🤣) that would have this behavior, so we keep this one simple and work for stateful scenarios.
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.
Fixed
src/index.ts
Outdated
type WorkflowExecOptions = { | ||
maxIterations?: number | ||
async: boolean; | ||
} |
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: I would suggest merging this with Workflow
type to keep things simple
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.
Fixed
src/index.ts
Outdated
|
||
// tbd: expose this via async API | ||
async execute(workflow: Workflow, execOptions: WorkflowExecOptions = { async: false }): Promise<string> { | ||
this.context.messages = [ |
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: I am thinking that stateless API is slightly cleaner, so I would suggest we revert back to previous scenario where nextTask
was outside of the class. To be very honest, I would like this method to be outside of the class too.
Here's how I would do it:
execute() should take workflow as options, and second argument, context.
We should create context inside execute function, not on the class.
We should remove while loop and move to recursion instead.
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.
Fixed
LGTM |
This is just a draft PR - still WiP