openai-node
openai-node copied to clipboard
openai
Switch from `node-fetch` to `undici`
One thing to note is that with undici, it looks like response.body isn't a Node Readable stream, which would be breaking change for node-fetch users… any thoughts there?
One thing to note is that with undici, it looks like response.body isn't a Node Readable stream, which would be breaking change for node-fetch users… any thoughts there?
Yes this an important distinction. It will be a Node.js Web Stream instead. This is to spec so that code is actually interoperable. node-fetch has been a cause of much frustration for a very long time because they don't do this. Developers using fetch - in any context - should expect it to behave as specified
This is to spec so that code is actually interoperable.
You mean, interoperable with web streams, not Node ones, right? So it will be breaking? Is there any way we can ease that migration for developers?
Current, interoperable with web streams. Also meaning that when you use it in code that runs in both browser and on the server it behaves the same.
There does exist some utilities for users to transform one to the other. And they are fairly close in implementation so some users won't even notice a difference. The key here is that this breaking change is intentional. Understand if you want that to happen in a major version of this library. Otherwise, if this library tried to do something else you'd just be kicking the can down the road. True interoperability should not have anything other than Web Streams. If true interoperability isn't the absolute goal, we could add a transform to this library and keep Node users on Node streams.
I'm probably leaning towards a major version bump in that case. I imagine shimming a web ReadableStream to also be compatible with a node Readable stream would be pretty gross. But if there is a library that does that sanely, it'd make the rollout much easier.
For what it's worth I doubt a huge number of people are likely to be relying on the raw response so far, so I doubt it'll be a very painful major version.
On Wed, Nov 8 2023 at 8:39 AM, Ethan Arrowood @.***> wrote:
Current, interoperable with web streams. Also meaning that when you use it in code that runs in both browser and on the server it behaves the same.
There does exist some utilities for users to transform one to the other. And they are fairly close in implementation so some users won't even notice a difference. The key here is that this breaking change is intentional. Understand if you want that to happen in a major version of this library. Otherwise, if this library tried to do something else you'd just be kicking the can down the road. True interoperability should not have anything other than Web Streams. If true interoperability isn't the absolute goal, we could add a transform to this library and keep Node users on Node streams.
— Reply to this email directly, view it on GitHub https://github.com/openai/openai-node/pull/402#issuecomment-1802261965, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFL6LT2IDSS6VVMHH5UCI3YDOYVBAVCNFSM6AAAAAA6OBVVX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBSGI3DCOJWGU . You are receiving this because you commented.Message ID: @.***>
No - haven't gotten back around to this. Need to still figure out how to run tests (still haven't created an OpenAI key). Do you have CI you could enable for this PR? I'll update and mark it "ready for review" so GitHub prompts it too.
Sadly we don't run CI in public for this repo yet, just internally.
The steps to test it locally are:
yarn
OPENAI_API_KEY=… yarn ts-node ecosystem-tests/cli.ts
npx prism mock https://github.com/openai/openai-openapi/raw/master/openapi.yaml & # or in a separate tab.
yarn test
Updated failing types I believe. Still cannot run tests. Not sure what's going wrong. Maybe my local env is in a bad state. I think this is very close, maybe a maintainer more familiar with the shim and test matrix can take it over the finish line.
The most important detail here is that Node.js now has an actual spec compliant Fetch implementation. Its types are different and most importantly it now returns Web Streams instead of Node.js Streams (spec compliant).
This is more expected IMO as fetch should do as fetch is documented to do. Node.js makes available a Readable.fromWeb() that can be used to convert the web streams back to node streams if needed.
Amazing, thank you so much @Ethan-Arrowood ! Finishing off those tests ourselves sounds totally fair; we have a lot on our plate right not but will try to take a look at getting this over the line soon.
I've approved running the CI workflow. Looks like it is failing with a module error:
yarn install v1.22.19
[1/4] Resolving packages...
[2/4] Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">= 18". Got "1[6](https://github.com/openai/openai-node/actions/runs/6908472736/job/18819218282?pr=402#step:4:7).20.2"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error: Process completed with exit code 1.
https://github.com/openai/openai-node/actions/runs/6908472736/job/18819218282?pr=402
Perhaps we need to update the workflow to use Node 18 rather than 16?
https://github.com/openai/openai-node/blob/master/.github/workflows/ci.yml
@Ethan-Arrowood can you rebase on master so that the latest CI workflow can be run with Node 18?
Looks like CI now runs correctly! Some type errors to fix next it looks like.
Thanks Atty!
I think @jedwards1211 is working on these type errors etc – Ethan, you shouldn't need to take a look.
When will this be merged into master? The DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead. warning is so annoying. Everybody that uses langchain gets this warning.
Sorry for the annoyance.
We're hoping to merge this within the next 6-8 weeks.
It will be a breaking change and there are some deep complexities, which is why it'll take longer than one might hope, but we are actively working on it behind the scenes.
Update here is that we're planning on moving entirely to builtin APIs so we won't even need to depend on undici! Sorry for the delays, this will still be a while.
Hey team - I am going to close this PR for now, given our imminent plans to migrate to built-in fetch. We can definitely reopen this if we feel we'd rather go in this direction. Thank you for starting this conversation, and for this contribution - the related issues should get cleared up when we remove the node-fetch dependency.
