Add loading from toml for condensers

[enyst]

• This change is worth documenting at https://docs.all-hands.dev/
• Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

End-user friendly description of the problem this fixes or functionality that this introduces.

---

Give a summary of what the PR does, explaining any non-trivial design decisions.

---

Link of any specific issues this addresses.

---

To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:f0df672-nikolaik   --name openhands-app-f0df672   docker.all-hands.dev/all-hands-ai/openhands:f0df672

[enyst]

@OpenHands Read all the review comment threads on this PR using GitHub API or graphQL.

Take a look at the config module and make sure you understand the similitudes and differences. Take a look at the unit tests for config, to understand the expected behavior. Then lets fix the review thread comments.

We will need to unit test every behavior here. Condenser config in toml is new, and so it doesn't have tests yet.

I prefer config tests to be in test_config with the others, but make sure they all have condenser in their name.

[enyst]

(openhands-ai)
I have fixed the failing test test_condenser_config_from_toml_with_missing_llm_reference by ensuring that when a missing LLM reference is encountered, we create a default LLMConfig instance instead of falling back to NoOpCondenserConfig.

The issue was that when a condenser config specified an LLM reference that didn't exist, we were not properly creating a default LLMConfig, which caused the test to fail because it expected a LLMSummarizingCondenserConfig but got a NoOpCondenserConfig instead.

All tests are now passing.

[enyst]

(openhands-ai)
I analyzed the code to answer your questions:

1. Yes, the TOML configuration works as described in your comment. You can define an LLM in the [llm] section and reference it from an agent configuration in the [agent] section using the llm_config field.

2. The implementation differs between agents and condensers:

   • For agents: The AgentConfig class stores the LLM config name as a string in its llm_config field. When an agent needs its LLM config, AppConfig.get_llm_config_from_agent() is called to look up the actual LLM config by name.
   • For condensers: The current PR implementation passes the entire dictionary of LLM configs to the from_toml_section() function and resolves the references immediately during parsing.

3. Yes, we can simplify the condenser implementation to match the agent implementation:

   • Instead of passing all LLMs to from_toml_section() and resolving references immediately, we could store the LLM config name (like agents do) and resolve it later when needed.
   • This would make the implementation more consistent and avoid passing the entire dictionary of LLM configs.
   • The change would involve:
     1. Modifying LLMSummarizingCondenserConfig and LLMAttentionCondenserConfig to store an LLM config name instead of the actual LLM config
     2. Adding a method to resolve the LLM config name when needed
     3. Updating from_toml_section() to not require the LLM configs parameter

This approach would be more consistent with how agents handle LLM configs and would simplify the implementation.

[enyst]

(openhands-ai)
I have implemented automatic assignment of the condenser configuration to the default agent configuration. Now, when a condenser configuration is loaded from TOML, it is automatically assigned to the default agent configuration, making it accessible through the AppConfig → AgentConfig → condenser chain.

I have also updated the tests to verify that the condenser configuration is correctly assigned to the default agent configuration. The comment "We don't have a direct way to access the condenser config from AppConfig" has been updated to reflect this change.

[csmith49]

@enyst Looks like all the functionality is there and the tests are passing, is this ready for a review?

[enyst]

@csmith49 Just testing locally! Last night I played with Sonnet 3.7 and we figured out some things, but I also felt I don't know exactly how we want it all to behave, so I was going to ask you about it.

Remarks:

• a case like this works fine

[condenser]
type="llm"
llm_config="or-gpt-4o"
keep_first=1
max_size=5

• the summarizing condenser seems to replace state.history (maybe it's just me missing something, I thought it wasn't supposed to?)

• Sonnet's description of behavior on this PR sounds good to me:

1. The condenser config is loaded from the [condenser] section
2. The llm_config attribute references a pre-existing LLM config, with fallback to default
3. The loaded condenser config gets assigned to the default agent configuration

[csmith49]

[condenser]
type="llm"
llm_config="or-gpt-4o"
keep_first=1
max_size=5

• Sonnet's description of behavior on this PR sounds good to me:

1. The condenser config is loaded from the [condenser] section
2. The llm_config attribute references a pre-existing LLM config, with fallback to default
3. The loaded condenser config gets assigned to the default agent configuration

I took a look through the unit tests and they all seem to capture the least-surprising behavior wrt fallbacks and assignments. If they're passing I'd say the description is accurate and the fallbacks work like we want them to.

• the summarizing condenser seems to replace state.history (maybe it's just me missing something, I thought it wasn't supposed to?)

That shouldn't be happening, the only thing modifying state.history should be the event stream and agent controller. Can you give me an example of what you saw?

[enyst]

I'm trying to replicate that strange issue, it doesn't happen right after the summarizing condenser, but I see two things:

• first, if there's an exception in the LLM call in condenser, we create an AgentCondensationObservation with content=None, that gives an error when the code attempts truncate_content on it - IMHO we could maybe not create any obs in that case? Or an empty or default content, or error obs.
• second, I think I just replicated the loss of history and it looks like a bug in the stream or controller, looking into it, but it's not the condenser.

[enyst]

• second, I think I just replicated the loss of history and it looks like a bug in the stream or controller, looking into it, but it's not the condenser.

OK I sort of got it: my session was missing event 1 (file 1.json in the local file store), for a reason I can't tell. So when restoring the session, it was restoring only event 0 from 55 events, adding the default message, so 2 events in state.history, which happened to be what the condenser made (from max_size=5 to 2 or 3). Sorry for the noise!

Back to this PR: looks good to me, I am also testing more configurations.

[enyst]

I'd note: I did have some second thoughts on not implementing custom sections, e.g behavior like

• the user defines [condenser] and [condenser.summary-prompt-with-xml], [condenser.summary-prompt-simple]
• and a command line arg to pick one for a run / script / evaluation.

Not possible now, just a [condenser] section can be defined.

If you feel that would be useful, we can do it, whether here or in a follow-up.