Skip to content

Commit 218728e

Browse files
committed
Refine LLM profile persistence
- move inline/profile compaction into LLM serializer/validator - use model_dump_json context in ConversationState persistence - add persistence settings module and cover profile reference tests - document persistence comparison and recommendations
1 parent acf67e3 commit 218728e

File tree

7 files changed

+139
-121
lines changed

7 files changed

+139
-121
lines changed

docs/llm_profiles.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,3 +85,22 @@ Notes on service_id rename
8585
### Follow-up coordination
8686
- Subsequent tasks (agent-sdk-20/21/22) will build on this foundation to expose CLI flags, update documentation, and improve secrets handling.
8787

88+
89+
## Persistence integration review
90+
91+
### Conversation snapshots vs. profile-aware serialization
92+
- **Caller experience:** Conversations that opt into profile references should behave the same as the legacy inline flow. Callers still receive fully expanded `LLM` payloads when they work with `ConversationState` objects or remote conversation APIs. The only observable change is that persisted `base_state.json` files can shrink to `{ "profile_id": "<name>" }` instead of storing every field.
93+
- **Inline vs. referenced storage:** Conversation persistence previously delegated everything to Pydantic (`model_dump_json` / `model_validate`). The draft implementation added a recursive helper (`compact_llm_profiles` / `resolve_llm_profiles`) that walked arbitrary dictionaries and manually replaced or expanded embedded LLMs. This duplication diverged from the rest of the SDK, where polymorphic models rely on validators and discriminators to control serialization.
94+
- **Relationship to `DiscriminatedUnionMixin`:** That mixin exists so we can ship objects across process boundaries (e.g., remote conversations) without bespoke traversal code. Keeping serialization rules on the models themselves, rather than sprinkling special cases in persistence helpers, lets us benefit from the same rebuild/validation pipeline.
95+
96+
### Remote conversation compatibility
97+
- The agent server still exposes fully inlined LLM payloads to remote clients. Because the manual compaction was only invoked when writing `base_state.json`, remote APIs were unaffected. We need to preserve that behaviour so remote callers do not have to resolve profiles themselves.
98+
- When a conversation is restored on the server (or locally), any profile references in `base_state.json` must be expanded **before** the state is materialised; otherwise, components that expect a concrete `LLM` instance (e.g., secret reconciliation, spend tracking) will break.
99+
100+
### Recommendation
101+
- Move profile resolution/compaction into the `LLM` model:
102+
- A `model_validator(mode="before")` can load `{ "profile_id": ... }` payloads with the `LLMRegistry`, while respecting `OPENHANDS_INLINE_CONVERSATIONS` (raise when inline mode is enforced but only a profile reference is available).
103+
- A `model_serializer(mode="json")` can honour the same inline flag via `model_dump(..., context={"inline_llm_persistence": bool})`, returning either the full inline payload or a `{ "profile_id": ... }` stub. Callers that do not provide explicit context will continue to receive inline payloads by default.
104+
- Have `ConversationState._save_base_state` call `model_dump_json` with the appropriate context instead of the bespoke traversal helpers. This keeps persistence logic co-located with the models, reduces drift, and keeps remote conversations working without additional glue.
105+
- With this approach we still support inline overrides (`OPENHANDS_INLINE_CONVERSATIONS=true`), profile-backed storage, and remote access with no behavioural changes for callers.
106+

openhands-sdk/openhands/sdk/conversation/persistence_utils.py

Lines changed: 0 additions & 106 deletions
This file was deleted.

