Feat: Introduce class for SessionInitData rather than using a dict

[tofarr]

Introduce class for SessionConfig rather than using a dict

• Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

---

• SessionConfig is now specified more formally.
• We retain the existing serialization in init for backwards compatibility
• Config changes are now applied only to the current session rather than to the shared global config.

Open Questions

• I placed SessionConfig in the session module rather than the core/config module as it is specific to web and the session and different from the rest of the configuration. If it is more understandable to place this with the rest of the config I am open to doing this.

---

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:4294c29-nikolaik   --name openhands-app-4294c29   docker.all-hands.dev/all-hands-ai/openhands:4294c29

[enyst]

This PR is refactoring the existing dict, and it looks nice overall. However, it seems to conflict with what we tracked as the way forward here:
#3220
(please see comment)

For example, the PR defines duplicates of llm_model etc attributes making it more clearly disconnected from the underlying config-based configuration. I know this is the current behavior - this PR makes more clear! 😅, but I would appreciate if @neubig and @rbren could take a look at where this is heading, and we can figure out the way forward.

Related question: are you considering multiple session_configs per user?

[enyst]

Allow me to note another detail, related to the above, but not quite the same:

Currently, the UI settings offer to users only a handful of the settings the application actually has. To my knowledge, we are moving towards offering to users in the UI all or most of our settings. That will mean that this SessionConfig class would become a completely or almost completely duplicate hierarchy of our settings?

I'd appreciate to take the time to think this one over.

[neubig]

Hi! I don't have a huge amount of time to look at this today unfortunately...

But basically what I have been saying for a while is:

1. It'd be good to have a single source of truth for all settings, and that source of truth gets updated when we change the settings anywhere (by modifying things in the UI, or in config files, etc)
2. That should be well documented

I think it's definitely OK to have session-specific (or project-specific) settings as well that override global defaults.

[rbren]

seems like we're still mucking with the global defaults here

[enyst]

It's the current behavior though. 🤔 What do you think it should do?

[rbren]

