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

WIP: Implement streaming completions #32

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

WIP: Implement streaming completions #32

wants to merge 6 commits into from

Conversation

antaz
Copy link
Collaborator

@antaz antaz commented Nov 8, 2024

Changes

  • Added Chunk response to represent a streamed completion chunk
  • Added streaming capability for OpenAI

Examples

completion = LLM.openai(KEY).complete Message.new("user", "write a small story"), stream: true
result = ""
while chunk = completion.resume
  result << chunk.choices.first.content
end
puts result

Copy link
Member

@0x1eef 0x1eef left a comment

Choose a reason for hiding this comment

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

Left a couple of comments

# frozen_string_literal: true

module LLM
class Response::Chunk < Response
Copy link
Member

Choose a reason for hiding this comment

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

Should we inherit from Response::Completion ? It is still a Completion response, right ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It certainly is, I was thinking about it, but got a little hesitant.

@@ -30,8 +30,18 @@ def complete(message, **params)
params = DEFAULT_PARAMS.merge(params)
body = {messages: messages.map(&:to_h)}.merge!(params)
req = preflight(req, body)
res = request(@http, req)
Response::Completion.new(res.body, self).extend(response_parser)
if params[:stream]
Copy link
Member

@0x1eef 0x1eef Nov 8, 2024

Choose a reason for hiding this comment

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

We could offload the complexity to another method

stream!(req, body) if params[:stream]
stream_completion!(req, body) if params[:stream]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@0x1eef I'm still not sure about we go about this

@antaz antaz changed the title Implement streaming completions WIP: Implement streaming completions Nov 8, 2024
# frozen_string_literal: true

module LLM
require_relative "completion"
Copy link
Member

@0x1eef 0x1eef Nov 8, 2024

Choose a reason for hiding this comment

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

Can we move this to lib/llm/response.rb instead ? The require_relative statements found in that file are properly ordered. For example, it could be like this and the dependencies should be properly met:

    require "json"
    require_relative "response/completion"
    require_relative "response/chunk"
    require_relative "response/embedding"

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.

2 participants