-
Notifications
You must be signed in to change notification settings - Fork 330
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
Updated types for Workers AI #3278
Updated types for Workers AI #3278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I truly appreciate the effort to continually improve Workers AI types, these changes would result in multiple major breaking changes to types of a GA product, as well as significant regressions in DX. I also have lots of other questions/concerns.
Removal of AiTextGenerationInput
, etc.
It is very common in applications to build out your model inputs like this:
const input: AiTextGenerationInput = {...};
This lets you then make dynamic changes to them while adhering to types before passing to AI.run()
, and prevents any human issues of typo'ing things like messages
, etc.
After this lands, this (and many other things like it) will immediately type-error when I upgrade my project to the latest workers-types
, or run npm update
. I would have to firstly know this changed (minor/patch versions wouldn't create this expectation with semver), install the new package, and then either update my tsconfig.json
or manually import the types I need to get my project type-checking correctly again.
DX issues
Types
With the above, making the AI.run()
function a generic any
essentially breaks all of the hard work from previous, and even the most recent fixes that landed just a short while ago with #3242.
Now, unless I realise that this breaking change has landed and needs me to install another package, I could pass anything to this and TS won't warn me of issues until I hit the runtime. Things that would have previously been caught before I deploy, now may break my production application because I don't have type warnings.
Onboarding
Has this been communicated with the C3/create-cloudflare team? When initiating Workers, workers-types
is installed and the tsconfig.json
configured as appropriate. It sounds like additional steps are going to be needed here now for any AI workers.
Usability Downgrade
Previously, env.AI.run
was all I needed to get accurate types. I simply had to define my env
which is handled mostly automagically for you today with c3
and wrangler
, and that was it - I had all the types I needed.
Now I have to do something like this:
import { AiModelTypes } from "@cloudflare/ai-types"
const ai = env.AI as Ai<AiModelTypes>;
which diverges from all other products, and adds additional complexity.
Visibility
workers-types
is a staple in the Cloudflare Workers ecosystem. It's open source, encourages so many third-party contributions, handles compatibility date changes, and even added recent snapshot checking to reduce regressions and other issues in #3188.
- Will the
@cloduflare/ai-types
package be OSS? - Will it encourage third party contributions?
- Will it have a similar approach to versioning and regression handling?
- Where can people get support for it and/or report issues? This repo still?
Conclusion
I would encourage you to reconsider this approach. It introduces a lot of DX regressions, and major breaking changes to the types of a GA product. If this is to land in the current state, I only hope it's at bare minimum in a new major version of workers-types
so it's very clear to people when upgrading that there are breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a (large) breaking change to workers-types, which will require additional changes across the release process
These were very valid points @Cherry , we have decided to continue adding all models to this repo instead of creating a new npm package. I have added new commits to add new models in the list as well. |
This PR also adds the flux-schnell model with it's own custom types, so we can close this duplicate PR: #3314 |
We are moving types for supported models under a new NPM package (https://www.npmjs.com/package/@cloudflare/ai-types). This PR removes all the model specific types and converts AI.run() into a generic function.
added whisper, whisper-tiny-en, whisper-large-v3-turbo, llama-3.2-11b-vision-instruct to the list
4583076
to
336cc99
Compare
We are moving types for supported models under a new NPM package (https://www.npmjs.com/package/@cloudflare/ai-types).
This PR removes all the model specific types and converts AI.run() into a generic function.