https://docs.all-hands.dev/ logo
Join Slack
Powered by
Can someone review this PR that adds ACP to GUI <h...
# proj-agent
s
Can someone review this PR that adds ACP to GUI https://github.com/OpenHands/OpenHands/pull/14227 @limited-student-10747 @millions-afternoon-47582 Thanks!
馃憖 1
l
I'm taking a look now. Why can't ACP agents use LLM profiles?
I thought that ACP agents would have an "LLM" object that they use to specify which LLM they connect to.
I guess right now we need to specify the LLM for ACP agents using a different mechanism? I guess we could do that for now and improve later.
I'm looking at software-agent-sdk to see if we can simplify the interface a little bit and require less case-by-casing when switching between the agents, will get back shortly
Hey @strong-ice-84145, in an attempt to simplify the eventual downstream interface so we don't need different routes for ACP and non-ACP calls, here's a proposed refactoring of software-agent-sdk: https://docs.google.com/document/d/1Z-G2soBsC7IPB8TXm0cqJaIJRw_csBapqiyVPogdYW4/edit?usp=sharing I have an agent working on it now, but tell me what you think or if you have any concerns. The TL;DR is that we could move in the direction of subclassing so that we can have a single route that decides which agent to use based on the content of "settings".
Here's the draft PR, still iterating a little bit but please tell me what you think: feat(agent-server): unify ACP conversation endpoint by neubig 路 Pull Request #3227 路 OpenHands/software-agent-sdk
馃憖 1
s
> I thought that ACP agents would have an "LLM" object that they use to specify which LLM they connect to. ACP accepts a free-form string to select an llm. Then it can authenticate either via subscription or via secrets for authentication. But the llm object that is attached to the acp.agent is not actually responsible for llm inference at all. So not sure if using llm profile makes sense, but will investigate further. I guess we could have llm profiles but then it would be a parallel implementation of the regular openhands llm profile
I am reviewing the design/PR!
ACP/model choice is not totally decoupled. The litellm proxy allows any combination but for a "regular user" you can't select gpt for claude code. And that's totally impossible if authentication is done via subscription. Still possible to do conditional llm profiles given the acp harness I guess! Will make an issue and tackle in another PR
l
One possibility for a future PR but not this one would be a translation layer that is:
Copy code
def get_acp_string(profile: LLMProfile, agent_settings: AgentSettings) -> str:
which will return the appropriate string to configure the ACP agent given an LLM profile, or throw an error if it is incompatible
s
left a small comment: you collapsed ConversationInfo into a single type in your first commits, it seems that it would make the code simpler if we could ship that PR with only one class?
馃憖 1
l
s
thanks, approved!
l
OK, this is in, so let's try to structure your PR around this new paradigm (if more convenient, you could do an SDK release first). Also, if it'd be easier to work in agent-canvas than OpenHands/OpenHands you could also try to implement the new settings in agent-canvas instead (not sure which is easier, since you already have a PR open for OpenHands/OpenHands, but agent-canvas is a simpler repo overall)
s
Updated the PR https://github.com/OpenHands/OpenHands/pull/14401 Waiting for a release to remove specific SHA, working on adding it to canvas!
5 Views
Open in Slack
PreviousNext
Powered by