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

Add a spec to test the client with GPT-4 #252

Closed
wants to merge 11 commits into from

Conversation

rmontgomery429
Copy link
Contributor

@rmontgomery429 rmontgomery429 commented Apr 28, 2023

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?

This adds a spec to test the client using gpt-4 as it was mentioned that @alexrudall does not have access to gpt-4 yet in #251. The problem was not manifested but it's still nice to have this spec for sake of completeness.

@rmontgomery429 rmontgomery429 marked this pull request as ready for review April 28, 2023 12:57
@alexrudall
Copy link
Owner

Thanks for your work on this! Couple of things with this

  • I like the shared examples and it's nice to test the separate models, as sometimes OpenAI remove them without much warning and good to catch that so we can update the README
  • Would be great to reproduce the Helicone issue but that's a separate thing maybe and also might be hard as the responses from the AI is not idempotent so we need to somehow check it's speaking correct English without missing any words? lol. I thought I read somewhere the chunks returned an index but all these have index: 0. Maybe they're coming through as one chunk and if they came separately like they seem to in production we'd be able to see the issue?
  • I need anyone who has an API key to be able to run the full specs (with the non-VCR option) and them pass, because it feels more inclusive and will also annoy me if that spec always fails for me until I get through the waitlist. unfortunately that means leaving GPT-4 out for now.

Anyway if you remove the GPT-4 spec (for now) I'll merge. BTW, you're the first contributor I've seen who's understood how I've written the tests and how to add to them (and actually improve them) and I really appreciate that.

@rmontgomery429
Copy link
Contributor Author

@alexrudall I'm happy to help. Regarding GPT-4, I agree with being more inclusive, but I also think like it's important to be able to prove that the gem works with as much of the OpenAI API as possible. Would you consider a switch to opt-in to testing with GPT-4? It could be something simple like setting an environment variable GPT4=true?

@rmontgomery429
Copy link
Contributor Author

@alexrudall Check out the latest changes and let me know what you think. I've added a note to the readme as well.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Jamie McCarthy <[email protected]>
@rmontgomery429
Copy link
Contributor Author

This has gone stale.

@rmontgomery429 rmontgomery429 deleted the spec-for-gpt-4 branch January 20, 2024 04:32
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.

4 participants