Skip to content
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

Make _request response more useful when stream: true #6

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

johnd0e
Copy link
Contributor

@johnd0e johnd0e commented Apr 8, 2024

(On top of #10)

This PR is created to discuss possible OpenAI\_request refactoring.

What's changed:

  • Collect chunks during streaming, and instead of bare text,
    return synthetic response object, similar to one returned when stream: false
  • Simplify ChatSession\generate_response, as there is no longer a need to re-parse the response strings

Reasoning:

  1. I argue that a simple plain-text return value proves to be quite ineffective when dealing with multiple SSE events. Consequently, it is more beneficial to allow the callback function to procure the entirety of the parsed data.
  2. Despite this, receiving integrated data as a return value might still be quite convenient.
    Use Case: The callback function primarily serves to display streaming output to the user. However, after this process, it becomes simpler to handle the integrated data.

The 1st one is most important, and I consider it as necessity. And I would like to know your opinion on the 2nd one.

To be done:

  • Collect other properties, such as finish_reason, usage, etc.

johnd0e added 2 commits April 18, 2024 00:15
And do not demand `object: "chat.completion.chunk"` within the chunk object
to support not fully OpenAI-compliant services
Make _request response more useful when stream: true.
Collect chunks during streaming, and instead of bare text,
return synthetic response object, similar to one returned when stream: false

Simplify ChatSession\generate_response, as there is no longer a need to re-parse the response strings
…reamed response

remove parse_completion_chunk as not used
@johnd0e
Copy link
Contributor Author

johnd0e commented Apr 19, 2024

Done in the last commit.
Cases where n>1 have also been tested.

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.

1 participant