alexrudall / ruby-openai

OpenAI API + Ruby! 🤖❤️ NEW: Assistant Vector Stores
MIT License
2.61k stars 302 forks source link

Conditionally require faraday/multipart #393

Open ajGingrich opened 7 months ago

ajGingrich commented 7 months ago

Only Require Faraday Multipart if Faraday Version is greater than 2 because Faraday V1 includes the gem already.

https://github.com/alexrudall/ruby-openai/issues/392

All Submissions:

alexrudall commented 7 months ago

Cool, thank you @ajGingrich! Have you tested this does work with both versions of Faraday?

ajGingrich commented 7 months ago

@alexrudall I haven't explicitly tested it with the newer versions but I've confirmed that it works with Faraday 1.1. I also grabbed this change from a OpenAPI generated repo that works.

Here are the changes from Faraday with 2.0.0 release indicate the dropping of the multipart middleware.

I'm confident it should be good but I can try to think through testing it with different versions more extensively if you would prefer before merging!

alexrudall commented 7 months ago

@ajGingrich I would definitely prefer that as it could affect tens of thousands of people if we get it wrong :) I always try to test PRs as much as possible even small ones. Thank you!

ajGingrich commented 7 months ago

@alexrudall Are you sure that Faraday 1 is supported? I was just testing a little and I'm getting failures that are unrelated to the multipart change.

Reproduction Steps

I'm getting a number of failures but everything passes with 2.7. Perhaps the solution is to actually remove support for Faraday 1.

alexrudall commented 7 months ago

eep, you're right...

I think we need to require faraday json response and request middleware, but can't find the right library 🤔

Thanks for checking, that's a really good spot

alexrudall commented 7 months ago

If we can fix this, could be nice to add Faraday 1 + 2 to the CircleCI matrix so both get tested.