feat(llm): switch model profile on user message#2192
Conversation
API breakage checks (Griffe)Result: Passed |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean, pragmatic implementation
What works well:
- Simple command syntax that solves a real user need
- Registry caching pattern (checks cache before disk I/O) - previous performance concern resolved ✅
- Clear separation: parse → handle → switch
- Tests verify actual behavior, not mocks
- No breaking changes (additive only)
Linus would say: "This is how you do it. Straightforward data flow, no special cases, solves the actual problem."
LGTM! 🚀
48a1809 to
0e254ea
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean, pragmatic implementation
What works well:
- Simple command syntax that solves a real user need
- Registry caching pattern (checks cache before disk I/O) - previous performance concern resolved ✅
- Clear separation: parse → handle → switch
- Tests verify actual behavior, not mocks
- No breaking changes (additive only)
Linus would say: "This is how you do it. Straightforward data flow, no special cases, solves the actual problem."
LGTM! 🚀
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
enyst
left a comment
There was a problem hiding this comment.
Thank you for this!
I think plugging this behavior in the conversation may be easy enough to do (and undo), so IMHO that is fine.
I think maybe we don’t want to send this information to the LLM, it’s just a toggle for the user, isn’t it?
The most important things was to do a first try to understand better the problem. I believe i can make it better :) I will adress you suggestion ;) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid architecture with a UX gap
What works well:
- Clean separation of concerns (
SwitchModelHandleris standalone and focused) - Registry caching pattern prevents redundant disk I/O (previous concern resolved ✅)
- Tests verify actual behavior with real code paths (no mock spam)
- Opt-in feature with no breaking changes
- Solves a real user need pragmatically
Key issue:
User feedback disappears into logs instead of being shown in the conversation. See inline comments.
Linus would say: "The data flow is clean and the code does what it should, but users are left in the dark. When someone types /model, they expect to see something back. Logger output is not user output."
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/conversation/switch_model_handler.py
Outdated
Show resolved
Hide resolved
|
Hey @enyst I’ve made some changes based on your feedback. The goal was to pollute the Additionally, I've addressed your comments and added an example (give it a try, it’s fun!). I also added a flag to enable or disable model switching. Let me know what you think! |
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
openhands-sdk/openhands/sdk/conversation/switch_model_handler.py
Outdated
Show resolved
Hide resolved
Thank you! Left a few tiny comments I think maybe it doesn't get saved, so it won't be restored, the initial profile is? As in, restoring a conversation from the saved Just a heads up, I won't be around for a bit, back later (some number of hours) |
So I ran a small experiment to see what is happening. If we save a convo and we restore it later: the last used LLM will be used. Which makes sense, right? |
|
[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. |
@enyst :-) |
neubig
left a comment
There was a problem hiding this comment.
This basically looks good to me! Thanks for implementing this.
openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
Outdated
Show resolved
Hide resolved
|
cc @enyst if you still think changes should be made could you list up the changes that need to be made, otherwise approve? |
There was a problem hiding this comment.
Could we maybe do it like this:
- SDK does the profile switch
- SDK exposes a little API for it, e.g. maybe
LLMRegistry.switch_profile()or evenconversation.switch_profileif we need to place it into conversation (it does a little much though)
- SDK exposes a little API for it, e.g. maybe
- CLI owns user interaction
- parses /model as command, calls the method to do stuff
WDYT?
|
Maybe like this separation: #2336 (comment) |
Agent server REST API breakage checks (OpenAPI)Result: Failed Log excerpt (first 1000 characters) |
|
I have rewritten the code to follow the comments. Now it is truly minimalistic, and the responsibility for catching the command is left to the CLI. Note that I used model_copy twice. I'm forced to do so because the |
Co-authored-by: OpenHands Bot <contact@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean, pragmatic implementation
Linus would say: "This is exactly how you do it. Straightforward data flow, solves the actual problem, no special cases."
What works well:
- Simple, focused API that solves a real user need (model switching mid-conversation)
- Registry caching pattern eliminates redundant disk I/O ✅
- Duplicate registration fix in
_ensure_agent_ready()prevents potential issues ✅ - Tests verify actual behavior with real code paths, not mock spam ✅
- No breaking changes (additive only) ✅
Minor observation: Registry operations happen outside the state lock in switch_profile() but inside the lock in _ensure_agent_ready(). This inconsistency is probably fine if the registry is internally thread-safe, but worth noting for future maintainers. Not blocking.
LGTM! 🚀
| for llm in list(self.agent.get_all_llms()): | ||
| self.llm_registry.add(llm) | ||
| if llm.usage_id not in registered: | ||
| self.llm_registry.add(llm) |
There was a problem hiding this comment.
Sorry, not about this PR, but this looks curious actually. Are we saying that the agent may have gotten an LLM instance that wasn't registered? Do we know that?
| except KeyError: | ||
| new_llm = self._profile_store.load(profile_name) | ||
| new_llm = new_llm.model_copy(update={"usage_id": usage_id}) | ||
| self.llm_registry.add(new_llm) |
There was a problem hiding this comment.
Really only a tiny thought, this code could be out of here and in the registry I think
Not for this PR, just thinking of responsibilities; now the Conversation knows both llm registry and llm profile store and does stuff between them, even though those two are so related they could arguably be the same. Maybe worth thinking about it later, sorry
enyst
left a comment
There was a problem hiding this comment.
I'm happy with this solution because it solves a real problem and it's indeed minimal for what it does, thank you!
|
On a side note, as you have seen, I am slowly growing a bit concerned with how we are doing things 😄 If we want to change the underlying design of the codebase on the current statelessness aspect, we can, of course we can, we just could maybe give it some good thought and submit it for discussion. I do think maybe we shouldn't do a lot of "sins" or I know my slow brain is guaranteed to lose track 😢 ... and I also know others may keep theirs for longer, but last I checked we're humans here so... idk if forever! 😹 |

Summary
SDK side of the
/model <MODEL_NAME> [prompt]command (ref #2018 )The SDK exposes a method
conversation.switch_profile()to switch LLM, which client applications can use to implement commands like, e.g./model <MODEL_NAME>: Switches the current LLM profile to the one specified.Note:
LocalConversationclass clean.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.13-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:63649ee-pythonRun
All tags pushed for this build
About Multi-Architecture Support
63649ee-python) is a multi-arch manifest supporting both amd64 and arm6463649ee-python-amd64) are also available if needed