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

cloudflare[minor]: use native Cloudflare Workers AI binding #5287

Merged
merged 12 commits into from
Sep 11, 2024

Conversation

Cherry
Copy link
Contributor

@Cherry Cherry commented May 5, 2024

As of 2024-04-11, there's no need to use the @cloudflare/ai package and it's deprecated:

https://developers.cloudflare.com/workers-ai/changelog/

This swaps the code over to use the native binding directly, and updates @cloudflare/workers-types which now ships the Ai type. No other code changes are needed.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label May 5, 2024
Copy link

vercel bot commented May 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Sep 11, 2024 0:01am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Sep 11, 2024 0:01am

@dosubot dosubot bot added the auto:improvement Medium size change to existing code to handle new use-cases label May 5, 2024
@@ -38,12 +38,11 @@
"author": "LangChain",
Copy link

Choose a reason for hiding this comment

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

Hey there! I noticed that this PR includes a change in dependencies, specifically the removal of "@cloudflare/ai" as a hard dependency and an update to the version of "@cloudflare/workers-types" as a dev dependency. I'm flagging this for your review to ensure it aligns with our dependency management strategy. Thanks!

Copy link
Collaborator

@bracesproul bracesproul left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, I have a couple comments to prevent breaking changes.

libs/langchain-cloudflare/package.json Outdated Show resolved Hide resolved
libs/langchain-cloudflare/src/embeddings.ts Show resolved Hide resolved
@bracesproul bracesproul added the in progress PRs that are in progress but not ready to merge label May 6, 2024
@Cherry Cherry changed the title cloudflare[patch]: use native Cloudflare Workers AI binding cloudflare[minor]: use native Cloudflare Workers AI binding May 13, 2024
@jacoblee93
Copy link
Collaborator

Hey so just got wind that the Rest API methods will be available in the main SDK:

https://www.npmjs.com/package/cloudflare

We should wait to land this until that is generally ready

@Cherry
Copy link
Contributor Author

Cherry commented May 13, 2024

The general cloudflare SDK makes sense outside of Workers, but bindings inside of Workers are faster and more secure - no additional authentication required, so using it inside a Worker for anything that has native bindings doesn't make a lot of sense in my experience.

Updating https://github.com/Cherry/langchainjs/blob/4b9192176440b4acab9eac60a140200640d8bee2/libs/langchain-cloudflare/src/chat_models.ts#L78 to use the official SDK once the AI endpoints are available there makes sense, but https://github.com/Cherry/langchainjs/blob/fix/workers-ai-native-binding/libs/langchain-cloudflare/src/embeddings.ts seems to be intended for use only within a Worker.

@jacoblee93
Copy link
Collaborator

jacoblee93 commented May 13, 2024

Yeah for sure but ideally we don't lose functionality for people using it outside workers - I've done that myself in demos.

It should accept either a binding or a config object/client IMO.

@Cherry
Copy link
Contributor Author

Cherry commented May 13, 2024

Agreed that would be ideal, but it doesn't look like it does today: https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain-cloudflare/src/embeddings.ts.

No functionality here will be lost, so perhaps that improvement would be better served in another PR?

@Cherry
Copy link
Contributor Author

Cherry commented Aug 28, 2024

As far as I can tell, this is ready to merge. Is there anything left to resolve before merging?

@jacoblee93
Copy link
Collaborator

Apologies will have a look as soon as I can!

@wanoo21
Copy link
Contributor

wanoo21 commented Sep 2, 2024

Hey team, I have an issue that might be related to this!

image

I'm looking forward to this PR!

Copy link

vercel bot commented Sep 2, 2024

@Cherry is attempting to deploy a commit to the LangChain Team on Vercel.

A member of the Team first needs to authorize it.

@jacoblee93
Copy link
Collaborator

Hey, extremely sorry about the delay, this looks good!

@dosubot dosubot bot added the lgtm PRs that are ready to be merged as-is label Sep 10, 2024
@jacoblee93
Copy link
Collaborator

We are about to push minor versions of all packages related to the 0.3 release, will push this out alongside that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases in progress PRs that are in progress but not ready to merge lgtm PRs that are ready to be merged as-is size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants