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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b91921
fix: use native Cloudflare Workers AI binding
Cherry May 5, 2024
e19caaa
Merge branch 'main' into fix/workers-ai-native-binding
Cherry May 13, 2024
f96a244
Merge remote-tracking branch 'upstream/main' into fix/workers-ai-nati…
Cherry May 24, 2024
0342a20
chore: rebuild yarn.lock
Cherry May 24, 2024
ec86a33
Merge branch 'main' into fix/workers-ai-native-binding
Cherry Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions libs/langchain-cloudflare/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,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!

"license": "MIT",
"dependencies": {
"@cloudflare/ai": "^1.0.47",
"@langchain/core": ">0.1.0 <0.3.0",
"uuid": "^9.0.1"
},
"devDependencies": {
"@cloudflare/workers-types": "^4.20231218.0",
"@cloudflare/workers-types": "^4.20240502.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're replacing @cloudflare/ai with this, it needs to be moved out of dev deps and into standard deps

Copy link
Author

@Cherry Cherry May 6, 2024 •
edited

Choose a reason for hiding this comment

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

workers-types only provides the types for the Ai binding - there's nothing runtime that would need this to be a dependency and not a devDependency. The runtime for the AI binding is handled entirely in workerd via wrangler.

"@jest/globals": "^29.5.0",
"@langchain/scripts": "~0.0.14",
"@langchain/standard-tests": "workspace:*",
Expand Down
7 changes: 3 additions & 4 deletions libs/langchain-cloudflare/src/embeddings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Ai } from "@cloudflare/ai";
import { Fetcher } from "@cloudflare/workers-types";
import { Ai } from "@cloudflare/workers-types";
import { Embeddings, EmbeddingsParams } from "@langchain/core/embeddings";
import { chunkArray } from "@langchain/core/utils/chunk_array";

Expand All @@ -14,7 +13,7 @@ type AiTextEmbeddingsOutput = {

export interface CloudflareWorkersAIEmbeddingsParams extends EmbeddingsParams {
/** Binding */
binding: Fetcher;
binding: Ai;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be a breaking change. Instead of this we should deprecate the binding input, and make it optional. With that we can add an additional (also optional) arg which is typed with the new Ai type.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, good thoughts. The confusion here might come from the fact that the new AI binding is more of a binding than the old one (the old one is often referred to as a "wrapped binding" in the cf dev ecosystem), so I don't have a good name for this.

Do you have a suggestion for what this could be called? Or would it make sense to just cut a breaking change given how this is in 0.0.x versions right now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's make this a minor version.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this also break people using this outside Cloudflare Workers?

We'll also want to update the docs but I can take care of that.

Copy link
Author

@Cherry Cherry May 13, 2024 •
edited

Choose a reason for hiding this comment

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

The external usage outside of Cloudflare Workers seems to be in https://github.com/Cherry/langchainjs/blob/4b9192176440b4acab9eac60a140200640d8bee2/libs/langchain-cloudflare/src/chat_models.ts#L78, which hasn't changed. This update to the embeddings.ts via CloudflareWorkersAIEmbeddingsParams seems to have only ever been intended for use inside of Cloudflare Workers, since it requires a Fetcher as the binding, which is a Cloudflare thing, unless I'm missing something?


/**
* Model name to use
Expand Down Expand Up @@ -57,7 +56,7 @@ export class CloudflareWorkersAIEmbeddings extends Embeddings {
"Must supply a Workers AI binding, eg { binding: env.AI }"
);
}
this.ai = new Ai(fields.binding);
this.ai = fields.binding;
this.modelName = fields?.model ?? fields.modelName ?? this.model;
this.model = this.modelName;
this.stripNewLines = fields.stripNewLines ?? this.stripNewLines;
Expand Down
18 changes: 5 additions & 13 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6693,24 +6693,17 @@ __metadata:
languageName: node
linkType: hard

"@cloudflare/ai@npm:^1.0.47":
version: 1.0.47
resolution: "@cloudflare/ai@npm:1.0.47"
checksum: a9e371f789256bb5795c0cb60d975749b07d6cb50141b997da6b5c94380ddf18e851cba518e246414fcab4239d8f4438c5fe29172e6f06c136be3be19a3b65e0
languageName: node
linkType: hard

"@cloudflare/workers-types@npm:^4.20230922.0":
version: 4.20230922.0
resolution: "@cloudflare/workers-types@npm:4.20230922.0"
checksum: 629bab47cdbcb74e3c42fc9486f5186734b6dd734154cea7a0983ad83ee053b4fb1ae13ff618a7287612bc3b3d19ad72d6a34a84289a903623cb8a13af57596b
languageName: node
linkType: hard

"@cloudflare/workers-types@npm:^4.20231218.0":
version: 4.20231218.0
resolution: "@cloudflare/workers-types@npm:4.20231218.0"
checksum: b7e50a76ee8e9d662227bbb74798b93b6102acc224f1071a9c99a9adb419ad0b3bdabf7561e7e1b4a320a6a4616badeecdfb1848fbdaada197c7b37d845b8774
"@cloudflare/workers-types@npm:^4.20240502.0":
version: 4.20240512.0
resolution: "@cloudflare/workers-types@npm:4.20240512.0"
checksum: 4a8671a7884c75a305b064c34470e7fb09ab2c967a9388be6bc046030d6ad4e46a2ca9e92288c2eb6d49624e357683f0e1f4bd0d2de1f540a9964635faf87a3b
languageName: node
linkType: hard

Expand Down Expand Up @@ -8991,8 +8984,7 @@ __metadata:
version: 0.0.0-use.local
resolution: "@langchain/cloudflare@workspace:libs/langchain-cloudflare"
dependencies:
"@cloudflare/ai": ^1.0.47
"@cloudflare/workers-types": ^4.20231218.0
"@cloudflare/workers-types": ^4.20240502.0
"@jest/globals": ^29.5.0
"@langchain/core": ">0.1.0 <0.3.0"
"@langchain/scripts": ~0.0.14
Expand Down