-
Notifications
You must be signed in to change notification settings - Fork 109
feat(condenser): Multi-summary views #1721
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi! I started running the condenser tests on your PR. You will receive a comment with the results shortly. Note: These are non-blocking tests that validate condenser functionality across different LLMs. |
Condenser Test Results (Non-Blocking)
🧪 Condenser Tests ResultsOverall Success Rate: 100.0% 📊 Summary
📋 Detailed Resultslitellm_proxy_anthropic_claude_opus_4_5_20251101
litellm_proxy_gpt_5.1_codex_max
Skipped Tests:
|
| if Reason.REQUEST in reasons: | ||
| return CondensationRequirement.HARD | ||
|
|
||
| def _get_summary_event_content(self, view: View) -> str: |
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.
No longer needed -- the previous summaries are now captured as regular events in the summary generation.
all-hands-bot
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.
This PR introduces important functionality for multi-summary views. I found a critical issue with batch atomicity enforcement and some suggestions for improving robustness. Details in inline comments.
|
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 |
|
@enyst While working on this PR I remembered why the manipulation index calculations were strict wrt the Never really mattered with the way we filtered summary events, but definitely matters after this PR. |
|
Ah I see. @OpenHands Do a /codereview-roasted on this PR. Investigate deeply! Respond as a comment on github on this PR, so consider that your output will be rendered as markdown. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
You asked for a /codereview-roasted comment (Markdown) on PR #1721. What I did
Deliverable produced
Checklist vs your request
Repo changes / pushing
If you want, I can also turn the “must fix / must justify” items into a smaller, actionable checklist for the PR author. |
|
🟡 Taste rating: Acceptable [CRITICAL ISSUES] (must fix / at least must justify loudly)1) Deterministic summary IDs are a hack glued to an unrelated constraint[openhands-sdk/openhands/sdk/context/view.py, ~451] summary_id = f"{condensation.id.replace('_', '-')}-summary"You’re encoding a file persistence regex constraint into the semantic identity of an event. That’s the kind of “works today, explodes later” coupling that makes systems rot.
2)
|
The summary offset semantics haven't changed, but documentation could be more verbose. Addressed these two points by moving the condensation semantics to the
Semantics of batch atomicity hasn't changed, just the way the code is structured. The API is a bit messy but that's what this PR is for.
The suggested fixes here are incompatible. The pipeline requires multiple linear passes if you actually want to separate the steps in code. We'd like a cleaner pipeline (and this PR improves the structure dramatically) but larger changes are beyond the scope of this PR.
Addressed.
Regressions are a concern, for sure, but the changes suggested here aren't a fix because they ignore the positional nature of summaries. Anecdotally I haven't noticed any performance degradation.
Says the model that wrote that line of code! (Also removed when addressing other issues).
This is standard and expected behavior (which is why the log level is |
Summary
While working on #1596 we came across a problem:
Viewobjects are limited to a single summary event. This limits our condensation abilities and adds some brittle corner-case logic in the condensers.The fix is to make it so views can have multiple summaries simultaneously.
This immediately introduces some problems:
Condensationevents because they don't have consistent IDs.So this PR deletes the specialized functions and adds a standard pattern for summary IDs. The logic for constructing the view is also simplified to process condensations one at a time instead of all at once, and some functions that were enforcing constraints on the view have their signatures reworked.
Additional changes include:
LLMSummarizingCondenserto handle the new view structure.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:4d6b5bf-pythonRun
All tags pushed for this build
About Multi-Architecture Support
4d6b5bf-python) is a multi-arch manifest supporting both amd64 and arm644d6b5bf-python-amd64) are also available if needed