-
Notifications
You must be signed in to change notification settings - Fork 23
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
Integrate feedback(Thumb up/down) with memory API #22
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,25 +10,8 @@ import { useChatContext } from '../contexts/chat_context'; | |
import { useCore } from '../contexts/core_context'; | ||
import { useChatState } from './use_chat_state'; | ||
|
||
interface AccountResponse { | ||
data: { user_name: string; user_requested_tenant: string; roles: string[] }; | ||
} | ||
|
||
interface SendFeedbackBody { | ||
metadata: { | ||
type: 'event_analytics' | 'chat' | 'ppl_submit'; | ||
sessionId?: string; | ||
traceId?: string; | ||
error?: boolean; | ||
user: string; | ||
tenant: string; | ||
}; | ||
input: string; | ||
output: string; | ||
correct: boolean | undefined; | ||
// Currently unused but required. | ||
expectedOutput: string; | ||
comment: string; | ||
feedback: boolean; | ||
} | ||
|
||
export const useFeedback = () => { | ||
|
@@ -39,7 +22,7 @@ export const useFeedback = () => { | |
|
||
const sendFeedback = async (message: IOutput, correct: boolean) => { | ||
const outputMessage = message; | ||
// Markdown type output all has traceId. | ||
// Markdown type output all has traceId. The traceId of message is equal to interaction id. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though traceId == interactionId, I think it's better we use interactionId explicitly, rather than rely on traceId. @SuZhou-Joe Do we already get interaction id of each message at FE? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For each |
||
const outputMessageIndex = chatState.messages.findIndex((item) => { | ||
return item.type === 'output' && item.traceId === message.traceId; | ||
}); | ||
|
@@ -50,25 +33,12 @@ export const useFeedback = () => { | |
return; | ||
} | ||
|
||
const { username, tenant } = chatContext.currentAccount; | ||
const body: SendFeedbackBody = { | ||
metadata: { | ||
type: 'chat', // currently type is only chat in feedback | ||
sessionId: chatContext.sessionId, | ||
traceId: outputMessage.traceId, | ||
error: false, | ||
user: username, | ||
tenant, | ||
}, | ||
input: inputMessage.content, | ||
output: outputMessage.content, | ||
correct, | ||
expectedOutput: '', | ||
comment: '', | ||
feedback: correct, | ||
}; | ||
|
||
try { | ||
await core.services.http.post(ASSISTANT_API.FEEDBACK, { | ||
await core.services.http.put(`${ASSISTANT_API.FEEDBACK}/${message.traceId}`, { | ||
body: JSON.stringify(body), | ||
}); | ||
setFeedbackResult(correct); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,18 @@ const updateSessionRoute = { | |
}, | ||
}; | ||
|
||
const feedbackRoute = { | ||
path: `${ASSISTANT_API.FEEDBACK}/{interactionId}`, | ||
validate: { | ||
params: schema.object({ | ||
interactionId: schema.string(), | ||
}), | ||
body: schema.object({ | ||
feedback: schema.boolean(), | ||
}), | ||
}, | ||
}; | ||
|
||
export function registerChatRoutes(router: IRouter, routeOptions: RoutesOptions) { | ||
const createStorageService = (context: RequestHandlerContext) => | ||
new AgentFrameworkStorageService( | ||
|
@@ -293,4 +305,24 @@ export function registerChatRoutes(router: IRouter, routeOptions: RoutesOptions) | |
} | ||
} | ||
); | ||
|
||
router.put( | ||
feedbackRoute, | ||
async ( | ||
context, | ||
request, | ||
response | ||
): Promise<IOpenSearchDashboardsResponse<HttpResponsePayload | ResponseError>> => { | ||
const storageService = createStorageService(context); | ||
const { interactionId } = request.params; | ||
|
||
try { | ||
const getResponse = await storageService.updateInteraction(interactionId, request.body); | ||
return response.ok({ body: getResponse }); | ||
} catch (error) { | ||
context.assistant_plugin.logger.error(error); | ||
return response.custom({ statusCode: error.statusCode || 500, body: error.message }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to return back an error code? For interaction I'd imagine it could be a fire and forget thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think users should be notified if their feedback didn't go through? |
||
} | ||
} | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,4 +181,32 @@ export class AgentFrameworkStorageService implements StorageService { | |
throw new Error('update converstaion failed, reason:' + JSON.stringify(error.meta?.body)); | ||
} | ||
} | ||
|
||
async updateInteraction( | ||
interactionId: string, | ||
additionalInfo: Record<string, string | boolean> | ||
): Promise<SessionOptResponse> { | ||
try { | ||
const response = await this.client.transport.request({ | ||
method: 'PUT', | ||
path: `/_plugins/_ml/memory/interaction/${interactionId}/_update`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can path be part of the constants file too? like |
||
body: { | ||
additional_info: additionalInfo, | ||
}, | ||
}); | ||
if (response.statusCode === 200) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i might be wrong but i thought javascript client throws errors on non success. if it's not |
||
return { | ||
success: true, | ||
}; | ||
} else { | ||
return { | ||
success: false, | ||
statusCode: response.statusCode, | ||
message: JSON.stringify(response.body), | ||
}; | ||
} | ||
} catch (error) { | ||
throw new Error('update interaction failed, reason:' + JSON.stringify(error.meta?.body)); | ||
} | ||
} | ||
} |
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.
Shall we make
feedback
an object for extensibility? something like: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 think sendFeedbackBody is already an object for feedback related content, but i think we can pass an object about feedback directly instead of flat it.
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.
Yes, you're right