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.

Sign up for GitHub

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: retrieve conversation metadata when loading conversation #27

Merged
merged 4 commits into from
Dec 1, 2023
Merged

feat: retrieve conversation metadata when loading conversation #27

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ca502e
feat: retrieve conversation metadata when loading conversation
SuZhou-Joe Nov 30, 2023
34c6e9c
feat: change /_plugins/_ml to a constant
SuZhou-Joe Dec 1, 2023
c5d1a71
feat: change import
SuZhou-Joe Dec 1, 2023
3f6c046
feat: optimize code
SuZhou-Joe Dec 1, 2023
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
2 changes: 1 addition & 1 deletion common/types/chat_saved_object_attributes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export interface Interaction {

export interface ISession {
title: string;
version: number;
version?: number;
createdTimeMs: number;
updatedTimeMs: number;
messages: IMessage[];
Expand Down
4 changes: 2 additions & 2 deletions public/hooks/use_sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ export const usePatchSession = () => {
dispatch({ type: 'request', abortController });
return core.services.http
.put(`${ASSISTANT_API.SESSION}/${sessionId}`, {
query: {
body: JSON.stringify({
title,
},
}),
signal: abortController.signal,
})
.then((payload) => dispatch({ type: 'success', payload }))
Expand Down
4 changes: 2 additions & 2 deletions server/routes/chat_routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ const updateSessionRoute = {
params: schema.object({
sessionId: schema.string(),
}),
query: schema.object({
body: schema.object({
title: schema.string(),
}),
},
Expand Down Expand Up @@ -225,7 +225,7 @@ export function registerChatRoutes(router: IRouter, routeOptions: RoutesOptions)
try {
const getResponse = await storageService.updateSession(
request.params.sessionId,
request.query.title
request.body.title
);
return response.ok({ body: getResponse });
} catch (error) {
Expand Down
50 changes: 26 additions & 24 deletions server/services/storage/agent_framework_storage_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { ApiResponse } from '@opensearch-project/opensearch/.';
import { TransportRequestPromise } from '@opensearch-project/opensearch/lib/Transport';
Copy link
Member

Choose a reason for hiding this comment

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

nit: i think api response can go in this import statement as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, done for that.

import { AgentFrameworkTrace } from '../../../common/utils/llm_chat/traces';
import { OpenSearchClient } from '../../../../../src/core/server';
import {
Expand All @@ -29,37 +30,38 @@ export class AgentFrameworkStorageService implements StorageService {
private readonly messageParsers: MessageParser[] = []
) {}
async getSession(sessionId: string): Promise<ISession> {
const session = (await this.client.transport.request({
method: 'GET',
path: `/_plugins/_ml/memory/conversation/${sessionId}/_list`,
})) as ApiResponse<{
interactions: Interaction[];
}>;
const [interactionsResp, conversation] = await Promise.all([
Copy link
Member

Choose a reason for hiding this comment

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

OpenSearch Dashboards supports batching concurrent requests when enabling courier:batchSearches which ends up making the application utilize _msearch.

Does the ML Commons plugin support batch requests? Could potentially save some time for total roundtrip so that it just batches these two requests together instead of making two requests and waiting for two responses.

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks kavilla, I do not think there is a mechanism in current opensearchClient that can batch concurrent requests to backend plugin. ML_common only gives APIs to retrieve the data we need. And it is better that Node.js has no knowledge about the implementation detail of how backend store these data.

As for roundtrip, I think that will be OK because we send these two requests in parallel and thus we only need to pay for one roundtrip time.

this.client.transport.request({
method: 'GET',
path: `/_plugins/_ml/memory/conversation/${sessionId}/_list`,
Copy link
Member

Choose a reason for hiding this comment

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

NIT: #22 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done for this.

}) as TransportRequestPromise<
ApiResponse<{
interactions: Interaction[];
}>
>,
this.client.transport.request({
method: 'GET',
path: `/_plugins/_ml/memory/conversation/${sessionId}`,
}) as TransportRequestPromise<
ApiResponse<{
conversation_id: string;
create_time: string;
updated_time: string;
name: string;
}>
>,
]);
const messageParserRunner = new MessageParserRunner(this.messageParsers);
const finalInteractions: Interaction[] = [...session.body.interactions];
const finalInteractions: Interaction[] = [...interactionsResp.body.interactions];
Copy link
Member

Choose a reason for hiding this comment

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

is the spread needed or is this just const finalInteractions: Interaction[] = interactionsResp.body.interactions;?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, thanks for catching.


/**
* Sort interactions according to create_time
*/
finalInteractions.sort((interactionA, interactionB) => {
const { create_time: createTimeA } = interactionA;
const { create_time: createTimeB } = interactionB;
const createTimeMSA = +new Date(createTimeA);
const createTimeMSB = +new Date(createTimeB);
if (isNaN(createTimeMSA) || isNaN(createTimeMSB)) {
return 0;
}
return createTimeMSA - createTimeMSB;
});
let finalMessages: IMessage[] = [];
for (const interaction of finalInteractions) {
finalMessages = [...finalMessages, ...(await messageParserRunner.run(interaction))];
}
return {
title: 'test',
version: 1,
createdTimeMs: Date.now(),
updatedTimeMs: Date.now(),
title: conversation.body.name,
createdTimeMs: +new Date(conversation.body.create_time),
Copy link
Member

Choose a reason for hiding this comment

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

is it still Ms?

Copy link
Member

Choose a reason for hiding this comment

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

+new Date(xxx) is milliseconds, but do we need to convert them to epoch or should we use string? since UI doesn't display epoch number. I originally wrote this because i thought backend would return epoch

Copy link
Member Author

Choose a reason for hiding this comment

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

An epoch is easy to convert into string or Date both in Browser and Node.js. For a string type, it is hard tell which format it is using from the type declaration.

updatedTimeMs: +new Date(conversation.body.updated_time),
messages: finalMessages,
interactions: finalInteractions,
};
Expand Down
Loading