Skip to content

refactor(llm): add LiteLLM-backed provider abstraction#2363

Draft
enyst wants to merge 8 commits intomainfrom
openhands/llm-provider-abstraction
Draft

refactor(llm): add LiteLLM-backed provider abstraction#2363
enyst wants to merge 8 commits intomainfrom
openhands/llm-provider-abstraction

Conversation

@enyst
Copy link
Collaborator

@enyst enyst commented Mar 9, 2026

Summary

Refactor SDK LLM provider handling around an internal LLMProvider helper backed by LiteLLM, so provider-specific logic stops re-splitting raw provider/model strings across the SDK.

This PR now takes the simpler direction discussed in issue #2274 and PR review:

  • accept the full model string at the SDK boundary
  • parse it once with LiteLLM
  • use LiteLLM's parsed provider + model view for LiteLLM-facing runtime paths
  • initialize LiteLLM transport provider parsing once per LLM instance instead of refreshing provider state during each transport call

Concretely, this PR:

  • adds openhands.sdk.llm.utils.litellm_provider.LLMProvider as a thin wrapper around LiteLLM provider parsing, provider model info lookup, API base inference, and call kwargs generation
  • keeps LLMProvider focused on LiteLLM's parsed runtime shape (provider, parsed model, resolved API base) rather than storing duplicate requested_* fields
  • updates LLM chat/responses transport calls to use provider-owned LiteLLM kwargs generation (model + custom_llm_provider, plus Bedrock-aware api_key forwarding) instead of repeatedly passing or re-splitting the full string
  • moves Bedrock-specific LiteLLM api_key handling into LLMProvider so provider-sensitive transport behavior lives alongside provider parsing
  • updates telemetry cost calculation to reuse the same parsed provider/model view
  • keeps capability and model_features lookups on the canonical model string (model_canonical_name or model) instead of introducing a second provider abstraction
  • initializes transport provider parsing once during LLM setup, so chat/responses transport paths reuse the same resolved provider metadata for the lifetime of the instance
  • removes dead provider inference wrappers that became redundant after the simplification
  • adds focused tests for the slimmer helper and the updated transport/canonical capability lookup behavior

Closes #2274.

Checklist

  • If the PR is changing/adding functionality, are there tests to reflect this?
  • If there is an example, have you run the example to make sure that it works? (N/A: no example changes)
  • If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
  • If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name? (N/A: internal refactor)
  • Is the github CI passing? (pending)

Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.13-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:2d5b6e4-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-2d5b6e4-python \
  ghcr.io/openhands/agent-server:2d5b6e4-python

All tags pushed for this build

ghcr.io/openhands/agent-server:2d5b6e4-golang-amd64
ghcr.io/openhands/agent-server:2d5b6e4-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:2d5b6e4-golang-arm64
ghcr.io/openhands/agent-server:2d5b6e4-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:2d5b6e4-java-amd64
ghcr.io/openhands/agent-server:2d5b6e4-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:2d5b6e4-java-arm64
ghcr.io/openhands/agent-server:2d5b6e4-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:2d5b6e4-python-amd64
ghcr.io/openhands/agent-server:2d5b6e4-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-amd64
ghcr.io/openhands/agent-server:2d5b6e4-python-arm64
ghcr.io/openhands/agent-server:2d5b6e4-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-arm64
ghcr.io/openhands/agent-server:2d5b6e4-golang
ghcr.io/openhands/agent-server:2d5b6e4-java
ghcr.io/openhands/agent-server:2d5b6e4-python

About Multi-Architecture Support

  • Each variant tag (e.g., 2d5b6e4-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 2d5b6e4-python-amd64) are also available if needed

Introduce an internal LLMProvider helper that wraps LiteLLM provider parsing,
provider model info lookup, and provider-aware API base inference.

Use the helper in LLM and Telemetry so Bedrock auth handling and
cost-calculation model/provider routing stop manually splitting model
strings. Extend model_features to accept an LLMProvider so provider-aware
rules can operate on parsed provider/model data while keeping the raw model
string available.

Add focused tests for the new helper and the updated feature lookup paths.

Co-authored-by: openhands <openhands@all-hands.dev>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Python API breakage checks — ✅ PASSED

