Enabled stream on lmi #316
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements streaming functionality for the LMI (Language Model Interface) by enabling the stream parameter in both call and call_single methods. The change replaces the previous "fake streaming" mode with proper streaming that returns an AsyncGenerator when stream=True.
Key changes include:
- Updated method signatures to support streaming with type overloads for both streaming and non-streaming modes
- Added string input support for convenience in the
callmethod - Modified the streaming implementation to yield results incrementally rather than accumulating them
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/lmi/src/lmi/llms.py | Implements streaming functionality with method overloads, updates type annotations from AsyncIterable to AsyncGenerator, and adds string input support |
| packages/lmi/tests/test_llms.py | Adds type assertions to ensure backwards compatibility and proper return types for streaming vs non-streaming calls |
Comments suppressed due to low confidence (1)
packages/lmi/src/lmi/llms.py:699
- This line is a duplicate of the assertion removed on line 699 in the diff. One of the duplicate assertions was properly removed, but this appears to be an inconsistency in the diff presentation.
state["__dict__"].pop("_router", None)
| Raises: | ||
| ValueError: If the LLM type is unknown. | ||
| """ | ||
| if isinstance(messages, str): |
There was a problem hiding this comment.
If we want to do this here, can we remove it from call_single?
packages/lmi/src/lmi/llms.py
Outdated
| A list of LLMResult objects containing the result of the call when stream=False, | ||
| or an AsyncGenerator[LLMResult] when stream=True. |
There was a problem hiding this comment.
Two comments:
- Indent the later lines by 4
- Consider not restating the type hints/variable names, as that can lead to drift over time
For example:
When not streaming, it's a list of result objects for each call, otherwise
it's an async generator of result objects.
| if tools: | ||
| raise NotImplementedError("Using tools with streaming is not supported") | ||
| if callbacks: | ||
| raise NotImplementedError("Using callbacks with streaming is not supported") |
There was a problem hiding this comment.
Can you reword to say "not yet supported"
Also, can you add a comment to each of these saying why they're not supported?
packages/lmi/src/lmi/llms.py
Outdated
| ) -> LLMResult: ... | ||
|
|
||
| @overload | ||
| async def call_single( # type: ignore[overload-cannot-match] |
There was a problem hiding this comment.
This type ignore -- why do you have it? We should have no type ignores here imo, otherwise it means the typing is wrong
This fixed our fake streaming mode we had with
acompletion_iter. Now settingstreamtoTruein bothcallandcall_singlereturns anAsyncGeneratorAlso allowed string inputs in
callfor convenience