-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore(weave): Fixes Stream response format in OpenAI #1726
Conversation
choice["finish_reason"] = chunk_choice.finish_reason | ||
if chunk_choice.logprobs: | ||
choice["logprobs"] = chunk_choice.logprobs | ||
def _process_chunk(self, chunk: ChatCompletionChunk) -> None: |
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.
This looks like a huge change, but if you hide whitespace, you see that it is just moving some logic to a helper function.
finish_run(result_with_usage.model_dump(exclude_unset=True)) | ||
|
||
return _stream_create_gen() # type: ignore | ||
base_stream = self._base_create(*args, **kwargs) |
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.
This logic is moved into the class
finish_run=finish_run, | ||
) | ||
|
||
return stream |
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.
Notice we get rid of type ignore!
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=0981df8615564059b170181490c24b8ac41b98fa |
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=af05a4d2f3fc0940b522df569a32d31e11baa1b5 |
@@ -84,6 +84,41 @@ async def __aiter__(self) -> AsyncIterator[ChatCompletionChunk]: | |||
self._finish_run(result_with_usage.model_dump(exclude_unset=True)) | |||
|
|||
|
|||
class WeaveStream(Stream): |
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.
New class - pretty simple. It follows the same pattern as the async class above and uses the same code that was previously in a function below.
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=af05a4d2f3fc0940b522df569a32d31e11baa1b5 |
1 similar comment
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=af05a4d2f3fc0940b522df569a32d31e11baa1b5 |
@@ -369,10 +369,52 @@ def teardown(): | |||
unpatch() | |||
|
|||
|
|||
class MockSyncResponse: |
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.
Follow same pattern as async
raise NotImplementedError("Function calls not supported") | ||
# function call | ||
if chunk_choice.delta.function_call: | ||
raise NotImplementedError("Function calls not supported") |
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.
@tssweeney: I would like to understand why function calls are not supported here. I know this is deprecated in Openai SDK in favor of tool calls, but libraries like LangChain currently still use this feature, and this causes the following test case related to agents break in LangChain:
def test_agent_run_with_tools( |
Our OpenAI integration was not returning a
Stream
object from the sync streaming API. This was not caught in the past because most people just use it for it's iterator interface and everything works. However, Langchain actually uses the context block (enter/exit) dunder methods. In order to bring our integration back into compliance, I implement a similar class as theWeaveStream
which is the sister class toWeaveAsyncStream
. These now follow a common pattern and are true subclasses of the original openai class.This should have no functional change for users and hits existing tests.
This came to my attention because the growth team is implementing Langchain and hit this underlying issue.