mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-10 14:27:00 -04:00
fix(cache/ccr): don't count a successful eviction as a retrieval (#2106)
## Description
The compression feedback learner treats a *successful* compression as
evidence that it should compress less, which inverts the learning
signal.
When `CompressionStore` evicts an entry that was never retrieved, it
emits a synthetic event to tell the learner the compression was fine
(the model never needed the original):
```python
success_event = RetrievalEvent(..., retrieval_type="eviction_success")
self._pending_feedback_events.append(success_event)
```
`process_pending_feedback` forwards every pending event to
`CompressionFeedback.record_retrieval` unconditionally. But
`record_retrieval` has no branch for `"eviction_success"` — and since
that string isn't `"full"`, it lands in the `else`:
```python
self._total_retrievals += 1
pattern.total_retrievals += 1
if event.retrieval_type == "full":
pattern.full_retrievals += 1
else:
pattern.search_retrievals += 1 # <-- eviction_success counted here
```
So a compression that worked is booked as a *search retrieval*, which
raises the tool's `retrieval_rate` and `search_rate`.
`get_compression_hints` reads a high retrieval rate as "we're
compressing too aggressively" and recommends larger `max_items` / lower
aggressiveness (or `skip_compression`). Net effect: the more often
compression succeeds, the more the learner backs off from compressing. A
standalone repro books a single successful eviction as a 100% retrieval
rate.
Every sibling consumer of the event distinguishes the type — telemetry
and TOIN both receive `retrieval_type="eviction_success"` and handle it
as its own thing. Only the local feedback counter ignores the
distinction.
## Fix
Recognize `"eviction_success"` in `record_retrieval` and leave it out of
the retrieval counters. The compression itself is already counted by
`record_compression` at store time, so an entry that is compressed and
never retrieved already yields a low retrieval rate — which is the
correct "compression worked" signal. Genuine `full`/`search` retrievals
are unchanged.
Closes #
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Code refactoring (no functional changes)
## Changes Made
- `headroom/cache/compression_feedback.py`: early-return in
`record_retrieval` for `retrieval_type == "eviction_success"` so it is
not counted as a retrieval, with a comment explaining the signal.
- `tests/test_ccr_feedback.py`: add
`test_eviction_success_is_not_counted_as_retrieval` (asserts the
counters stay at zero after a successful eviction, and that a genuine
retrieval afterward still counts).
- `CHANGELOG.md`: Bug Fixes entry.
## Testing
- [ ] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [ ] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed
### Test Output
```text
$ uvx ruff@0.15.17 check headroom/cache/compression_feedback.py tests/test_ccr_feedback.py
All checks passed!
$ python -m py_compile headroom/cache/compression_feedback.py tests/test_ccr_feedback.py
OK
```
## Real Behavior Proof
- Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17`. Importing
`headroom` pulls in the torch/transformers stack and a full `pytest`
gets OOM-killed on this box, so I verified the counting logic with a
dependency-free script that replicates
`record_compression`/`record_retrieval` and the
`retrieval_rate`/`search_rate` properties, and left the full pytest to
CI.
- Exact command / steps: recorded one compression, then a
`retrieval_type="eviction_success"` event, under the old counting (no
branch) and the new counting (early return), plus a genuine `search`
retrieval as a control.
- Observed result: old counting books the successful eviction as a
retrieval — `retrieval_rate=1.0`, `search_rate=1.0` — so the learner
would back off from compressing; new counting leaves
`retrieval_rate=0.0` and `total_retrievals=0`; a real retrieval
afterward still increments to 1. The new test asserts exactly this.
- Not tested: an end-to-end store-evict-then-hint cycle through
`CompressionStore.process_pending_feedback`; full local `pytest`
deferred to CI (OOM, per above).
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Checklist
- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] I have updated the CHANGELOG.md if applicable
## Additional Notes
The "unit tests pass locally" and "type checking" boxes are unchecked
because the full suite imports the ML stack, which I can't run in this
environment; the fix is a single early-return in a pure counting method,
verified by the standalone proof and the new regression test (which
reuses the existing `test_ccr_feedback.py` pattern) for CI. Scope is
deliberately limited to the local feedback learner — telemetry and TOIN
already receive the `eviction_success` type and handle it separately.
Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
parent
aa788164fd
commit
eecb81e847
3 changed files with 58 additions and 0 deletions
|
|
@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
### Bug Fixes
|
||||
|
||||
* **cache/ccr:** stop counting a successful eviction as a retrieval in the compression feedback learner, which inverted the learning signal. When an entry is evicted without ever being retrieved, `CompressionStore` emits a synthetic `retrieval_type="eviction_success"` event to mark that the compression was sufficient (the LLM never needed the original). `CompressionFeedback.record_retrieval` had no branch for it, so — because the type is not `"full"` — it was counted as a *search retrieval*, inflating the tool's `retrieval_rate`/`search_rate`. `get_compression_hints` reads a high retrieval rate as "compressing too aggressively" and backs off, so a compression that actually worked pushed the learner toward *less* compression (a standalone repro scores one successful eviction as a 100% retrieval rate). The event is now recognized and left out of the retrieval counters; the compression is still counted by `record_compression` at store time, so a never-retrieved entry correctly yields a low retrieval rate. Genuine retrievals are unaffected.
|
||||
* **proxy/anthropic:** don't launder a non-2xx upstream into HTTP 200 when enterprise security scans the response. On the non-streaming `/v1/messages` path the response-scan branch rebuilt the reply as `httpx.Response(status_code=200)` and returned it without checking the upstream status, so a rate-limit (429), overloaded (529), or other 4xx error whose JSON body was scanned reached the client as an HTTP 200 — the client's retry/backoff never fired and an error looked like success. The branch is now gated on a 200 upstream, matching the sibling CCR/cache/buffered-stream blocks in the same handler; non-2xx responses fall through and keep their real status.
|
||||
* **learn:** classify timeout and connection tool failures correctly instead of as generic runtime errors. In `classify_error` the generic `RUNTIME_ERROR` pattern (`Traceback|Exception:|Error:`) was checked before the dedicated `TIMEOUT` and `CONNECTION_ERROR` patterns. Because every Python exception repr is `XxxError: ...`, a `TimeoutError: ...` or `ConnectionError: ...` matched the catch-all first and was miscategorized as `RUNTIME_ERROR`, leaving those two categories unreachable for the common colon-repr form (they only fired for tokenless phrasings like `deadline exceeded`). The `TIMEOUT` and `CONNECTION_ERROR` patterns are now checked before the generic catch-all; tokenless generic errors still classify as `RUNTIME_ERROR`.
|
||||
* **tokenizers:** resolve HuggingFace tokenizer names by the most-specific prefix. `get_tokenizer_name` scanned `MODEL_TO_TOKENIZER` in dict-insertion order and returned the first key the model merely starts with, so a short family key shadowed a more-specific one — `qwen2-7b-instruct` matched `qwen` before `qwen2`/`qwen2-7b` and resolved to the Qwen1 tokenizer (a different vocabulary, hence wrong token counts); `qwen2.5-*` and `deepseek-v2.x` were mis-resolved the same way. It now picks the longest matching prefix, mirroring the order-dependent-prefix guard the sibling tiktoken `get_encoding_for_model` already documents.
|
||||
|
|
|
|||
13
headroom/cache/compression_feedback.py
vendored
13
headroom/cache/compression_feedback.py
vendored
|
|
@ -273,6 +273,19 @@ class CompressionFeedback:
|
|||
if not tool_name:
|
||||
return
|
||||
|
||||
# An entry evicted without ever being retrieved is a compression
|
||||
# SUCCESS, not a retrieval: the LLM never needed the original data (see
|
||||
# CompressionStore._record_eviction_success). It arrives here as
|
||||
# retrieval_type="eviction_success"; because that is not "full" it used
|
||||
# to fall into the search_retrievals branch below and inflate
|
||||
# retrieval_rate/search_rate, which drove get_compression_hints toward
|
||||
# LESS aggressive compression -- the inverse of the intended signal. The
|
||||
# compression itself was already counted by record_compression at store
|
||||
# time, so a never-retrieved entry already yields a low retrieval rate;
|
||||
# this event must not be counted as a retrieval.
|
||||
if event.retrieval_type == "eviction_success":
|
||||
return
|
||||
|
||||
with self._lock:
|
||||
self._total_retrievals += 1
|
||||
|
||||
|
|
|
|||
|
|
@ -86,6 +86,50 @@ class TestCompressionFeedback:
|
|||
assert pattern.retrieval_rate == 0.5
|
||||
assert pattern.full_retrieval_rate == 1.0 # All were full retrievals
|
||||
|
||||
def test_eviction_success_is_not_counted_as_retrieval(self):
|
||||
"""An eviction-without-retrieval is a compression success, not a retrieval.
|
||||
|
||||
The event arrives with retrieval_type="eviction_success". Because that
|
||||
isn't "full" it used to fall into the search_retrievals branch and
|
||||
inflate retrieval_rate/search_rate, driving get_compression_hints toward
|
||||
less aggressive compression — the inverse of the intended signal. It must
|
||||
leave the retrieval counters untouched.
|
||||
"""
|
||||
feedback = CompressionFeedback()
|
||||
feedback.record_compression("test_tool", 100, 10)
|
||||
|
||||
event = RetrievalEvent(
|
||||
hash="abc123",
|
||||
query=None,
|
||||
items_retrieved=0,
|
||||
total_items=100,
|
||||
tool_name="test_tool",
|
||||
timestamp=time.time(),
|
||||
retrieval_type="eviction_success",
|
||||
)
|
||||
feedback.record_retrieval(event, strategy="smart")
|
||||
|
||||
pattern = feedback.get_all_patterns()["test_tool"]
|
||||
assert pattern.total_retrievals == 0
|
||||
assert pattern.search_retrievals == 0
|
||||
assert pattern.retrieval_rate == 0.0 # a successful compression, not a retrieval
|
||||
|
||||
# A genuine retrieval afterward is still counted.
|
||||
feedback.record_retrieval(
|
||||
RetrievalEvent(
|
||||
hash="def456",
|
||||
query="find errors",
|
||||
items_retrieved=50,
|
||||
total_items=100,
|
||||
tool_name="test_tool",
|
||||
timestamp=time.time(),
|
||||
retrieval_type="search",
|
||||
)
|
||||
)
|
||||
pattern = feedback.get_all_patterns()["test_tool"]
|
||||
assert pattern.total_retrievals == 1
|
||||
assert pattern.search_retrievals == 1
|
||||
|
||||
def test_hints_default_with_no_data(self):
|
||||
"""Default hints returned when no data exists."""
|
||||
feedback = CompressionFeedback()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue