-
Notifications
You must be signed in to change notification settings - Fork 112
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
Multi-action renderConversationForModel with cli #4963
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.
Why tool
everywhere? 😬
@@ -12,6 +12,53 @@ export type ModelConversationType = { | |||
messages: ModelMessageType[]; | |||
}; | |||
|
|||
export type ContentFragmentMessageTypeModel = { |
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.
We likely want to get rid of this type entirely and either render that as a fake agent message with function call + response function message or a user 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.
If we decide to have function call + reponse it could look like that:
{
"role": "assistant",
"content": null,
"functionCalls": [
{
"id": "81",
"type": "function",
"function": {
"name": "retrieve_content",
"arguments": "{\"content_type\":\"slack_thread_content\"}" // or file_attachment
}
}
]
},
{
"role": "tool",
"toolCallId": "81",
"content": "Content from the fragment"
}
Or if we decide it's a user message:
{
"role": "user",
"name": "get_slack_thread_content", // or get_file_attachment
"content": "Content from the fragment"
},
=> Since some provider are erroring when a convo starts with an agent message cf https://github.com/dust-tt/tasks/issues/699, I will add a commit to set it as a user message. Let me know if you think otherwise!
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.
Other providers won’t allow having two user messages in a row I believe 🙃
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.
Not sure there’s a great solution 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.
Task created to handle it from Core: #4967
Because I understood we wanted to align on the new name and specs from #4931 but I'll add a commit to change 👍🏻 |
Yes but through core :-) |
5d19dec
to
d9337af
Compare
content: m.content, | ||
}); | ||
} | ||
if (m.action) { |
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.
We'll need m.actions
but will create a separate task for that.
@lasryaric does your current work on inverting relationship include refactoring from message.action
into message.actions
?
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.
LGTM deferring to @spolu for final review !
let text = `${m.role} ${"name" in m ? m.name : ""} ${m.content ?? ""}`; | ||
if ("function_calls" in m) { | ||
text += m.function_calls | ||
.map((f) => `${f.function.name} ${f.function.arguments}`) |
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 be : search_data_sources {"query":"Soupinou cute","relativeTimeFrame":"all","topK":32}
export type AssistantFunctionCallMessageTypeModel = { | ||
role: "assistant"; | ||
content: string | null; | ||
function_calls: { |
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.
function_calls is an array
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.
my bad it is!
c5f6217
to
d1708e0
Compare
d1708e0
to
b4b0ce7
Compare
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.
Can we just rename for all actions:
renderFunctionMessageForForProcessAction => renderProcessActionFunctionCall
renderAssistantFunctionCallForProcessAction => renderProcessActionForMultiActionModel
…ven AgentMessageType
ac80d35
to
00d40f5
Compare
* Multi-action renderConversationForModel with cli * Replace tool by function * Snake case as expected by the model * I can't be git blamed on the fuck it comment :D * Back to function_calls & improve token count estimation * Get change from #4973 * Fix we want a single assistant message for all function calls of a given AgentMessageType * Better naming
Description
Implements a
renderConversationForModelMultiActions
. Not called yet fromgetNextAction
.For convenience it's implemented with a simple cli command.
Result:
Risk
Not called from the product yet so not risky and rollbackable.
Deploy Plan
Deploy front.