-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix AttributeError in streaming response cleanup #4236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
mattf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r-bit-rry a chain of hasattr like this suggests we've done something wrong in the design. have we or can we just call close?
It really comes down to what we want to support, since this was never strictly typed, I'm assuming there are other objects that can be generated by the sse_generator. and on a more serious note @mattf so its our decision if we want to enforce certain typings, and act upon, or let this pattern "catch all" |
mattf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as propose, the hasattr chain will cover up an api contract bug somewhere in the system.
an AsyncStream is making it to a place where only AsyncIterators should be.
i did a little sleuthing and i think there is a bug in at least _maybe_overwrite_id. there are multiple provider impls, so there may be others.
will you find the places where the api contract is being violated and patch them?
also, will you create a regression test that at least tests the openai mixin provider?
|
@mattf sure thing, I'll start working on those |
|
@mattf We're facing two options in order to avoid hasattr chain as I see it when treating the this is explicit and simple but carries a small overhead per chunk option 2: adapter pattern direct delegation with no re-yielding and a more explicit intent regarding locations of violations, where we will need patching, these are the places I was able to spot:
returned AsyncStream (has close()) instead of AsyncIterator (has aclose())
Returned raw client.chat.completions.create() response
Returned raw client.completions.create() response
Returned raw litellm.acompletion() result
|
|
@r-bit-rry great finds! it looks like we're violating the api contract and using |
|
@mattf I suggest using the first option, with direct declaration of the types, and changing the API contract for openai_completion, removing the #type: ignore comments. |
|
@r-bit-rry sounds good! |
|
@mattf as I'm digging, I'm finding new stuff, apparently we have a discrepancy between official and internal documentation of openai_completion and the implementation in place: however the documentation both on our end and on openai official end does not support streaming to begin with (docs/docs/contributing/new_api_provider.mdx:40-43:
OpenAI documentation: |
mattf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get rid of the type ignores now?
| Generate an OpenAI-compatible completion for the given prompt using the specified model. | ||
| :returns: An OpenAICompletion. | ||
| :returns: An OpenAICompletion or an async iterator of OpenAICompletion chunks when streaming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the async iterator business is an impl detail, what about mentioning SSE events instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm not sure I can dismiss async iterator as impl detail, as we decided that the contract has changed, its an expected type to be returned, I don't mind for documentation/comment purpose considering SSE events even though those are not typed.
- Remaining
# type: ignorecomments are only for external library issues:
# type: ignore[arg-type]- LiteLLM streaming types don't match OpenAI
# type: ignore[return-value]- External libs lack type stubs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r-bit-rry this text gets published in the OpenAPI spec, which is language / implementation agnostic.
what about passthrough? |
This PR fixes issue #3185
The code calls
await event_gen.aclose()but OpenAI'sAsyncStreamdoesn't have anaclose()method - it hasclose()(which is async).when clients cancel streaming requests, the server tries to clean up with:
But
AsyncStreamhas never had a publicaclose()method. The error message literally tells us:Verification
reproduce_issue_3185.shcan be used to verify the fix.