Skip to content

Fix streaming doesn't return finish reason #798

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2023
Merged

Fix streaming doesn't return finish reason #798

merged 1 commit into from
Oct 19, 2023

Conversation

gmcgoldr
Copy link
Contributor

@gmcgoldr gmcgoldr commented Oct 7, 2023 •
edited
Loading

When stream == True the branch with the yield statement that contains the finish_reason isn't visited when remaining_tokens is empty.

Moved the yield statement containing finish_reason after the loop over remaining_tokens. This branch is always visited when stream == True.

Fixes #735

@gmcgoldr
Fix streaming doesn't return finish reason
89ae8bd 
When streaming the yield that contains the finish can be skipped. This change ensures that yield isn't skipped.
@gmcgoldr gmcgoldr mentioned this pull request Oct 7, 2023
Closed
4 tasks
@abetlen
Copy link
Owner

abetlen commented Oct 18, 2023

@gmcgoldr sorry for the delay on this, got swamped with work recently. It looks good but I need to re-enable the completion tests before I merge this. Should be able to do that soon.

@abetlen abetlen merged commit 09a8406 into abetlen:main Oct 19, 2023
@ChengjieLi28 ChengjieLi28 mentioned this pull request Nov 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG?] finish_reason is None when using create_chat_completion(stream=True)
2 participants
@gmcgoldr @abetlen