-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add token-based condensing threshold support #7441
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
- Allow configuring condensing threshold in tokens (values > 100) - Percentage thresholds remain for values between 5-100 - Token thresholds take precedence when both could apply - Add comprehensive tests for token-based thresholds Addresses #7440
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| // Truncate if we're within TOKEN_BUFFER_PERCENTAGE of the context window | ||
| const allowedTokens = contextWindow * (1 - TOKEN_BUFFER_PERCENTAGE) - reservedTokens | ||
|
|
||
| // Determine the effective threshold to use |
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.
Would it be helpful to add a comment here explaining the threshold logic? Something like:
| // Determine the effective threshold to use | |
| // Determine the effective threshold to use | |
| // Values interpretation: | |
| // -1: inherit from global setting | |
| // 5-100: percentage of context window | |
| // >100: absolute token count | |
| let effectiveThreshold = autoCondenseContextPercent |
This would make it immediately clear to future maintainers how the different threshold values are interpreted.
| // Valid percentage threshold | ||
| effectiveThreshold = profileThreshold | ||
| } else if (profileThreshold > MAX_CONDENSE_THRESHOLD) { | ||
| // Values above 100 are treated as token counts |
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.
Should we consider adding an upper bound check for token thresholds? For example, values larger than typical context windows (e.g., > 1,000,000) might indicate a configuration error.
| effectiveTokenThreshold = profileThreshold | ||
| } else { | ||
| // Invalid threshold value, fall back to global setting | ||
| console.warn( |
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 warning message could be more informative about the new token-based thresholds:
| console.warn( | |
| console.warn( | |
| `Invalid profile threshold ${profileThreshold} for profile "${currentProfileId}". Valid values are: -1 (inherit), 5-100 (percentage), or >100 (token count). Using global default of ${autoCondenseContextPercent}%`, | |
| ) |
| const role = i % 2 === 0 ? "user" : "assistant" | ||
| // Create content that roughly corresponds to the desired token count | ||
| // This is a simplification - actual token count depends on the tokenizer | ||
| const content = "x".repeat(tokensPerMessage * 4) // Rough approximation |
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.
Would it be worth adding a comment explaining this approximation? The 4 characters per token ratio is a testing simplification that future maintainers might find helpful to understand.
| } else if (profileThreshold >= MIN_CONDENSE_THRESHOLD && profileThreshold <= MAX_CONDENSE_THRESHOLD) { | ||
| // Valid custom threshold | ||
| // Valid percentage threshold | ||
| effectiveThreshold = profileThreshold |
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.
Consider extracting the magic number 100 as a constant like PERCENTAGE_TOKEN_BOUNDARY. This would make the distinction between percentage and token thresholds clearer throughout the code.
|
Closing this PR since it doesn't properly implement a token-based context threshold. The UI element to change the value for the percentage is a slider that only goes up to 100%. To properly implement this feature, the user should be able to switch between percentage and token modes explicitly, rather than using the ambiguous heuristic of treating values >100 as token counts. This approach would require UI changes to add a toggle or dropdown for selecting the threshold type. |
Summary
This PR adds support for configuring the Condensing Trigger Threshold in tokens (in addition to the existing percentage-based configuration).
Changes
How it works
Example Usage
Users can now configure profile-specific thresholds like:
50= Condense when context reaches 50% of the window8000= Condense when context reaches 8000 tokens (regardless of window size)This provides more precise control for profiles with known models and token limitations.
Testing
Fixes #7440
Important
Adds token-based condensing thresholds to
truncateConversationIfNeeded()inindex.ts, with tests insliding-window.spec.ts.truncateConversationIfNeeded()inindex.ts.sliding-window.spec.ts.This description was created by
for be1511a. You can customize this summary. It will automatically update as commits are pushed.