[Config] Reduce the duplication in run_controller argument

[xingyaoww]

What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?

A general rule (after discussion with @neubig in #3184) is that we try to minimize the overlap between different input arguments.

For example, in run_controller, now we have two things:

• max_budget_per_task (higher priority), which overlaps with config.max_budget_per_task
• max_iterations (higher priority), which overlaps with config.max_iterations

---

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

This PR tries to get rid of this overlap and remove the two extra input arguments to make run_controller a relatively simpler interface.

TODO for future PRs (as discussed with @neubig), we should break the gigantic AppConfig into several composable things (e.g., agent_config, llm_config, sandbox_config), which is used to separately initialize different components without relying on the same AppConfig.

---

Other references

[enyst]

Nice, removing them is cleaner! This continues what we did by removing such individual attributes from LLM class for example, once we had a usable llm_config to send them in. (same reason I think condensable=True also belongs in llm_config, and not a new param, just when we got rid of the others 😅)

we should break the gigantic AppConfig into several composable things (e.g., agent_config, llm_config, sandbox_config), which is used to separately initialize different components without relying on the same AppConfig.

Please tell if I don't understand this correctly, AppConfig is already composed of those things, we have moved vars that used to be defined in the app_config into these smaller ones, and continued to split some more in time. Do you mean we should add more "buckets of settings" like them, such as a TaskConfig for iterations and the like? This is IMHO better, as best we can where they make sense, and it's the direction the code has been heading to. It would make things more readable, at least.

Just a question on a particular bit of this PR code though.

[xingyaoww]

Please tell if I don't understand this correctly, AppConfig is already composed of those things, we have moved vars that used to be defined in the app_config into these smaller ones, and continued to split some more in time. Do you mean we should add more "buckets of settings" like them, such as a TaskConfig for iterations and the like? This is IMHO better, as best we can where they make sense, and it's the direction the code has been heading to. It would make things more readable, at least.

Correct me if @neubig think differently: Yes, if it make sense, I think at some point we can make AppConfig solely consists of a few config like LLMConfig, AgentConfig (or even making LLMConfig a part of AgentConfig), RuntimeConfig (the current SandboxConfig). So that it is possible for us to use them separately:

config = AppConfig(
    task_config=task_config,
    agent_config=agent_config,
    runtime_config=agent_config
)

runtime = Runtime.from_config(runtime_config)
agent = Agent.from_config(agent_config)

state = run_controller(
    task=instruction,
    task_config=task_config,
    agent=agent,
    runtime=runtime
)
# OR use them altogether
state = run_controller(
    task=instruction,
    config=app_config
)

While config=app_config is sufficient for our OpenDevin web server, breaking this config into smaller components probably allows our components to be more reusable.

For example, people can just create a Runtime from RuntimeConfig separately (and only use it for their research purpose), without the need to create and understand how AppConfig works (we currently have one parameter in AppConfig that stay in the way of this..).

[enyst]

This looks a bit strange to me. For most of the time, we really have treated the application configuration as immutable, or almost, even though it isn't. I think working that way had definite benefits, it makes it more understandable, and the user should be able to count on the values they set, once fully loaded and initialized, in the right order of precedence.

If we then change this at runtime, IMO it's harder for the users to understand what value came from where or why.

I wrote some more about this, but unfortunately I... lost the contents of that post. 😅 Anyway I guess that since we no longer let the core parse the command line args for the client scripts, the scripts have to do that. I wonder if we can do it better though, at some point (doesn't have to be right now).

[xingyaoww]

Yeah, I think creating an immutable config definitely has its benefits! But I think it could be slightly tricky to do now bc we might need to reload config from env var, config.toml etc.

I'm working on an evaluation refactor (should be done in the next couple days) for the new runtime, where I'll go through every config in the evaluation and initialize the whole config all at once (is this more aligned with what you think?):

https://github.com/OpenDevin/OpenDevin/blob/c3d6320f381c93e032598ea54bbeddfac4e83ac2/evaluation/swe_bench/run_infer.py#L117-L144

[enyst]

That looks very cool, explicit and self-contained! And no other place in SWE-bench sneaking in some other value in it, right? 😅

I think that's exactly on point, a single place per external script to do their config initialization looks just right. ❤️

[xingyaoww]

And no other place in SWE-bench sneaking in some other value in it, right? 😅

Yep! "sneaking in some other value" has caused me a lot of trouble a while back 😿 definitely want to get rid of that behavior

[enyst]

Hey arguably, this line is sneaky too. 🤣 We can live with it since we're killing it again!

[xingyaoww]

Kill it in 2-5 days 🦾

[enyst]

AppConfig solely consist of a few configs

Just a quick note on this, I kinda doubt we will succeed to make AppConfig only from a few sub-configs, it's really big and some don't mesh together with anything really ("disable_color"? 😆), and if we force them into something it might become less understandable. I could be wrong. IMHO it will be fine if we continue as we do, pick and put stuff together when it becomes obvious it needs it and/or makes too much sense. 😆

[xingyaoww]

@enyst totally agree! not trying to do that to the fullest extent - but trying to have that in mind as the final goal regardless of whether we can eventually achieve it 😆

[neubig]

Just chiming in to agree!