Skip to content

Fixes #184: JSON parsing issue when async streaming #216

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

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

athyuttamre
Copy link

@athyuttamre athyuttamre commented Feb 7, 2023

I was able to reproduce #184 where sometimes calling Completion.acreate(stream=True) would result in a JSON parsing issue.

This was happening because our code was trying to parse incomplete JSON objects. It was receiving chunks that looked like this (notice the incomplete line at the end):

data: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": "\\n", "index": 0, "logprobs": null, "finish_details": null}], "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": "Hello", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": " again", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": "!", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": " Is", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": " there", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "created": 1675757473, "choices": [{"text": " something", "index": 0, "logprobs": null, "finish_details": null, "model": "text-davinci-003"}\n\ndata: {"id": "cmpl-6hDMH0KtbEmSdkAvX4xEHy7be8XMz", "object": "text_completion", "c

It looks like we were using the wrong function from aiohttp to parse streaming bodies. Instead of rbody.iter_chunks() we should be using async for line in rbody instead. See docs here: https://docs.aiohttp.org/en/stable/streams.html#asynchronous-iteration-support.

This change immediately fixed the issue in my code.

@athyuttamre athyuttamre force-pushed the dev/atty/fix-stream-bug branch from 0ed54a6 to 28c97f0 Compare February 7, 2023 09:13
@athyuttamre athyuttamre force-pushed the dev/atty/fix-stream-bug branch from 28c97f0 to 54c38e6 Compare February 7, 2023 09:14
@athyuttamre athyuttamre requested a review from ddeville February 7, 2023 09:14
@ddeville
Copy link
Contributor

ddeville commented Feb 7, 2023

We went back and forth on this that tbh I've lost sense of what the right fix for this is :)

We moved to using iter_chunks in #172 and then I I went back to looping over the body itself and splitting additional lines afterwards, and then put back the iter_chunks upon review in #177.

Reading the docs your change seems correct to me but since we've had issues in the past make sure that what was raised in #171 doesn't happen again after this PR.

@athyuttamre
Copy link
Author

I ran the previously added tests. I think I saw those issues myself too and no longer see them. Fingers crossed!

Copy link
Contributor

@ddeville ddeville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@athyuttamre athyuttamre merged commit 5386db1 into main Feb 7, 2023
@athyuttamre athyuttamre deleted the dev/atty/fix-stream-bug branch February 7, 2023 22:18
davedittrich pushed a commit to davedittrich/openai-python that referenced this pull request Nov 14, 2023
cgayapr pushed a commit to cgayapr/openai-python that referenced this pull request Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants