-
Notifications
You must be signed in to change notification settings - Fork 11
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: prepare architecture for parallelism and add helpers #111
Conversation
…e're sharing for example paths and file names in it - not only kind of background info
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.
Generally this is great. Some minor changes requested.
Plus, maybe parallelism should be explicitly enabled/disabled because I can imagine some kinds of workflows that cannot be processed in parallel - I mean this kind of scenario where there are a few steps to be executed, but the next steps depend on the previous result. In this case, there's a significant risk that the supervisor won't get these details and split the tasks. There should be a way to prevent this from happening. Another thing is to have a configurable number of parallel tasks (children) at hand. |
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.
Helpers are great. Parallelism probably need to be more harnessed with max-concurrency etc. But in general this is cool
Should parallelism be enabled by default? Or shall we consider it advanced feature? |
…ve object (#105) Fixes #68 Related #102 There are a lot of changes in this PR, I will break them down here, together with motivation. The prime motivation for the change was to simplify the structure of "state" by getting rid of agent specific properties (such as agentName, agentRequest) and moving to a simple state object that can have "child" states (either single element, or an array). That way, we could get rid of special handling for supervisor, resource planner (two built-in agents) and further simplify the logic. Thanks to that, we have "out-of-the-box" support for parallelism, cancellations, hand-offs. If agent wants to delegate, simply create new `workflowState` and add it as a child, then return state (see how "supervisor" is implemented). If agent wants to hand-off the task, they can simply replace their entire state (see how "resourcePlanner" gets its job done). Other changes worth mentioning: - Agent does not have role anymore. Now, what matters is the key on "team" object. This is aligned with how we define tools. This will make it better and more future proof if we serialize state object on the server. We no longer rely on array positions. - Renamed "members" array to "team" object - Supervisor and Planner are now normal agents (and you can overwrite them, or change their behavior, or define your own) - Added tool helpers to be used on the server side Here is screenshot of what the output looks like at the moment: ![3EB0FB32C3162E9A2F78_1](https://github.com/user-attachments/assets/b2e3b7b4-afea-436e-b6b2-73e6f7407c13) --------- Co-authored-by: Piotr Karwatka <[email protected]>
… + dividing it to more self-explanatory agent roles for making resourcePlanner life easier
We separate knowledge from workflow description to avoid tasks/descriptions interfering with agent. This could possibly be something else in the future, like a method that gets it from the database/something. This is just demonstration, as alternative to #105. I am still not sure about this.
export type Message = ChatCompletionMessageParam | ||
export type Conversation = [Request, ...Message[]] | ||
|
||
export const getSteps = (conversation: Message[]): Message[] => { |
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.
@pkarw should we leave this function as returning an array of steps, or should we change its signature to return string, and construct this message here? I am leaning towards the latter!
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 also think Steps are more like a single message - because so far, messages somehow mimic the interactions between different actors, and the list of steps is just one side story (?)
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.
Will send a follow-up converting it into a message
@grabbou parallelism should be disabled for now - until we stabilize the Parallelism is clearly |
Yup, there's no parallel support at this point, so we're good here! |
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.
All good. All examples pass.
This PR enables architecture to handle multiple children, effectively enabling parallelism. It also adds few state helpers. In a follow-up PR, I will send a draft of parallel supervisor - works OK, but needs testing! --------- Co-authored-by: Piotr Karwatka <[email protected]>
This PR enables architecture to handle multiple children, effectively enabling parallelism. It also adds few state helpers.
In a follow-up PR, I will send a draft of parallel supervisor - works OK, but needs testing!