-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
Add streaming with Faraday #234
Conversation
alexrudall
commented
Apr 2, 2023
•
edited
Loading
edited
- WIP on v4 of the gem - main breaking change is switching HTTP library to improve performance and allow streaming and parallel requests
- Switch to Faraday
- Add streaming to Chat as MVP
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.
The PR diff size of 22336 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 22335 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 22338 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 22338 lines exceeds the maximum allowed for the inline comments feature.
ruby-openai.gemspec
Outdated
@@ -25,7 +25,8 @@ Gem::Specification.new do |spec| | |||
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } | |||
spec.require_paths = ["lib"] | |||
|
|||
spec.add_dependency "httparty", ">= 0.18.1" | |||
spec.add_dependency "faraday", "2.7.4" | |||
spec.add_dependency "faraday-typhoeus", "1.0.0" |
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.
I personally have no interest in using or install typhoeus on my system. The advantage of Faraday is you can leave those decisions to someone else.
If you want to test Faraday with typhoeus in the gem, the pattern for that would be to put typhoeus in the Gemfile but not the gemspec, that way it's not required.
Also, our Faraday requirement in our app
gem "faraday", ">= 2"
We happen to be on 2.7.4, but I'd prefer not to be locked in to that version
- spec.add_dependency "faraday", "2.7.4"
+ spec.add_dependency "faraday", ">= 2"
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.
Great points, thanks for review! Experimenting with Faraday only version.
This looks great! How can I help push this through the finish line? |
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.
The PR diff size of 23814 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 23814 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 23812 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 23812 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 23810 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 24977 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26402 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26402 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26397 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26397 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26397 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 26998 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 27377 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 27377 lines exceeds the maximum allowed for the inline comments feature.
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.
The PR diff size of 27380 lines exceeds the maximum allowed for the inline comments feature.
Streaming getting there :) Apr-24-2023.18-38-28.mp4 |
Thanks for your input all. Would appreciate any final reviews 👍
|
Improved stream parsing
This is rad. Thanks for this change! ❤️ |
Thank you everyone! |
lib/openai/client.rb
Outdated
JSON.parse(response.body) | ||
rescue JSON::ParserError | ||
# Convert a multiline file of JSON objects to a JSON array. | ||
JSON.parse(response.body.gsub("}\n{", "},{").prepend("[").concat("]")) |
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.
Hi @alexrudall, I come here now in October 😅. I was wondering when happens this case. I have seen that this happen to us when OpenAI returns empty string. Is this documented somewhere why it happens?