-
Notifications
You must be signed in to change notification settings - Fork 62
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(chat): filter namespace messages from history if it exists in metadata VSCODE-611 #866
Conversation
…de into gagik/add-test-filtering
…de into gagik/one-database-handling
…-js/vscode into gagik/no-database-or-collection-error
…ngodb-js/vscode into gagik/filter-namespace
…ngodb-js/vscode into gagik/filter-namespace
6ed4488
to
4ad4c5f
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.
Nice, left a couple code quality suggestions.
@@ -163,16 +165,27 @@ export abstract class PromptBase<TArgs extends PromptArgsBase> { | |||
protected getHistoryMessages({ |
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.
This function is starting to get a bit long and hard to follow all of the things happening in it.
It already has the // eslint-disable-next-line complexity
which is usually an indicator that we should break it into a few functions, even if it comes with a slightly hit to performance (like running through the messages multiple times).
Should we do that now? Break this function into multiple parts where each is doing a certain thing? That'll also make it more easily unit testable.
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.
More like a question about this area in code and maybe a bit of a request :) if you will refactor this code, could we split the history into something like getUserHistoryMessages
and getAssistantHistoryMessages
. When testing this functionality I found it difficult to print the history content because it is already wrapped into the vscode.LanguageModelChatMessage
format when it is returned here: https://github.com/mongodb-js/vscode/blob/main/src/participant/prompts/promptBase.ts#L130
Maybe, it could be something like:
const messages = [
vscode.LanguageModelChatMessage.Assistant(this.getAssistantPrompt(args)),
vscode.LanguageModelChatMessage.Assistant(this.getAssistantHistoryMessages()),
vscode.LanguageModelChatMessage.User(this.getUserHistoryMessages()),
vscode.LanguageModelChatMessage.User(prompt),
];
Or something like that. The idea here is that we have some unformatted string value to print to see what message we send to the model.
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.
The ordering of the messages here is important, although the last message will always be the user prompt and the first is the assistant prompt, the user history and assistant history aren't sequential.
We do currently log information about the messages we are sending to the model:
vscode/src/participant/participant.ts
Line 143 in d5a9345
messages: modelInput.messages.map( |
@alenakhineika we can add something there that will log them in their entirety, and not just the metadata, if an environment variable is set. How does that sound?
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 am also dealing changing getHistoryMessages in VSCODE-632 so I think it'll be best to deal with some refactoring for it in the PR there.
For the sake of keeping this simple to review, going to merge and do any greater potential refactoring work in the PR for VSCODE-632. |
Scrub the namespace messages from the history when we have identified and are building the messages to send to the ai. We already include the namespace in the query assistant message. We might want to move these to be additions to the user's message instead (in both query and schema).
Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes