-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
60 commits
Select commit
Hold shift + click to select a range
a54d67d
Filter long / invalid prompts
gagik 6ff0d8d
Try helper include
gagik c9aa589
Use content value
gagik 698051f
Use a cleaner test
gagik bf05722
delete old helper
gagik 2445dc5
Add explainer
gagik ff0c1cc
WIP
gagik f9dd536
Move around dependencies
gagik 09c7414
Remove grep
gagik aada8e1
Use firstCall
gagik eb2c7dc
Add test filtering
gagik bcd260b
Update CONTRIBUTING.md
gagik a9beef1
Escape the environment variable
gagik 2e4bd70
Merge branch 'gagik/add-test-filtering' of github.com:mongodb-js/vsco…
gagik 33c995e
Fix wording
gagik 8582193
Add schema tests
gagik a8bc30d
align tests and use a stub
gagik 5ddc7fb
Add saving to metadata
gagik 06435e8
Move things
gagik 5a58171
Better org
gagik fdbabfc
Merge branch 'gagik/add-test-filtering' of github.com:mongodb-js/vsco…
gagik f23e19f
simplify tests and picking logic
gagik 274fe17
typos
gagik 54885d6
Align with broken test
gagik cfb9d61
Add error info and tests
gagik f080016
wrap l10n
gagik 190f4f6
Merge branch 'gagik/one-no-collection-handling' of github.com:mongodb…
gagik 182dc15
wrap in l10n
gagik a341e45
remove settings change
gagik bec38d8
Merge branch 'gagik/no-database-or-collection-error' of github.com:mo…
gagik 548f247
Merge branch 'gagik/fix-long-prompts' of github.com:mongodb-js/vscode…
gagik c1b6534
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/no-dat…
gagik 378b202
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/one-no…
gagik f468adf
Add tests
gagik 62c07ef
Apply suggestions from code review
gagik f2b54ea
changes from review
gagik 276ce90
move error types to inside participant
gagik bf2a0c7
switch to parametrized tests
gagik 02f45dc
Merge branch 'gagik/one-no-collection-handling' into gagik/no-databas…
gagik a1765ef
remove patched vscode
gagik ccacb8a
remove vscode
gagik 886aa45
better comments
gagik 52dbdf6
fix potential CI discrepancy
gagik 686c7cd
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/one-no…
gagik fee668e
combine message text
gagik e1d12bf
add explicit undefined returns
gagik c92cdcb
Merge branch 'gagik/one-no-collection-handling' into gagik/no-databas…
gagik 985ed49
Move tests to parameterized
gagik f012622
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/filter…
gagik aba650c
delete mistaken duplicate
gagik 650bd79
cleanup tests
gagik 0843c64
fix expected history
gagik 6cb9ad3
revert
gagik 0bbeeb2
Merge branch 'main' into gagik/filter-namespace
gagik 377b6bd
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/filter…
gagik 853eccf
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/no-dat…
gagik 86a33f2
Merge branch 'gagik/no-database-or-collection-error' of github.com:mo…
gagik 4ad4c5f
small cleanup
gagik 0a8b085
Merge branch 'main' of github.com:mongodb-js/vscode into gagik/filter…
gagik 3c63e76
Use helpers for tests
gagik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
andgetAssistantHistoryMessages
. When testing this functionality I found it difficult to print the history content because it is already wrapped into thevscode.LanguageModelChatMessage
format when it is returned here: https://github.com/mongodb-js/vscode/blob/main/src/participant/prompts/promptBase.ts#L130Maybe, it could be something like:
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
@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.