Repository navigation
feat(llm): add DecisionModel with OTEL tracing - #6068
Conversation
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||
519c752 to
6c27cfd
Compare
6c27cfd to
088a2c9
Compare
|
088a2c9 to
36e1b15
Compare
36e1b15 to
7eb19e6
Compare
30de7b2 to
6b49cc0
Compare
Validation: decision calls in Laminar tracesTested Result: pass. Each decision call shows in Laminar as an LLM span. The span sits under the active span, including the tool span in an agent run. Its Input pane shows the questions and its Output pane shows the answers. The state is in no span. Setup
What I checked
Click a screenshot to open it at full size. This comment was drafted by an AI agent (OpenHands) on behalf of John-Mason Shackelford. |
Validation: DecisionModel inputs, errors and metricsTested Result: pass, with 2 findings. All three answer types work, sync and async. Invalid input and provider errors become the SDK's exceptions with the expected number of attempts. Metrics add up across calls. The findings are in this PR's response handling and score answer type, listed at the end. Setup
What I checked
FindingsBoth are in this PR's code, not in LiteLLM.
Two inputs fail the same way on This comment was drafted by an AI agent (OpenHands) on behalf of John-Mason Shackelford. |
Rough edges from manual testing: what changedManual testing of
This comment was drafted by an AI agent (OpenHands) on behalf of John-Mason Shackelford. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope. In-repository change: a provider-neutral DecisionModel facade in the SDK's LLM layer, per linked issue #6015 (closed, ready-for-dev). The SDK's only LiteLLM dependency is the generic systemone/asystemone entry point; no provider-specific code lives in the SDK, so the "new AI providers" checkpoint does not apply. Additive public API only (openhands.sdk.llm and openhands.sdk exports); no persisted-settings or event-shape changes.
Verification on the exact head (99e1920a).
uv run pytest tests/sdk/llm/test_decision.py— 10/10 pass, exercising the real call path against a local HTTP server (request format, all three answer types, sync/async, retry on 429, exhaustion →LLMRateLimitError, missing-answerLLMResponseErrorwithout retry, metrics/cost, key redaction, span nesting and attributes).- Full
tests/sdk/llm: 1409 passed; the 2 failures intests/sdk/llm/auth/test_credentials.pyare pre-existing, environment-dependent (Path.home()double-join), and in files this PR does not touch. - Pre-commit on the changed files (Ruff format/lint, pycodestyle, pyright) passes; no
# type: ignorein the new module. - GitHub Actions on this head: 15 runs, 0 failures (sdk-tests, cross-tests, build-binary-and-test on all three OSes, integration-test, REST/OpenAPI breakage checks all green).
Findings. None material. The secrets path uses the canonical validate_secret/serialize_secret helpers with the key redacted in dumps/repr and never placed on spans; retry classification mirrors LLM (LLM_RETRY_EXCEPTIONS + transient HTTP, quota exhaustion excluded, LiteLLM retries disabled via max_retries=0); each attempt gets its own decision.<model> span nested under the active span, and the potentially sensitive state is deliberately excluded — all confirmed by the author's live Laminar validation comments.
One non-blocking pre-merge item, already acknowledged by the author: the PR body says "Documentation is in OpenHands/docs#TBD". The SDK package guidance requires a corresponding OpenHands/docs PR for new public API surfaces — please file and cross-reference it before merge.
✅ APPROVED
Gaps found while writing the examples, closed in 709460cI found these while writing the examples in #6339 against the Jev docs. 1. Questions could only be plain strings. Jev's build guide recommends JSON structure in questions. For example, a The fix:
2. LiteLLM caps a call at 128 questions. Jev's docs say a request has no limit on the number of questions, only on size. LiteLLM's Still not supported: Validation
This comment was drafted by an AI agent on behalf of the user. |
|
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
I don't know if we should do that
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope. Unchanged and in-repository: a provider-neutral DecisionModel facade in the SDK's LLM layer per issue #6015 (ready-for-dev, priority:high). LiteLLM remains the default provider behind the generic systemone/asystemone entry point; the new send()/asend() override hook keeps any provider-specific knowledge in subclasses, not in the SDK. Additive public API only; no persisted-settings or event-shape changes.
Verification on the exact head (2eb3abbc).
uv run pytest tests/sdk/llm/test_decision.py— 22/22 pass, exercising real code paths: the LiteLLM path against a local HTTP server and the newsend()/asend()subclass hook against scripted replies (typed results, metrics including cache tokens and cost, retry on the three documented SDK errors, non-transient errors passing through unchanged, missing-answerLLMResponseErrorwithout retry, the LiteLLM 128-question limit failing before any request, and span nesting/attributes for both paths).- Full
tests/sdk/llm: 1412 passed. The two known failures intests/sdk/llm/auth/test_credentials.pyare pre-existing and environment-dependent (Path.home()double-join) and in files this PR does not touch. - Pre-commit on the changed files (Ruff format/lint, pycodestyle, pyright, dynamic-attribute guard, import dependency rules) passes.
- GitHub Actions on this head: every completed run succeeds (11), including pre-commit, REST/OpenAPI breakage, persisted-settings compatibility, TypeScript CI, and docstring checks; the remaining two are image-build/test-matrix runs still in progress with no failures. The earlier "Review Thread Gate" failure reflects the then-unresolved threads, which are now resolved.
- Verified against the installed LiteLLM 1.105.1 that
MAX_DECISION_QUESTIONS(128) matches LiteLLM's ownmax_lengthon the questions mapping, so the SDK's pre-requestValueErrormirrors the provider's real constraint; and thatlitellm.systemoneaccepts exactly thestr | Mapping | Sequencestate and question-dict shapes the facade passes.
Findings. None material. The secrets path, retry classification (LiteLLM errors + transient HTTP, quota exhaustion excluded), span contents (questions/answers/tokens/cost on the span; state deliberately excluded) and metrics accumulation are unchanged from the previously reviewed head and remain correct. Both review threads from the human reviewer are addressed in d202caf4 (public send()/asend() extension point with docstring contract) and 2eb3abbc (_QuestionBase consolidation, dead import removed). One non-blocking pre-merge item, already acknowledged by the author: the required corresponding OpenHands/docs PR is still "TBD" per the PR body.
✅ APPROVED
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
went through decision.py once more and left a few comments. the raise from e and the json.dumps ones look like real bugs to me, the rest is smaller.
|
Related PRs for
Both will be rebased onto this PR's latest commit ( This comment was posted by an AI agent (OpenHands) on behalf of @jpshackelford. |
… spans Co-authored-by: openhands <openhands@all-hands.dev>
Laminar shows a span's Input and Output panes from lmnr.span.input and lmnr.span.output. Chat calls get them from the child litellm.completion span, which LiteLLM instrumentation does not create for systemone, so decision spans showed raw attributes only. Set the questions as input and the answers as output on the decision span itself, and record gen_ai.response.model. The state stays off the span. Co-authored-by: openhands <openhands@all-hands.dev>
A reply that omitted a requested question used to succeed with fewer answers, and the accessor then raised a bare KeyError. decide() and adecide() now raise LLMResponseError naming the missing questions. The call is not retried, and its tokens and cost are still recorded. The accessor's KeyError lists the answers that exist. Co-authored-by: openhands <openhands@all-hands.dev>
ScoreAnswer.probabilities is keyed by level index, and the map from index to level text was only on raw_response. Copy LiteLLM's legend onto ScoreAnswer so callers can read the level names from the SDK type. Co-authored-by: openhands <openhands@all-hands.dev>
Run the retries-exhausted test for adecide() as well as decide(), so the async path's exception mapping is exercised. Add a test for a reply with no usage: LiteLLM fills in zero tokens, and no cost is recorded. Co-authored-by: openhands <openhands@all-hands.dev>
…odel LiteLLM's decision types accept JSON structure (a string, object or list) for question instructions, Choice option descriptions and Score levels, and an optional true/false criteria on Noul questions. DecisionModel only took plain strings, so these Jev features were out of reach. Widen the question types to match, add NoulCriteria, and type ScoreAnswer.legend so structured levels come back intact. Unset optional fields stay off the wire. LiteLLM also rejects calls with more than 128 questions. Check that limit before sending, with an error that says to split the questions across calls with the same state, and document it on decide(). Co-authored-by: openhands <openhands@all-hands.dev>
DecisionModel called litellm.systemone directly and returned LiteLLM's DecisionsResponse in DecisionResult.raw_response, so a provider LiteLLM does not support could not be added without LiteLLM types. Move the provider call into public send() and asend() methods that take the SDK's question types and return a new SDK DecisionReply (answers, model, DecisionUsage, cost, raw payload). The default methods send through LiteLLM, and they are now the only code that touches LiteLLM. decide() and adecide() wrap each send in the same retries, error mapping, metrics, missing-answer check and decision.<model> span, whichever provider answers. DecisionResult gains model, the versioned model that answered, and raw_response becomes the provider's untyped payload. SDK errors that a custom send() raises pass through unchanged, and LLMRateLimitError, LLMTimeoutError and LLMServiceUnavailableError are retried. LiteLLM's 128-question limit is now checked only on the LiteLLM path. Co-authored-by: openhands <openhands@all-hands.dev>
…otations Co-authored-by: openhands <openhands@all-hands.dev>
- Keep the original exception chain when decide()/adecide() re-raise an SDK or unmapped error, as LLM does. - Build span JSON only when a span exists, once per attempt, and never let it fail the call; set each span attribute independently. - Mirror LiteLLM's question schema: instructions are optional (a Noul needs instructions or criteria), Choice options may lack a description. - Raise LLMBadRequestError for LiteLLM's 128-question and 255-option limits, in the LiteLLM send path only. - Raise LLMResponseError for answers of the wrong type or that nobody asked for, without retry. - Drop frozen=True from the question models. - Document DecisionResult.metrics as the model's running totals. - Share the retry condition with LLM via transient_error_condition(). - Drop the max_retries kwarg, which systemone never reads. - Forward custom_llm_provider, extra_headers, safety_identifier and AWS credentials to LiteLLM. - Fail the test server loudly on requests beyond the scripted replies. Co-authored-by: openhands <openhands@all-hands.dev>
Add tests for the state staying off the decision span, one span per retry attempt, spans on failed and async calls, HTTP 5xx/401/400/quota handling on the LiteLLM path, forwarded provider options, model validation, cost and metrics edge cases, and API key leakage. Add a strict xfail for non-JSON values in the state or questions, which are retried as a service outage instead of failing fast as a bad request.
8ff03b8 to
d7bf909
Compare
HUMAN:
An OpenHands customer is using decision models in their OpenHands agent code but wishes that the SDK supported them because they miss the OTEL visibility they have with the rest of their agent work. LiteLLM now supports them and this PR adds a facade and models so that like our LLM class so that LiteLLM client code is kept to a single entry point.
AGENT:
Why
Calls to decision models (classifier-style models that answer typed questions about a state) go through
litellm.systemone, and Laminar's LiteLLM instrumentation only traces chat and responses calls. A decision-model call therefore leaves no span, and callers get none of the SDK's retries, exception mapping or metrics either.DecisionModelruns those calls with the same retry rules, exception mapping,MetricsandSecretStrhandling asLLM, and wraps each attempt in adecision.<model>span. The span records the questions, answers, token counts and cost, but not the state, which may be sensitive. Questions, answers and replies are SDK types. LiteLLM is the default provider, used only insideDecisionModel.send()andasend(). A subclass can override those two methods to call a provider LiteLLM does not support, and it keeps the retries, metrics and span.Summary
DecisionModelwithdecide()andadecide(), theNoul,ChoiceandScorequestion types, and typedDecisionResultanswers.questionsis a dict of named questions of any mix of types, and the result holds one answer per name. Through LiteLLM a call takes at most 128 questions and aChoiceat most 255 options. The LiteLLM path raisesLLMBadRequestErrorbefore sending if a call has more.Choiceoption descriptions andScorelevels, and an optionalNoulCriteriathat says what makes aNoultrue or false.instructionsis optional (aNoulneedsinstructionsorcriteria), and aChoiceoption may have no description.LLMResponseError, without retry, when a reply is missing an answer, has an answer of the wrong type, or has an answer nobody asked for.custom_llm_provider,extra_headers,safety_identifierand the AWS credential fieldsLLMhas to LiteLLM.send()andasend()take SDK questions and return an SDKDecisionReplywith answers, the model that answered,DecisionUsageand cost.DecisionResult.modelrecords the model that answered.LLM's retry rules (through a sharedtransient_error_condition()helper), exception mapping, metrics and secret handling, for every provider. A customsend()raisesLLMRateLimitError,LLMTimeoutErrororLLMServiceUnavailableErrorfor failures worth retrying.decision.<model>span, nested under the active span, with questions, answers, tokens and cost.DecisionModelandDecisionResultfromopenhands.sdk, and all new names fromopenhands.sdk.llm.Issue Number
Refs #6015
How to Test
uv run pytest tests/sdk/llm/test_decision.pyruns the real call path against a local HTTP server and checks the request format, all three answer types, sync and async, retry on 429, the mappedLLMRateLimitErrorwhen retries run out, metrics and cost, redaction of the API key, and that the span nests under an active Laminar span with the expected attributes. ADecisionModelsubclass that overridessend()andasend()gets the same checks: typed results, metrics, retries on SDK errors, the missing-answer error and the span. Branch coverage ofdecision.pyis 98%.Against the live Jev API (
typesafe/jev-latest, LiteLLM 1.105.1), onedecide()call with aNoul, aChoiceand aScorequestion returned an answer for each (refund 0.98, teamtechnical, impact 1.0) fromjev-1.13.0, using 411 input and 68 output tokens at a cost of $0.0000173. One call with 100 mixed questions returned all 100 answers in 0.10 s for $0.0002, about $0.000002 per question. One question alone cost $0.0000132.Video/Screenshots
None.
Design Doc
None.
Type
Notes
Builds on #6030 (LiteLLM 1.105.1):
litellm.systemonefirst ships in LiteLLM 1.104.3, and the tests here need it. Examples are in #6339 and documentation is in OpenHands/docs#1054. Both will be rebased onto this PR's latest API.This PR was drafted by an AI agent on behalf of the user.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:d7bf909-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d7bf909-python) is a multi-arch manifest supporting both amd64 and arm64d7bf909-python-amd64) are also available if needed