-
Notifications
You must be signed in to change notification settings - Fork 402
[inference provider] Add wavespeed.ai as an inference provider #1424
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
base: main
Are you sure you want to change the base?
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.
Hello, thank you for your contribution
The code is of great quality overall - I left a few comments regarding our code style.
Please make sure the client can be used to query your API for all supported tasks, and that the payload are matching your API.
Thanks again!
packages/inference/README.md
Outdated
- [HF Inference API (serverless)](https://huggingface.co/models?inference=warm&sort=trending) | ||
|
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.
- [HF Inference API (serverless)](https://huggingface.co/models?inference=warm&sort=trending) |
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.
has deleted
hfModelId: "wavespeed-ai/wan-2.1/i2v-480p", | ||
providerId: "wavespeed-ai/wan-2.1/i2v-480p", | ||
status: "live", | ||
task: "image-to-video", |
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 task is not supported in the client code - let's remove it for now
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.
has deleted
import { InferenceOutputError } from "../lib/InferenceOutputError"; | ||
import { ImageToImageArgs } from "../tasks"; | ||
import type { BodyParams, HeaderParams, RequestArgs, UrlParams } from "../types"; | ||
import { delay } from "../utils/delay"; | ||
import { omit } from "../utils/omit"; | ||
import { base64FromBytes } from "../utils/base64FromBytes"; | ||
import { | ||
TaskProviderHelper, | ||
TextToImageTaskHelper, | ||
TextToVideoTaskHelper, | ||
ImageToImageTaskHelper, | ||
} from "./providerHelper"; | ||
|
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.
We use import type
when the import is only used as a type
import { InferenceOutputError } from "../lib/InferenceOutputError"; | |
import { ImageToImageArgs } from "../tasks"; | |
import type { BodyParams, HeaderParams, RequestArgs, UrlParams } from "../types"; | |
import { delay } from "../utils/delay"; | |
import { omit } from "../utils/omit"; | |
import { base64FromBytes } from "../utils/base64FromBytes"; | |
import { | |
TaskProviderHelper, | |
TextToImageTaskHelper, | |
TextToVideoTaskHelper, | |
ImageToImageTaskHelper, | |
} from "./providerHelper"; | |
import { InferenceOutputError } from "../lib/InferenceOutputError"; | |
import type { ImageToImageArgs } from "../tasks"; | |
import type { BodyParams, HeaderParams, RequestArgs, UrlParams } from "../types"; | |
import { delay } from "../utils/delay"; | |
import { omit } from "../utils/omit"; | |
import { base64FromBytes } from "../utils/base64FromBytes"; | |
import type { | |
TaskProviderHelper, | |
TextToImageTaskHelper, | |
TextToVideoTaskHelper, | |
ImageToImageTaskHelper, | |
} from "./providerHelper"; | |
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.
Modify as suggested
}; | ||
} | ||
|
||
type WaveSpeedAIResponse<T = WaveSpeedAITaskResponse> = WaveSpeedAICommonResponse<T>; |
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.
I'm not sure this type alias is needed, can we remove it?
type WaveSpeedAIResponse<T = WaveSpeedAITaskResponse> = WaveSpeedAICommonResponse<T>; |
WaveSpeedAICommonResponse
can be renamed to WaveSpeedAIResponse
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 type is needed and will be used in two places. It's uncertain whether it will be used again in the future.
It follows the DRY (Don't Repeat Yourself) principle
It provides better type safety (through default generic parameters)
It makes the code more readable and maintainable
case "completed": { | ||
// Get the video data from the first output URL | ||
if (!taskResult.outputs?.[0]) { | ||
throw new InferenceOutputError("No video URL in completed response"); | ||
} | ||
const videoResponse = await fetch(taskResult.outputs[0]); | ||
if (!videoResponse.ok) { | ||
throw new InferenceOutputError("Failed to fetch video data"); | ||
} | ||
return await videoResponse.blob(); |
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.
From what I understand, the payload can be something else than a video (eg an image)
Let's update the error message to reflect that
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.
yes,
I revised it.
if (!args.parameters) { | ||
return { | ||
...args, | ||
model: args.model, | ||
data: args.inputs, | ||
}; | ||
} else { | ||
return { | ||
...args, | ||
inputs: base64FromBytes( | ||
new Uint8Array(args.inputs instanceof ArrayBuffer ? args.inputs : await (args.inputs as Blob).arrayBuffer()) | ||
), | ||
}; | ||
} | ||
} | ||
|
||
override preparePayload(params: BodyParams): Record<string, unknown> { | ||
return { | ||
...omit(params.args, ["inputs", "parameters"]), | ||
...(params.args.parameters as Record<string, unknown>), | ||
image: params.args.inputs, | ||
}; | ||
} |
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.
I think only one of the two ( preparePayload
or preparePayloadAsync
) should be responsible for building the payload, meaning, I'd rather move the rename of inputs
to image
in preparePayloadAsync
an have preparePayload
as dumb as possible
cc @hanouticelina - would love your opinion on that specific point
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.
I only kept preparePayloadAsync func
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.
I think only one of the two ( preparePayload or preparePayloadAsync) should be responsible for building the payload, meaning, I'd rather move the rename of inputs to image in preparePayloadAsync an have preparePayload as dumb as possible
yes agree!
inputs: base64FromBytes( | ||
new Uint8Array(args.inputs instanceof ArrayBuffer ? args.inputs : await (args.inputs as Blob).arrayBuffer()) | ||
), |
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.
Does the wavespeed API support base64-encoded images as inputs?
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.
yes
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.
thank you @arabot777 for the PR! I left some minor comments. I tested the 3 tasks supported by Wavespeed.ai and it works as expected with the changes I suggested.
constructor() { | ||
super(WAVESPEEDAI_API_BASE_URL); | ||
} | ||
|
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.
you should override preparePayload
to avoid sending unexpected fields to the API, since we rely on preparePayloadAsync
to format the payload for this task
override preparePayload(params: BodyParams): Record<string, unknown> { | |
return params.args; | |
} |
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.
Here, for the preparePayload
, I directly reused the preparePayload
of the base class WavespeedAITask
because there is no ambiguity in all types of interface parameter fields provided by the wavespeed api.
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.
I wonder if this consideration point complies with the code specification? Or do I need to make some better modifications in accordance with the norms? thinks
async preparePayloadAsync(args: ImageToImageArgs): Promise<RequestArgs> { | ||
return { | ||
...args, | ||
inputs: args.parameters?.prompt, |
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.
based on https://wavespeed.ai/models/wavespeed-ai/hidream-e1-full, it seems that the prompt should be send in "prompt" not "inputs". let me know if i'm missing something here.
inputs: args.parameters?.prompt, | |
prompt: args.parameters?.prompt, |
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.
Because here I reused the preparePayload
function of the general class WavespeedAITask
. The prompt here is obtained from params.args.inputs
.
For general processing, an "inputs
" field was added to place the prompt when handling this logic
Co-authored-by: célina <[email protected]>
Co-authored-by: célina <[email protected]>
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.
Second round of code review, thank you! We're getting there
Note: make sure you run pnpm format
and pnpm lint
to conform our code style.
/** | ||
* Common response structure for all WaveSpeed AI API responses | ||
*/ | ||
interface WaveSpeedAICommonResponse<T> { | ||
code: number; | ||
message: string; | ||
data: T; | ||
} | ||
|
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 abstraction is not necessary IMO, let's remove it (see my other comment)
/** | |
* Common response structure for all WaveSpeed AI API responses | |
*/ | |
interface WaveSpeedAICommonResponse<T> { | |
code: number; | |
message: string; | |
data: T; | |
} |
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.
It has been modified as suggested
type WaveSpeedAIResponse<T = WaveSpeedAITaskResponse> = WaveSpeedAICommonResponse<T>; | ||
|
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.
Following the previous comment - let's remove one level of abstraction
type WaveSpeedAIResponse<T = WaveSpeedAITaskResponse> = WaveSpeedAICommonResponse<T>; | |
interface WaveSpeedAIResponse { | |
code: number; | |
message: string; | |
data: WaveSpeedAITaskResponse | |
} | |
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.
It has been modified as suggested
preparePayload(params: BodyParams): Record<string, unknown> { | ||
const payload: Record<string, unknown> = { | ||
...omit(params.args, ["inputs", "parameters"]), | ||
...(params.args.parameters as Record<string, unknown>), | ||
prompt: params.args.inputs, | ||
}; | ||
// Add LoRA support if adapter is specified in the mapping |
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.
We don't need to cast into Result<string, unknown>
if the params
have the proper type
ImageToImageArgs
, TextToImageArgs
, and TextToVideoArgs
need to be improrted from "../tasks"
preparePayload(params: BodyParams): Record<string, unknown> { | |
const payload: Record<string, unknown> = { | |
...omit(params.args, ["inputs", "parameters"]), | |
...(params.args.parameters as Record<string, unknown>), | |
prompt: params.args.inputs, | |
}; | |
// Add LoRA support if adapter is specified in the mapping | |
preparePayload(params: BodyParams<ImageToImageArgs | TextToImageArgs | TextToVideoArgs>): Record<string, unknown> { | |
const payload: Record<string, unknown> = { | |
...omit(params.args, ["inputs", "parameters"]), | |
...params.args.parameters, | |
prompt: params.args.inputs, | |
}; | |
// Add LoRA support if adapter is specified in the mapping |
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.
It has been modified as suggested
if (params.mapping?.adapter === "lora" && params.mapping.adapterWeightsPath) { | ||
payload.loras = [ | ||
{ | ||
path: params.mapping.adapterWeightsPath, |
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.
For reference, adapterWeightsPath
is the path to the LoRA weights inside the associated HF repo
eg, for nerijs/pixel-art-xl, it will be
"pixel-art-xl.safetensors"
Let's make sure that is indeed what your API is expecting when running LoRAs
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.
Here I see that fal is the endpoint that has been concatenated with hf.
Can I directly set the adapterWeightsPath to a lora http address? Or any other address.
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.
In the test cases, I conducted the test in this way. The adapterWeightsPath
was directly passed over as an input parameter of lora.
"wavespeed-ai/flux-dev-lora": {
hfModelId: "wavespeed-ai/flux-dev-lora",
providerId: "wavespeed-ai/flux-dev-lora",
status: "live",
task: "text-to-image",
adapter: "lora",
adapterWeightsPath:
"https://d32s1zkpjdc4b1.cloudfront.net/predictions/599f3739f5354afc8a76a12042736bfd/1.safetensors",
},
"wavespeed-ai/flux-dev-lora-ultra-fast": {
hfModelId: "wavespeed-ai/flux-dev-lora-ultra-fast",
providerId: "wavespeed-ai/flux-dev-lora-ultra-fast",
status: "live",
task: "text-to-image",
adapter: "lora",
adapterWeightsPath: "linoyts/yarn_art_Flux_LoRA",
},
However, I'm not sure whether the input parameters submitted by hf to lora must be the abbreviation of the file path of the hf model and then concatenated with the hf address in the code. If it is this kind of specification, I can complete it in the format of fal
override prepareHeaders(params: HeaderParams, isBinary: boolean): Record<string, string> { | ||
this.accessToken = params.accessToken; | ||
const headers: Record<string, string> = { Authorization: `Bearer ${params.accessToken}` }; | ||
if (!isBinary) { | ||
headers["Content-Type"] = "application/json"; | ||
} | ||
return headers; | ||
} |
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 the same behavior as the blanket implementation here:
https://github.com/arabot777/huggingface.js/blob/f706e02d6128f559bd5551072344ff6e31b9c4be/packages/inference/src/providers/providerHelper.ts#L114-L124
No need for an override IMO
override prepareHeaders(params: HeaderParams, isBinary: boolean): Record<string, string> { | |
this.accessToken = params.accessToken; | |
const headers: Record<string, string> = { Authorization: `Bearer ${params.accessToken}` }; | |
if (!isBinary) { | |
headers["Content-Type"] = "application/json"; | |
} | |
return headers; | |
} |
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.
I removed this part of the logic at the beginning. However, the getresponse
method of imageToimage.ts
did not pass in header information.
I have to rewrite prepareHeaders here and by assignment
this.accessToken = params.accessToken;
To ensure that the complete ak information of the header can be passed on when calling getresponse
What’s in this PR
WaveSpeedAI is a high-performance AI image and video generation service platform, offering industry-leading generation speeds. Now, want to be listed as an Inference Provider on the Hugging Face Hub
The JS Client Integration was completed based on the inference-providers help documentation and passed the test. I am submitting the pr now and look forward to further communication with you
Test