fix(compression): Discard compression result if it results in more token usage#7047
fix(compression): Discard compression result if it results in more token usage#7047richieforeman merged 23 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @richieforeman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a crucial fix to the chat compression logic, ensuring that the system will not utilize a compressed context if the compression process paradoxically leads to an increase in token usage. This enhancement prevents potential inefficiencies and higher costs associated with inflated token counts, making the compression mechanism more robust and reliable. It directly addresses the first part of issue #3533 by refining how chat history is managed for optimal performance.
Highlights
- Preventing Token Inflation During Chat Compression: A new validation has been added to the
tryCompressChatmethod to ensure that the compression process does not inadvertently increase the token count. If the compressed output results in more tokens than the original input, the compression result is now discarded. - New Test Case for Compression Validation: A dedicated unit test has been introduced to specifically verify that the
tryCompressChatfunction correctly handles scenarios where compression leads to an increase in token usage, ensuring it returnsnullas expected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a safeguard to discard chat history compression if it results in a higher token count, which is a valuable improvement. However, the current implementation has a critical flaw: when an inflated compression is discarded, the original chat history is not restored, leading to the loss of the conversation. My review focuses on this critical issue and provides a detailed recommendation for fixing it by properly managing and restoring the chat state.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
This will kick of a compression event again in the next user prompt. Is this the intended behavior? But also, we can create a hard stop on the output using the |
I am not observing that, can you help me understand more? 
I like the idea with maxOutputTokens, i'll fiddle with that and verify. |
I had thought that if the token count didn't decrease, the same conditional would pick it up on the next user input where we check the token count. This would cause another compression attempt which may succeed but could also fail since we're not changing any system instructions. I'm mostly referring to when the context size increases at the 80% point. |
That's a great point, I've addressed this in the CL |
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved after this small comment is addressed.
packages/core/src/core/client.ts
Outdated
|
|
||
| if (newTokenCount > originalTokenCount) { | ||
| this.hasFailedCompressionAttempt = !force && true; | ||
| console.warn('Compression incorrectly inflated the token count.'); |
There was a problem hiding this comment.
can we continue to return the original and new token count and add an extra bool indicating that the request was skipped. this will allow the CLI package to show a meaningful message explaining why the request was skipped which is more useful then logging to the console. in fact the pr for such a message has already landed :)
There was a problem hiding this comment.
abhipatel12
left a comment
abhipatel12
left a comment
There was a problem hiding this comment.
Thanks for taking this on! Left a few comments! Lmk what you think!
| @@ -846,7 +869,13 @@ export class GeminiClient { | |||
| }); | |||
There was a problem hiding this comment.
I can't click the proper line b/c that's just how this review tool works 😤
But one line above, we do contents: this.getChat().getHistory(), to count the tokens. Should we be counting the tokens in the proposed new chat? Which would be:
contents: chat.getHistory(), There was a problem hiding this comment.
I've updated this. This is not possible to test without a larger refactor of the tests. Currently, the tests are actually testing a mock of some of the bits of Client. In many ways, these tests are actually mocking the unit-under-test, which is very harmful and dangerous. I can explore making these tests test the actual implementation in a follow up, but I fear it will be a more involved refactor of the tests as a whole.
There was a problem hiding this comment.
Sg thanks for looking into it!
richieforeman
changed the title
3 participants
TLDR
Refuse to use the new "compressed" context if the compression result has more tokens that the source.
Testing Matrix
Linked issues / bugs
This is the first part of 3533 and probably the least controversial thing that should just be shipped 🚢