Skip to content
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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 4 additions & 34 deletions public/hooks/use_feed_back.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

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:

feedback: {
  user_satisfaction: boolean
}

Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right

}

export const useFeedback = () => {
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For each markdown message, yes.

const outputMessageIndex = chatState.messages.findIndex((item) => {
return item.type === 'output' && item.traceId === message.traceId;
});
Expand All @@ -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);
Expand Down
32 changes: 32 additions & 0 deletions server/routes/chat_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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 });
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

}
}
);
}
44 changes: 0 additions & 44 deletions server/routes/langchain_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,48 +130,4 @@ export function registerLangchainRoutes(router: IRouter) {
}
}
);

router.post(
{
path: ASSISTANT_API.FEEDBACK,
validate: {
body: schema.object({
metadata: schema.object({
user: schema.string(),
tenant: schema.string(),
type: schema.string(),
sessionId: schema.maybe(schema.string()),
traceId: schema.maybe(schema.string()),
error: schema.maybe(schema.boolean()),
selectedIndex: schema.maybe(schema.string()),
}),
input: schema.string(),
output: schema.string(),
correct: schema.boolean(),
expectedOutput: schema.string(),
comment: schema.string(),
}),
},
},
async (
context,
request,
response
): Promise<IOpenSearchDashboardsResponse<HttpResponsePayload | ResponseError>> => {
try {
await context.core.opensearch.client.asCurrentUser.index({
index: LLM_INDEX.FEEDBACK,
body: { ...request.body, timestamp: new Date().toISOString() },
});

return response.ok();
} catch (error) {
console.error(error);
return response.custom({
statusCode: error.statusCode || 500,
body: error.message,
});
}
}
);
}
28 changes: 28 additions & 0 deletions server/services/storage/agent_framework_storage_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can path be part of the constants file too?

like ${OPENSEARCH_ML}

body: {
additional_info: additionalInfo,
},
});
if (response.statusCode === 200) {
Copy link
Member

Choose a reason for hiding this comment

The 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 200 would it go to catch block?

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));
}
}
}
Loading