Skip to content

Raise exceptions #344

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 11 commits into from
Nov 5, 2023
Merged

Raise exceptions #344

merged 11 commits into from
Nov 5, 2023

Conversation

atesgoral
Copy link
Contributor

@atesgoral atesgoral commented Oct 19, 2023

Note
This PR builds on #338. I unfortunately can't set the base when cutting a PR from a forked repo. Reviewing it commit by commit after skipping the first commit would be helpful.

  • BREAKING CHANGE: ruby-openai now raises errors instead of silently ignoring them. It is better for production apps to fail fast and take note of any errors instead of ignoring them. OpenAI is not expected to return any invalid payload in its normal operation.
  • Removed the completion test case from the finetunes suite because it wasn't adding any value since it doesn't really exercise an actual fine-tuned model. Besides, the create cassette was broken (had 400 responses throughout). The reason that test was passing is because ruby-openai was eating those errors.
  • Tweaks to README:
    • Tweak/add some bits about token counting during streaming
    • Replace "ChatGPT" with "GPT" (the former is a web app, the latter is the actual model)

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?

@atesgoral atesgoral marked this pull request as ready for review October 20, 2023 00:57
@atesgoral atesgoral mentioned this pull request Oct 20, 2023
3 tasks
@alexrudall
Copy link
Owner

Thanks @atesgoral - taking a look now

Copy link
Owner

@alexrudall alexrudall left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this!

@alexrudall
Copy link
Owner

Fixed a couple of errors but that spec is still failing due to Faraday raising the error 🤔

@atesgoral
Copy link
Contributor Author

@alexrudall We might need to pass in a full-fledged mock Request object. I'll poke around.

@alexrudall
Copy link
Owner

@alexrudall We might need to pass in a full-fledged mock Request object. I'll poke around.

Legend, thank you

@atesgoral
Copy link
Contributor Author

Ah, I just remembered removing that HTTP error case from the http spec because I was running into issues with mocking a request. I then added a VCR at the client spec level instead.

The old test might have popped back in due to a bad rebase. I removed it again.

@atesgoral atesgoral requested a review from alexrudall November 1, 2023 15:28
@alexrudall alexrudall merged commit e30f5c2 into alexrudall:main Nov 5, 2023
@atesgoral atesgoral deleted the raise-exceptions branch November 6, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants