Conversation
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for OpenAI’s stateful Responses API to the lmi LiteLLM wrapper, enabling multi-turn continuity via previous_response_id rather than requiring a stateless full-history assumption.
Changes:
- Introduces Responses API request/response conversion helpers and a new
LiteLLMModelResponses API execution path (streaming + non-streaming). - Tracks and propagates
response_idthroughLLMResultandMessage.infofor stateful follow-up calls. - Adds tests for delta detection, conversion/parsing helpers, and call behavior; introduces
USE_RESPONSES_APIenv flag and addsorjsondependency.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/lmi/src/lmi/llms.py | Adds Responses API conversion/parsing utilities and integrates a stateful Responses API call path (including streaming). |
| packages/lmi/src/lmi/types.py | Extends LLMResult with response_id to support stateful multi-turn. |
| packages/lmi/src/lmi/constants.py | Adds USE_RESPONSES_API env-controlled feature flag. |
| packages/lmi/tests/test_llms.py | Adds unit tests covering Responses API helpers and stateful call behavior. |
| packages/lmi/pyproject.toml | Adds orjson dependency to support litellm Responses API requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| converted.append(item) | ||
| return converted | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
Do we want to just blow up here?
There was a problem hiding this comment.
This is run over all tool responses, and it's very possible for a tool response to start with [ (I think InterpreterEnv might prepend [stdout]?).
I added a logger.warning instead of making it silent.
jamesbraza
left a comment
jamesbraza
left a comment
There was a problem hiding this comment.
Nice work! Huge win for us
| def _convert_tool_response_content(content: str | None) -> str | list[dict[str, Any]]: | ||
| """Convert tool response content to Responses API format. | ||
|
|
||
| Aviary stores images as JSON: [{"type": "image_url", "image_url": {"url": "..."}}] | ||
| Responses API expects: [{"type": "input_image", "image_url": "..."}] | ||
| """ | ||
| if not content: | ||
| return "" | ||
| if content.startswith("["): |
There was a problem hiding this comment.
Can we pass the entire ToolResponseMessage here, not just content?
The reason is, the if content.startswith("["): -- this can be better done with message.content_is_json_str
Also imo there may be other cases in the future where you need the full message to make a conversion.
My most ideal here is to make this a method of ToolResponseMessage. Then you can just do:
tool_response: ToolResponseMessage
tool_response.to_responses_api()There was a problem hiding this comment.
Yeah this is a good idea, I moved to it. However, I did not go so far as .to_responses_api() - that would have to be in aviary, and I don't want to upstream this logic that far.
sidnarayanan
force-pushed
the
stateful-responses
branch
from
832f088 to
bb5eb93
Compare
Better version of #389, don't need to make stateless assumption