Modifying global state with a single session would potentially create a ton of bugs and security issues, so I think the deepcopy we have above now makes this much safer. Will be much more important in a multi-Conversation world too (which we're working on now)

[enyst]

Right, I'm not thinking of it as global state, but it kinda is. It's the fallback LLMConfig/etc instance one gets if they didn't make / retrieve a specific instance, right?

If what we need here is a an LLMConfig / SecurityConfig instance per sid, could we make an LLMConfig / SecurityConfig instance per sid?

[rbren]

seems like we're still mucking with the global defaults here

[tofarr]

We do a deep copy of the global config when we create a session, so settings are not copied to the global shared object here.

[tofarr]

I did not envision this one resulting in so much discussion! Here is the rationale / use case as briefly as I can:

There are already 2 types of config:

• Global shared defaults derived from config.toml / environment applying to the whole server
• Session specific parameters from the Web UI, and apply only to the current user / session

Currently the session specific parameters are not formalized and represented by a dict. This PR really only formalizes them.

The justification for not simply using the existing config classes here is:

• These entities have distinct origins (Config file vs web UI)
• These entities have distinct scopes (A user session vs global)
• The SessionConfig is likely to only be a subset of the global config (But this is not guaranteed - it may end up containing attributes for which no default makes logical sense)

I doubt we will ever offer a full listing of everything in the config.toml in the web ui:

• Such a UI is likely to be very complex (and experienced users tend to prefer a config file for this level of detail anyway)
• There are technical concerns - Some values require a server restart to be effective.
• There are security concerns - Some values should NOT be shared between users.

If we later have config that makes sense in the context of a user rather than a session, I suggest we add such config at that time - in any case this is an interim step in that direction.

[enyst]

Those are good points! Please let me take a simpler part of it all, LLMConfig.

Currently, the UI only offers like three attributes, from all of it.

• there's no reason, is it, why we wouldn't let all the attrs of litellm completion to be customized by users (there's a bunch of historical posts that converge towards the intention to do it, too)
• because the UI offers the user the ability to override 3/n, the users experience the confusing issue linked above (and there are others like it), where they feel like modifying the toml should work and it doesn't. (it works for n-3 😅)
• there are multiple LLMs possible in the backend, but the UI only offers one; that may change - e.g. we have a draft llm that users can configure
• there is an LLM config per agent in the backend, and that... can be configured in the toml partially, and strange things may happen because the user HAS the ability to choose both stuff in the UI (an agent) and stuff in config.toml (agent llm) and the interference with the default LLM attributes... sorry, I don't remember what it breaks, but it breaks some expectations. 😅

What do think about a session-based configuration that starts like:

class SessionConfig
    llm_config: LLMConfig # main LLM
    draft_llm_config: LLMConfig # draft LLM
    ...

[tofarr]

What do think about a session-based configuration that starts like:

class SessionConfig
    llm_config: LLMConfig # main LLM
    draft_llm_config: LLMConfig # draft LLM
    ...

I think you may be on to something here - If I understand you correctly, you refer to the issue where values that are specified in the WebUI seem to ignore any value from the config.toml / ENV.

Maybe we do need to separate these out for Web - though the CLI and Headless modes still accept values from the config.toml at present AFAIK. Let me think a little more and update the proposal.

[rbren]

@enyst I agree we have a real problem with the config.toml + UI configuration

Right now, I'm mainly concerned about the 80% of users who are UI-only. Keeping a simple UI in place, and making sure those settings persist across sessions (and across different browsers connected to the same server), is the main objective we're going after right now.

I do think we'll need to figure out how to properly sync between config.toml and the UI. Tim's next PR (which will create long-term server-side storage for the LLM configuration) will hopefully set us up a little better here. By default it'll create a file on the user's machine that they can modify (though it'd be separate from config.toml)

[rbren]

Also

are you considering multiple session_configs per user?

Good question--probably not in the near term. I don't see much use in e.g. having 1 session going with GPT and another going with Claude, except for just mucking about and experimenting

[tofarr]

@enyst - the overall plan here is...

• This PR formalizes the SessionInitData from a dict to a dataclass
• The next PR will introduce server side storage for this so it is no longer in local storage. (In the OSS this will be a file store)

There will be no change to the current extensive config available to power users, aside from the documentation updates included in this PR

[enyst]

@rbren I see, and sure, I don't think this PR needs to solve our age-old problem there in a blink. 😅

@tofarr your point of view is most interesting: you're describing how things look like from the perspective of building the web ui / remote app. It's instructive.

I do want to note that, of course the distinction between user configuration and server/global configuration is real. And we're not currently making it anywhere really. We should. But I don't think it lies in "what happens to be available in the Web client today" vs "what is in files". That can't be right, there are other clients than the web client, there are other user-specific or session-specific settings. I would even argue that there is such thing as CLI sessions, although not formalized.

Re:

Tim's next PR (which will create long-term server-side storage for the LLM configuration) will hopefully set us up a little better here. By default it'll create a file on the user's machine that they can modify (though it'd be separate from config.toml)

For the record, we have the ability to define the location of the config.toml intended per user (e.g. can drop it in ~/config.toml), coming from previous chat, configurable location PR.

This was intended as a step for multi-user use, albeit a different one.

[enyst]

Maybe we can now remove ConfigType. Last I checked, it was only used here.

[enyst]

This means we need one AppConfig per sid, with its other stuff. For other clients, when we need a specific instance of a config class, the config module is able to make them, under predefined names (like draft_llm_config) or user-defined names (like these).

Maybe we can take that into account for these config instances? I don't think it needs to be in this PR.

[tofarr]

The reason I added this in here was the idea of writing back into the global config from the web input is really scary!
For example this line (I preserved the existing logic):
default_llm_config.api_key = session_init_data.llm_api_key or default_llm_config.api_key

Before the deep copy, this would write a user's api_key back into the global config. Another user could come along and call init without an API key (They would need to modify the client side code) and boom - they use an API key owned by the last person who entered one! (This was actually the primary motivation behind this PR.)

[tofarr]

This had me 😱 when I spotted it!

[enyst]

Argh. By the way, this is why these methods exist. Their only reason for existence is to be used by the UI to get defaults - actual defaults, dataclass values only, not from config, not from env, not from, uh, previous values.

Currently, the UI isn't using them, so nothing is using them. They're just sitting there and be pretty. 😅