Conversation
API breakage checks (Griffe)Result: Passed |
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable
Core design is clean and pragmatic, but this PR falls into eval-risk territory and has one style issue to address.
🔴 Eval Risk - Human Review Required
This PR modifies condenser/memory behavior, which can affect benchmark performance. Per repo policy, changes to memory/condenser behavior require human maintainer review + lightweight evals before approval.
Recommendation: Flag for maintainer review and consider running lightweight evals to verify no regression in agent performance.
Key Design Strengths
✅ Clean separation: Config (dict) in definition, object at factory time - mirrors the model: inherit pattern
✅ Smart validation: Keys validated at parse time (clear errors), values validated by Pydantic (no duplication)
✅ Strong tests: Real behavior tested, sync test prevents drift with LLMSummarizingCondenser.model_fields
✅ No breaking changes: Additive feature with sensible defaults
See inline comments for the style issue.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Clean design, well-tested, pragmatic approach.
Eval-Risk Confirmation: This modifies condenser/memory behavior. Per repo policy, requires human maintainer review + lightweight evals before merge.
Code Quality: Solid implementation with comprehensive test coverage. The design choice to store config as dict[str, Any] in AgentDefinition and construct the condenser at factory time mirrors the model: inherit pattern and is a reasonable approach. The sync test guarding against drift between _CONDENSER_VALID_KEYS and LLMSummarizingCondenser is a pragmatic safeguard.
No blocking issues found. ✅ Code is mergeable once eval verification confirms no benchmark regressions.
| from openhands.sdk.context.condenser import LLMSummarizingCondenser | ||
|
|
||
| condenser = LLMSummarizingCondenser( | ||
| llm=llm.model_copy(update={"usage_id": "condenser"}), |
There was a problem hiding this comment.
I do worry a little bit that the more of these we do the more we'll have to sort out... what if we sent a profile name, or think about how this could be like to replace these LLMs with profiles references if that makes sense?
Though the condenser is a pretty obvious place, maybe not exactly to worry much about
There was a problem hiding this comment.
ah solution would be to save a condenser LLM profile that we use just for that and we go on the default one if there is no one.
There was a problem hiding this comment.
Yes! That sounds great to me, or even, if possible, read the condenser profile at the start of the conversation, if none initialize one from the other
There was a problem hiding this comment.
but I think this should be addressed in another PR, right?
There was a problem hiding this comment.
I don't know, the more model_copy(update...) we add to the codebase, the more we add tech debt and need to fix it
But I also see the opposite argument here. I'm just starting to feel we're adding too many, though I could be wrong
|
[Automatic Post]: It has been a while since there was any activity on this PR. @VascoSch92, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
Summary
(ref #2186 )
Allow subagents defined via Markdown files to declare a condenser configuration. The condenser LLM is inherited from the agent's own LLM at factory time — the definition only stores the tuning parameters (e.g., max_size, keep_first).
Changes
openhands-sdk/openhands/sdk/subagent/schema.py
openhands-sdk/openhands/sdk/subagent/registry.py
tests/sdk/subagent/test_subagent_schema.py
matches LLMSummarizingCondenser.model_fields to prevent drift
Usage
Design decisions
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:37c2787-pythonRun
All tags pushed for this build
About Multi-Architecture Support
37c2787-python) is a multi-arch manifest supporting both amd64 and arm6437c2787-python-amd64) are also available if needed