-
Notifications
You must be signed in to change notification settings - Fork 101
fix(condenser): Retry on empty condensation #1577
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
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
🧪 Integration Tests ResultsOverall Success Rate: 92.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_moonshot_kimi_k2_thinking
Skipped Tests:
Failed Tests:
litellm_proxy_gpt_5.1_codex_max
Skipped Tests:
Failed Tests:
litellm_proxy_deepseek_deepseek_chat
Skipped Tests:
litellm_proxy_claude_sonnet_4_5_20250929
litellm_proxy_vertex_ai_gemini_3_pro_preview
litellm_proxy_mistral_devstral_2512
Skipped Tests:
Failed Tests:
|
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
🧪 Integration Tests ResultsOverall Success Rate: 98.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_deepseek_deepseek_chat
Skipped Tests:
litellm_proxy_claude_sonnet_4_5_20250929
litellm_proxy_vertex_ai_gemini_3_pro_preview
litellm_proxy_mistral_devstral_2512
Skipped Tests:
Failed Tests:
litellm_proxy_moonshot_kimi_k2_thinking
Skipped Tests:
litellm_proxy_gpt_5.1_codex_max
Skipped Tests:
|
|
There are two integration test runs here -- the first one has the token condenser test failing for Devstral, but it's the same failure mode as with GPT 5.1. I'm hesitant to mark it skipped because the failure seems to be intermittent. |
|
IMPORTANT NOTE: This PR is not a complete solution. There are still some strange cases where we can get caught in loops. For example, if a context window exception is thrown we'll end up retrying into infinity. |
Could we fail hard in this case? IMO fail hard is better here than going into infinite loop though |
That's doable. The challenge right now is just figuring out when we're in that situation. I've got some draft changes that make that easy though. |
|
[Automatic Post]: I have assigned @xingyaoww as a reviewer based on git blame information. Thanks in advance for the help! |
xingyaoww
left a comment
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.
Just one comment, otherwise LGTM
|
Hey @xingyaoww can you take another look now? Here are the changes from what you saw:
This flow lets us wait until the manipulation indices are good when we cross the event count threshold but throw an error in pretty much all other cases. Next step (in a separate PR) is instead condensing everything if we get a hard requirement and no condensation. |
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.
LGTM! Let's run integration test - good to merge when it passes
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
🧪 Integration Tests ResultsOverall Success Rate: 98.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_gpt_5.1_codex_max
Skipped Tests:
litellm_proxy_deepseek_deepseek_chat
Skipped Tests:
litellm_proxy_claude_sonnet_4_5_20250929
litellm_proxy_mistral_devstral_2512
Skipped Tests:
Failed Tests:
litellm_proxy_moonshot_kimi_k2_thinking
Skipped Tests:
litellm_proxy_vertex_ai_gemini_3_pro_preview
|
xingyaoww
left a comment
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.
LGTM!
Summary
Adds a graceful failure mechanism when a condenser doesn't forget any events. Instead of throwing an uncaught exception, we now throw a
NoCondensationAvailableExceptionwhich is caught by the base condenser class and returns the view provided.Notably, this means if we cannot find a condensation (because, e.g., the entirety of the message history is a tool loop) then the condensation will be effectively delayed until a valid chunk of events can be forgotten.
Also updates the condenser integration test to report the number of events condensed in the first condensation, adds unit tests for the exception handling, and updates a handful of other related tests.
Intended to address #1518.
Checklist
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:22eb6f2-pythonRun
All tags pushed for this build
About Multi-Architecture Support
22eb6f2-python) is a multi-arch manifest supporting both amd64 and arm6422eb6f2-python-amd64) are also available if needed