openhands-sdk/openhands/sdk/conversation/state.py

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
# state.py
2-
import json
32
from collections.abc import Sequence
43
from enum import Enum
54
from typing import TYPE_CHECKING, Any, Self
@@ -11,17 +10,16 @@
1110
from openhands.sdk.conversation.event_store import EventLog
1211
from openhands.sdk.conversation.fifo_lock import FIFOLock
1312
from openhands.sdk.conversation.persistence_const import BASE_STATE, EVENTS_DIR
14-
from openhands.sdk.conversation.persistence_utils import (
15-
compact_llm_profiles,
16-
resolve_llm_profiles,
17-
should_inline_conversations,
18-
)
1913
from openhands.sdk.conversation.secrets_manager import SecretsManager
2014
from openhands.sdk.conversation.types import ConversationCallbackType, ConversationID
2115
from openhands.sdk.event import ActionEvent, ObservationEvent, UserRejectObservation
2216
from openhands.sdk.event.base import Event
2317
from openhands.sdk.io import FileStore, InMemoryFileStore, LocalFileStore
2418
from openhands.sdk.logger import get_logger
19+
from openhands.sdk.persistence.settings import (
20+
INLINE_CONTEXT_KEY,
21+
should_inline_conversations,
22+
)
2523
from openhands.sdk.security.confirmation_policy import (
2624
ConfirmationPolicyBase,
2725
NeverConfirm,
@@ -139,10 +137,11 @@ def _save_base_state(self, fs: FileStore) -> None:
139137
Persist base state snapshot (no events; events are file-backed).
140138
"""
141139
inline_mode = should_inline_conversations()
142-
payload = compact_llm_profiles(
143-
self.model_dump(mode="json", exclude_none=True), inline=inline_mode
140+
payload = self.model_dump_json(
141+
exclude_none=True,
142+
context={INLINE_CONTEXT_KEY: inline_mode},
144143
)
145-
fs.write(BASE_STATE, json.dumps(payload))
144+
fs.write(BASE_STATE, payload)
146145

147146
# ===== Factory: open-or-create (no load/save methods needed) =====
148147
@classmethod
@@ -170,12 +169,11 @@ def create(
170169
base_text = None
171170

172171
inline_mode = should_inline_conversations()
172+
context = {INLINE_CONTEXT_KEY: inline_mode}
173173

174174
# ---- Resume path ----
175175
if base_text:
176-
raw_payload = json.loads(base_text)
177-
payload = resolve_llm_profiles(raw_payload, inline=inline_mode)
178-
state = cls.model_validate(payload)
176+
state = cls.model_validate_json(base_text, context=context)
179177

180178
# Enforce conversation id match
181179
if state.id != id:

openhands-sdk/openhands/sdk/llm/llm.py

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import json
55
import os
66
import warnings
7-
from collections.abc import Callable, Sequence
7+
from collections.abc import Callable, Mapping, Sequence
88
from contextlib import contextmanager
99
from typing import TYPE_CHECKING, Any, ClassVar, Literal, get_args, get_origin
1010

@@ -16,8 +16,12 @@
1616
Field,
1717
PrivateAttr,
1818
SecretStr,
19+
SerializationInfo,
20+
SerializerFunctionWrapHandler,
21+
ValidationInfo,
1922
field_serializer,
2023
field_validator,
24+
model_serializer,
2125
model_validator,
2226
)
2327
from pydantic.json_schema import SkipJsonSchema
@@ -75,6 +79,10 @@
7579
from openhands.sdk.llm.utils.retry_mixin import RetryMixin
7680
from openhands.sdk.llm.utils.telemetry import Telemetry
7781
from openhands.sdk.logger import ENV_LOG_DIR, get_logger
82+
from openhands.sdk.persistence.settings import (
83+
INLINE_CONTEXT_KEY,
84+
should_inline_conversations,
85+
)
7886

7987

8088
logger = get_logger(__name__)
@@ -267,6 +275,22 @@ class LLM(BaseModel, RetryMixin, NonNativeToolCallingMixin):
267275
extra="forbid", arbitrary_types_allowed=True
268276
)
269277

278+
@model_serializer(mode="wrap", when_used="json")
279+
def _serialize_with_profiles(
280+
self, handler: SerializerFunctionWrapHandler, info: SerializationInfo
281+
) -> Mapping[str, Any]:
282+
inline_pref = None
283+
if info.context is not None and INLINE_CONTEXT_KEY in info.context:
284+
inline_pref = info.context[INLINE_CONTEXT_KEY]
285+
if inline_pref is None:
286+
inline_pref = True
287+
288+
data = handler(self)
289+
profile_id = data.get("profile_id") if isinstance(data, dict) else None
290+
if not inline_pref and profile_id:
291+
return {"profile_id": profile_id}
292+
return data
293+
270294
# =========================================================================
271295
# Validators
272296
# =========================================================================
@@ -291,11 +315,40 @@ def _validate_api_key(cls, v):
291315

292316
@model_validator(mode="before")
293317
@classmethod
294-
def _coerce_inputs(cls, data):
295-
if not isinstance(data, dict):
318+
def _coerce_inputs(cls, data: Any, info: ValidationInfo):
319+
if not isinstance(data, Mapping):
296320
return data
297321
d = dict(data)
298322

323+
profile_id = d.get("profile_id")
324+
if profile_id and "model" not in d:
325+
inline_pref = None
326+
if info.context is not None and INLINE_CONTEXT_KEY in info.context:
327+
inline_pref = info.context[INLINE_CONTEXT_KEY]
328+
if inline_pref is None:
329+
inline_pref = should_inline_conversations()
330+
331+
if inline_pref:
332+
raise ValueError(
333+
"Encountered profile reference for LLM while "
334+
"OPENHANDS_INLINE_CONVERSATIONS is enabled. "
335+
"Inline the profile or set "
336+
"OPENHANDS_INLINE_CONVERSATIONS=false."
337+
)
338+
339+
registry = None
340+
if info.context is not None:
341+
registry = info.context.get("llm_registry")
342+
if registry is None:
343+
from openhands.sdk.llm.llm_registry import LLMRegistry
344+
345+
registry = LLMRegistry()
346+
347+
llm = registry.load_profile(profile_id)
348+
expanded = llm.model_dump(exclude_none=True)
349+
expanded["profile_id"] = profile_id
350+
d.update(expanded)
351+
299352
if "service_id" in d and "usage_id" not in d:
300353
warnings.warn(
301354
SERVICE_ID_DEPRECATION_MSG,
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
"""Persistence configuration helpers."""
2+
3+
from .settings import INLINE_CONTEXT_KEY, INLINE_ENV_VAR, should_inline_conversations
4+
5+
6+
__all__ = [
7+
"INLINE_CONTEXT_KEY",
8+
"INLINE_ENV_VAR",
9+
"should_inline_conversations",
10+
]
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
"""Shared helpers for SDK persistence configuration."""
2+
3+
from __future__ import annotations
4+
5+
import os
6+
7+
8+
INLINE_ENV_VAR = "OPENHANDS_INLINE_CONVERSATIONS"
9+
INLINE_CONTEXT_KEY = "inline_llm_persistence"
10+
_FALSE_VALUES = {"0", "false", "no"}
11+
12+
13+
def should_inline_conversations() -> bool:
14+
"""Return True when conversations should be persisted with inline LLM payloads."""
15+
16+
value = os.getenv(INLINE_ENV_VAR, "true").strip().lower()
17+
return value not in _FALSE_VALUES

tests/sdk/llm/test_llm_registry_profiles.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
from openhands.sdk.llm.llm import LLM
66
from openhands.sdk.llm.llm_registry import LLMRegistry
7+
from openhands.sdk.persistence.settings import INLINE_CONTEXT_KEY
78

89

910
def test_list_profiles_returns_sorted_names(tmp_path):
@@ -86,6 +87,32 @@ def test_register_profiles_skips_invalid_and_duplicate_profiles(tmp_path):
8687
assert registry.list_usage_ids() == ["shared"]
8788

8889

90+
def test_llm_serializer_respects_inline_context():
91+
llm = LLM(model="gpt-4o-mini", usage_id="service", profile_id="sample")
92+
93+
inline_payload = llm.model_dump(mode="json")
94+
assert inline_payload["model"] == "gpt-4o-mini"
95+
96+
referenced = llm.model_dump(mode="json", context={INLINE_CONTEXT_KEY: False})
97+
assert referenced == {"profile_id": "sample"}
98+
99+
100+
def test_llm_validator_loads_profile_reference(tmp_path, monkeypatch):
101+
monkeypatch.setenv("OPENHANDS_INLINE_CONVERSATIONS", "false")
102+
registry = LLMRegistry(profile_dir=tmp_path)
103+
source_llm = LLM(model="gpt-4o-mini", usage_id="service")
104+
registry.save_profile("profile-tests", source_llm)
105+
106+
parsed = LLM.model_validate(
107+
{"profile_id": "profile-tests"},
108+
context={INLINE_CONTEXT_KEY: False, "llm_registry": registry},
109+
)
110+
111+
assert parsed.model == source_llm.model
112+
assert parsed.profile_id == "profile-tests"
113+
assert parsed.usage_id == source_llm.usage_id
114+
115+
89116
def test_validate_profile_reports_errors(tmp_path):
90117
registry = LLMRegistry(profile_dir=tmp_path)
91118

0 commit comments

Comments
 (0)