Repository navigation
refactor(sdk): type LLM streaming, message, and tokenizer boundaries - #5504
Conversation
|
🧹 PR Artifact Cleanup Queued The |
Preserves the completion precedence and regression identified by alanhuangyoo in OpenHands#4772. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Preserve absent LiteLLM fields, response replay metadata, and subscription config behavior. Cover stream output reconstruction without callbacks. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Normalize generic LiteLLM output-item events at the stream boundary and preserve opaque metadata, ignored text parts, and Responses item IDs. Add focused regressions and retain validation evidence in temporary .pr artifacts. Co-authored-by: openhands <openhands@all-hands.dev>
Keep only a concise validation summary in .pr; preserve detailed development artifacts in an ignored local archive. Co-authored-by: openhands <openhands@all-hands.dev>
Verify that provider events filtered by the typed adapter still reset the upstream stream idle timer and do not emit callbacks. Co-authored-by: openhands <openhands@all-hands.dev>
Record the green full SDK suite, preserved upstream stream timeouts, transport and tokenizer checks, and rebuilt packaged-server validation. Co-authored-by: openhands <openhands@all-hands.dev>
5b07d0e to
120e17a
Compare
|
This is ready for review from my side. Could a maintainer approve the pending CI workflows? Local validation passed: 6,905 SDK tests, the validation matrix, pre-commit checks, and transport/packaged-server checks. |
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
|
Test Suites Triggered
Results will be posted here when complete. |
🧪 Integration Tests ResultsOverall Success Rate: 98.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_openai_gpt_5.5
litellm_proxy_gemini_3.1_pro_preview
litellm_proxy_anthropic_claude_sonnet_4_6
Failed Tests:
litellm_proxy_deepseek_deepseek_v4_flash
litellm_proxy_minimax_MiniMax_M2.7
Skipped Tests:
|
neubig
left a comment
There was a problem hiding this comment.
Approving on behalf of @neubig.
Verified independently:
tests/sdk/llm/-> 1218 passed locally on this branch.- Real agent sessions through
Conversation+TerminalToolon this branch, instrumentinglitellm_responses/litellm_completionto confirm the endpoint hit: all three sessions routed through the Responses path (includingstream=Trueand a reasoning-heavy task), all reachedFINISHEDwith correct tool-use outcomes. - Protocol/fallback semantics confirmed:
OutputItemEventcorrectly rejects pydantic-extra objects,_GenericOutputItemEventcatches them, andcompleted_responseprecedence behaves as documented. - All required status checks green (pre-commit, sdk-tests, tools-tests, cross-tests, agent-server-tests, build-binary-and-test (ubuntu-latest), Check OpenAPI Schema).
Rejection of malformed provider output is accepted as intended behavior.
Note for follow-up (non-blocking): the branch is behind main (main is at 1.53.0, branch at 1.51.0), which is why the non-required 'Check package versions' job reports version changes. The PR diff does not touch version files, so a squash merge will not revert main's versions.
This review was produced by an AI agent (OpenHands) on behalf of @neubig.
HUMAN:
trying to contribute towards issue #4976, this change makes the data/format of data that the llm actually expects a little clearer. hopefully this will make maintenance and debugging easier.
AGENT:
Why
LLM integration currently probes provider objects dynamically throughout core
streaming, message conversion, and tokenizer code. This change moves those
assumptions into explicit typed contracts and focused normalization helpers.
It also retains a completion observed in a stream when the wrapper's final
completed_responseis staleNone, following alanhuangyoo's work in #4772.Summary
sync/async iterator support, optional Transformers loading, and fallback paths.
LiteLLM output-item events, opaque metadata, tool-call replay IDs, and ignored
text parts. Access the declared authentication field directly.
Issue Number
Closes #4976.
How to Test
Recorded local validation on 2026-10-05 after refreshing onto upstream
cb7eabaf7, on macOS arm64 / Python 3.13.3 / LiteLLM 1.93.0 / SDK 1.53.0:9 skipped, 12 xfailed in 323.84s. The sole failure was
TestACPSessionIdPersistence.test_mask_callback_does_not_retain_agent,which timed out waiting for garbage collection. It passed in isolation,
and all 39 tests in its surrounding group passed on rerun. The test
and ACP implementation are unchanged from upstream. This suggests a
cleanup/timing flake; the broad run is not represented as all-green.
No LLM or matrix case failed.
cases. The development harness is local; permanent regressions are under
tests/sdk/llm/./health; its ownedprocess was stopped.
passed.
upstream/mainpassed: no packageversion changes detected. The branch inherits upstream 1.53.0.
See validation summary. Detailed development harnesses and
reports are preserved locally rather than included in the final PR diff.
Regression reproduction: construct
GenericEventorBaseLiteLLMOpenAIResponseObjectwithtype='response.output_item.done', followedby a completion with empty output. Before the boundary correction, all six
generic-event tests failed; now reconstruction works across sync, async, and
async callers receiving sync streams, preserving output-item identity.
Video/Screenshots
Not applicable: internal SDK refactor; transport and packaged-runtime evidence
is recorded above.
Design Doc
No separate design artifact. Provider variability is isolated in three private
LLM helpers; core code consumes declared fields. Public exports, persisted event
fields, REST contracts, dependencies, and package versions are unchanged.
Type
Notes
Parent tracking issue: Remove dynamic attribute access from LLM and telemetry code #4904. Completion-selection semantics follow
alanhuangyoo's earlier PR fix(sdk): keep the completion a Responses stream yielded #4772, credited here and in the implementation.
Refreshed onto upstream
cb7eabaf7without additional conflicts. Conflicts in LLM/test imports and asyncResponses iteration are resolved. Upstream hard/idle timeouts and transient
error classification are preserved. Idle timing wraps the raw provider stream
before event filtering; a new regression verifies ignored events keep the
stream alive without producing callbacks. Ready for maintainer review; fork CI runs require maintainer approval.
Known-kind projections reject some malformed provider values previously
tolerated (e.g. reasoning parts that are primitives or falsy non-list
collections). No real provider emitting those shapes was demonstrated; this
compatibility limitation remains explicit for review.
Only
.pr/validation.mdremains in the final diff. Earlier commits stillcontain development artifacts; rebasing changed commit IDs but did not strip
those artifacts from historical commits.
Companion documentation: docs(sdk): describe typed provider boundaries and Responses streaming docs#891 (ready for review).