fix(agent): isolate output-cap recovery from compression - #62197
fix(agent): isolate output-cap recovery from compression#62197Vansh5632 wants to merge 2 commits into
Conversation
aafc9b0 to
de84f00
Compare
Fix PR for open #61761 (output-cap retry loop never converges when the fixed margin is erased by per-retry input drift). Competing/related open fix PRs for the same issue via different mechanisms: #61228 (raise the 64->512 margin + overflow-spiral guard) and #61846 (grow the margin exponentially). This PR instead uses a 128-token + observed-input-growth reserve, a dedicated output-cap retry counter (separated from compression accounting), and keeps the retry in the inner API loop. Flagging the cluster so a maintainer can pick the canonical approach. Note: this PR also bundles an unrelated 291-line |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real recovery-path bug: current main still uses the fixed 64-token reserve and charges parsed output-cap retries to compression accounting at agent/conversation_loop.py:3451-3479.
Problems
- The new handler remains after the disabled-auto-compaction guard at
agent/conversation_loop.py:3021-3076. vLLM'smaximum context lengthwording matches_CONTEXT_OVERFLOW_PATTERNSinagent/error_classifier.py:242-253, so withcompression.enabled: falsethe turn returnscompaction_disabledbefore it can perform the output-cap retry. The PR's added coverage uses the shared fixture that forcescompression_enabled = Trueattests/run_agent/test_413_compression.py:97-102.
Suggested changes
- Route a parsed output-cap error around that guard, while leaving true input-overflow behavior unchanged.
- Add a vLLM-format regression with auto-compaction disabled that verifies a reduced-cap retry succeeds without calling
_compress_context.
This is an automated hermes-sweeper review.
| safe_out = max(1, available_out - 64) # small safety margin | ||
| # Rebuild only the request kwargs in the inner retry loop: | ||
| # the conversation itself already fits and must not be | ||
| # compacted or reprocessed through the outer agent loop. |
There was a problem hiding this comment.
This parsed recovery still sits after the earlier compression.enabled: false overflow return (agent/conversation_loop.py:3021-3076). vLLM wording is classified as context_overflow, so that configuration exits before this retry. Route parsed output-cap errors around the guard and add a disabled-compaction regression.
Route recoverable output-cap errors around the compression.enabled: false overflow guard so inner-loop max_tokens retries still run when auto-compaction is off. Add a vLLM-format regression test for the disabled-compaction path.
|
Closing as superseded, with thanks. Since this was filed, main took a different direction for the drift problem: #55546 made the output-cap retry path compress alongside the clamp (input drift can no longer erase the margin unnoticed), and #90563 (salvaging #89923, auto-merge armed) removes the root cause of the per-retry drift itself — vLLM's "at least N input tokens" figure is back-computed from the constraint, so the fix detects that degenerate shape and halves the cap instead of trusting it. With those two in, the adaptive-margin machinery here would be dead weight on a path that no longer drifts. Two ideas from this PR did survive into the merged work and are credited: treating output-cap exhaustion as its own failure class rather than "compression failed", and reserving for observed growth between retries (now unnecessary post-#89923, but it informed the review). Appreciate the thorough test coverage — the +65-token drift scenario in your tests is exactly what the convergence regression test now pins. |
What does this PR do?
Fixes a retry loop for output-cap errors on OpenAI-compatible providers such as local vLLM. This is not a conversation-too-large problem: the prompt fits, but the requested output plus the prompt exceeds the model context window. Previously Hermes used a fixed 64-token reserve, rebuilt the whole outer loop after each failure, and counted those retries as compression attempts. If the provider reported even 65 extra input tokens on the next request, every retry remained just over the limit and eventually reported a misleading compression failure.
The recovery now retries inside the existing API loop with the same message payload, uses an output-cap-specific retry budget and error message, and reserves 128 tokens plus any observed input growth from the prior output-cap error.
Related Issue
Fixes #61761
Related: #61846 addresses the fixed margin only. This change also keeps output-cap recovery in the inner API retry loop and separates it from compression accounting.
Type of Change
Changes Made
The output-cap recovery now calculates a safer one-shot output limit through
compute_safe_output_tokens()rather than subtracting a fixed 64 tokens. It keeps the retry in the inner API loop, so Hermes rebuilds only request kwargs and does not rerun outer-loop hooks, middleware, or compression-related work. Output-cap retries have their own bounded counter and return an output-budget-specific error if they cannot recover, instead of claiming that context compression failed. Regression tests cover the vLLM error format, +65-token input growth, stable outbound messages, no compression, and the new terminal result.How to Test
Run the targeted regression suite:
Result:
78 passed.The full
scripts/run_tests.shsuite was also run. It did not finish green because 7 tests outside this change failed and 3 unrelated test files hit the per-file timeout; this PR's targeted regression suite passes.Reproduce the original failure using the local mock vLLM server and throwaway
repro-outcapHermes profile. The mock reports +65 input tokens on each retry.Before this change, the fixed 64-token reserve never converged:
After this change, the same smaller local repro succeeds on its second request:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (full CI-parity suite was attempted; see test note above)Documentation & Housekeeping