-
Notifications
You must be signed in to change notification settings - Fork 83
fix: improve history management #2312
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
Conversation
c34b0fb
to
e31a621
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/historyV2 #2312 +/- ##
==================================================
Coverage 60.17% 60.17%
==================================================
Files 256 256
Lines 57054 56982 -72
Branches 3499 3496 -3
==================================================
- Hits 34332 34291 -41
+ Misses 22656 22625 -31
Partials 66 66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e31a621
to
ccf2b70
Compare
// Compaction | ||
// Maximum number of characters per request used for compaction prompt | ||
// 200K tokens * 3.5 = 700K characters, intentionally overestimating with 3.5:1 ratio | ||
export const MAX_OVERALL_CHARACTERS = 700_000 |
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.
Not related to this change but just curious: In case we introduce more new models in the future, and the models might have different limits, we need to change this to a model -> maxChar map?
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
|
||
// 1. Make sure the length of the history messages don't exceed MaxConversationHistoryMessages | ||
let allMessages = this.getMessages(tabId, maxConversationHistoryMessages) | ||
let allMessages = this.getMessages(tabId) |
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.
Just to confirm, this returns all the messages across different conversationId
s under the tabId
? Can you double check if there's a limitation on the number of messages included under history? I don't really remember why we had MaxConversationHistoryMessages
.
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 limit is removed
Problem
Solution
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.