Result:PASSED

Action log

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

REST API breakage checks (OpenAPI) — ✅ PASSED

Result:PASSED

Action log

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2026

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/llm
   llm.py4747783%440, 493, 727, 833, 835–836, 864, 912, 923–925, 929–933, 941–943, 953–955, 958–959, 963, 965–966, 968, 1152–1153, 1350–1351, 1360, 1373, 1375–1380, 1382–1399, 1402–1406, 1408–1409, 1415–1424, 1479, 1481
openhands-sdk/openhands/sdk/llm/utils
   litellm_provider.py791186%59–60, 71–72, 109–110, 115–118, 122
   model_features.py40197%32
   telemetry.py1751392%133, 158, 164–165, 175, 187–188, 227, 347, 349, 363, 369, 374
TOTAL20335588971% 

Rework the new LLMProvider helper so it no longer keeps both raw and
parsed model strings internally.

Accept full model strings at the SDK boundary, then immediately normalize to
LiteLLM's parsed provider/model pair for transport, telemetry, and feature
checks. Unknown models still remain opaque strings, but the provider
abstraction itself now avoids duplicate raw-vs-parsed state.

Also route chat/responses calls through parsed provider kwargs and tighten the
associated tests.

Co-authored-by: openhands <openhands@all-hands.dev>
Keep LLMProvider for LiteLLM-facing transport logic only and use the canonical model string for capability/feature lookups.

Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
@enyst
Copy link
Collaborator Author

enyst commented Mar 18, 2026

@OpenHands Babysit this PR, monitor CI, fix until green.

@openhands-ai
Copy link

openhands-ai bot commented Mar 18, 2026

I'm on it! enyst can track my progress at all-hands.dev

@openhands-ai
Copy link

openhands-ai bot commented Mar 18, 2026

Final summary:

  • Checked out context for PR refactor(llm): add LiteLLM-backed provider abstraction #2363 on branch openhands/llm-provider-abstraction
  • Verified the PR status and CI results using GitHub tooling
  • Confirmed all checks are green: 27 successful, 1 skipped, 0 failing, 0 pending
  • Confirmed the review-thread gate is also passing
  • Determined no code changes were needed, because there was nothing failing to fix

Conciseness:

  • No repository files were changed
  • No extraneous changes were introduced
  • No commit or push was necessary

Current state:

PR link: #2363

@enyst
Copy link
Collaborator Author

enyst commented Mar 19, 2026

@OpenHands /codereview-roasted

@openhands-ai
Copy link

openhands-ai bot commented Mar 19, 2026

I'm on it! enyst can track my progress at all-hands.dev

Copy link
Collaborator Author

@enyst enyst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Needs improvement

[CRITICAL ISSUES]

  • [openhands-sdk/openhands/sdk/llm/llm.py, Lines 495-499, 893-899, 1049-1056] Breaking change / duplicate state: _provider_info is cached once during initialization and then reused for every transport call. That would be fine if LLM were actually immutable, but it isn't: model_copy(update=...) shallow-copies private attrs and does not rerun validators. So a copy with an updated model or base_url now keeps stale provider metadata. I verified this locally: copying a gpt-4o LLM with model_copy(update={"model": "ollama/llama2"}) leaves _provider_info as openai/gpt-4o, so transport kwargs are wrong. The old code re-inferred provider from self.model/self.base_url on each call, so this is a real regression introduced by the refactor.

[TESTING GAPS]

  • [tests/sdk/llm/test_llm.py, Lines 419-430] Missing regression test: the new tests only prove _provider_info is initialized on construction. Add coverage for model_copy(update={"model": ...}) and model_copy(update={"base_url": ...}) (or make those operations impossible). Right now the exact stale-cache failure mode above isn't exercised at all.

VERDICT:Needs rework

KEY INSIGHT: this refactor adds a second source of truth for provider routing, and because LLM instances are still copyable/mutable, that cached state can drift away from the public model/base_url fields.

Leaving this as a comment review rather than approval because this touches LLM runtime behavior and could affect eval-facing execution paths.

@openhands-ai

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactoring Proposal: Large Classes Identified for Decomposition

2 participants