Repository navigation
fix: enable the OpenHands SDK default condenser - #383
mnajafian-nv wants to merge 7 commits into
Conversation
The OpenHands SDK leaves Agent.condenser unset, and the adapter built its agent without one. Long runs therefore resent the full event history on every model request, and a context-window error from the model failed the invocation instead of triggering condensation. Configure the SDK default_condenser, the LLMSummarizingCondenser sizing (80 events, keep the first 4) that the OpenHands default agent uses. Pass the agent LLM object itself rather than a copy with a separate usage ID, so summarization calls accrue to the metrics the adapter reports as invocation usage. This changes default runtime behavior for OpenHands runs. The adapter descriptor declares no settings, so the condenser is not configurable. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The OpenHands adapter now summarizes older conversation history. Document when condensation triggers, that summarization calls count toward invocation usage, and that NeMo Fabric does not expose settings to disable or tune it. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Condensing history by default adds summarization model calls and can change results for an unchanged configuration. Give users who need the previous behavior, for example to compare against earlier runs, an explicit opt-out through the adapter's existing settings schema rather than a new configuration surface. harness.settings.condenser accepts default, the OpenHands SDK default condenser, or none, which builds the agent without a condenser as before. Planning rejects any other value. Planning does not apply schema defaults, so the adapter treats a missing value as default. Regenerate the adapter catalog for the descriptor change and document the setting in the adapter README, the integration guide, and the adapter configuration matrix. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The adapter enabled the condenser for every condenser value other than "none". Planning rejects other values, but a host that starts the adapter without planning could pass "off" or "None" and silently get summarization calls, which cost model usage. Accept only "default" and "none" in the adapter and raise openhands_condenser_invalid for anything else, matching how an existing adapter re-validates its enum settings. Add a test for unknown string and null values. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Each condensation returns from an OpenHands agent step, and the SDK counts every step toward max_iteration_per_run, which the adapter sets from runtime.max_turns. A run with a tight max_turns that used to finish can now stop at the iteration limit, so say so. Also state that the trigger is the history sent to the model rather than the total event count, replace the sentence that contradicted the condenser setting with "does not expose condenser tuning parameters", mention that other setting values are rejected, and wrap the new paragraphs like the surrounding text. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The condenser tests only checked how start() builds the agent, so nothing showed that a run condenses its history, keeps going, and reports the summarization call in invocation usage. Drive the adapter with OpenHands SDK 1.50.0 and a stubbed LiteLLM transport: once past the 80-event limit, and once after a context window error. Each case asserts one Condensation event, a successful finish, and usage that includes the summarization call. The test skips when the OpenHands SDK is not installed. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThe OpenHands adapter now supports an enabled-by-default history condenser. The ChangesOpenHands condenser
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant HarnessConfig
participant OpenHandsAdapter
participant OpenHandsAgent
participant AgentLLM
OpenHandsAdapter->>HarnessConfig: Read condenser setting
OpenHandsAdapter->>OpenHandsAdapter: Create default_condenser with agent LLM when enabled
OpenHandsAdapter->>OpenHandsAgent: Attach condenser during startup
OpenHandsAgent->>AgentLLM: Summarize history during condensation
Merge Risk: ⚪ Minimal · up to The condenser change is mergeable after normal checks; no actionable merge-blocking issue remains. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (6 skipped: 6… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title follows Conventional Commits format with the allowed lowercase type "fix", uses an imperative summary, stays under 72 characters, has no trailing period, and accurately describes the change. Description check The description includes the required overview, reviewer starting point, related-issues section with an allowed action keyword, and both contribution confirmation checkboxes. It also provides implemen…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (6 skipped: 6 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
-
-
🧪 Generate unit tests (beta)
-
-
-
- Create a new PR
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Fern docs preview: https://nvidia-preview-pull-request-383.docs.buildwithfern.com/nemo/fabric |
This branch was successfully deployed
1 participant
Overview
Enable the OpenHands SDK default condenser. Without it, long runs resend their full history and can fail at the model context limit. Runs now summarize older events by default;
harness.settings.condenser: nonekeeps the previous behavior. Summarization adds to reported model usage and consumes amax_turnsiteration. No dependency change.Details
default/nonesetting; regenerate the adapter catalog.Validation
Where should the reviewer start?
Review
start()and_condenser_enabled()inadapters/python/openhands/src/nemo_fabric_adapters/openhands/adapter.py, then the two SDK-backed cases intests/adapters/test_openhands.py.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to: none
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
Summary by CodeRabbit
condensertononeto preserve the full conversation history. Supported values aredefaultandnone.