Repository navigation
fix: add batch item dispatching to specific handlers based on URL - #1495
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a dispatchBatchItem function to the batch processor, enabling support for multiple API endpoints including embeddings, moderations, image generation, and video generation. Previously, the batch processor was limited to chat completions. The changes also include manual HTTP tests and a unit test to verify correct routing. However, the dispatchBatchItem function contains a bug where it fails to strictly force stream: false for chat requests, which will cause parsing errors during batch execution. Additionally, there are concerns regarding the routing of image edits and video endpoints to generation-specific handlers, as these typically require different request formats.
| async function dispatchBatchItem( | ||
| url: string, | ||
| headers: Headers, | ||
| body: Record<string, unknown> | ||
| ): Promise<Response> { | ||
| const request = new Request(`http://localhost${url}`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify(body), | ||
| }); | ||
|
|
||
| switch (url) { | ||
| case "/v1/embeddings": | ||
| return handleEmbeddingsRoute(request); | ||
| case "/v1/moderations": | ||
| return handleModerationsRoute(request); | ||
| case "/v1/images/generations": | ||
| case "/v1/images/edits": | ||
| return handleImagesGenerationsRoute(request); | ||
| case "/v1/videos": | ||
| case "/v1/videos/generations": | ||
| return handleVideosGenerationsRoute(request); | ||
| // /v1/chat/completions, /v1/completions, /v1/responses | ||
| default: { | ||
| return handleChat( | ||
| new Request(`http://localhost${url}`, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify({ | ||
| ...body, | ||
| // BATCH-SPECIFIC: Force stream: false — batches don't support SSE responses | ||
| stream: body.stream ?? false, | ||
| }), | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The dispatchBatchItem function contains a bug where stream: body.stream ?? false preserves stream: true if present in the input body. Since batch processing does not support SSE responses and expects a standard JSON response, this will cause failures when the processor attempts to parse the stream as JSON. Additionally, the Request object is redundantly created at the start and then discarded in the default case.
I suggest refactoring the function to determine the final body first and then create the request once, while ensuring stream is strictly forced to false for chat endpoints.
async function dispatchBatchItem(
url: string,
headers: Headers,
body: Record<string, unknown>
): Promise<Response> {
// BATCH-SPECIFIC: Force stream: false — batches don't support SSE responses.
// This is primarily relevant for chat/completion endpoints.
const isChatEndpoint = !["/v1/embeddings", "/v1/moderations", "/v1/images/generations", "/v1/images/edits", "/v1/videos", "/v1/videos/generations"].includes(url);
const request = new Request(`http://localhost${url}`, {
method: "POST",
headers,
body: JSON.stringify({
...body,
...(isChatEndpoint ? { stream: false } : {}),
}),
});
switch (url) {
case "/v1/embeddings":
return handleEmbeddingsRoute(request);
case "/v1/moderations":
return handleModerationsRoute(request);
case "/v1/images/generations":
case "/v1/images/edits":
return handleImagesGenerationsRoute(request);
case "/v1/videos":
case "/v1/videos/generations":
return handleVideosGenerationsRoute(request);
default:
return handleChat(request);
}
}| case "/v1/images/edits": | ||
| return handleImagesGenerationsRoute(request); | ||
| case "/v1/videos": | ||
| case "/v1/videos/generations": | ||
| return handleVideosGenerationsRoute(request); |
There was a problem hiding this comment.
Routing /v1/images/edits and /v1/videos to handlers imported from generations/route is likely to cause issues. These endpoints typically expect different request payloads (e.g., images/edits usually requires multipart data for image files) and follow different logic than standard generation endpoints. If these handlers are strictly for generations, they will likely reject requests to these other paths.
098639e
into
cheaper-inference:release/v3.7.0
|
Thanks @hartmark for this great contribution! 🎉 It is now officially merged into the release/v3.7.0 branch and will be part of the next release. We appreciate your effort! |
…eaper-inference#1495) Integrated into release/v3.7.0
…eaper-inference#1495) Integrated into release/v3.7.0
Summary
My previous PR #1479 had one issue, it always routed to the chat endpoint, so for example bulking embeddings failed.
This PR fixes that issue.
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Added tests for affected code