-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle empty responses from Chutes AI API #7323
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -341,7 +341,10 @@ describe("ChutesHandler", () => { | |
| mockCreate.mockClear() | ||
| mockCreate.mockImplementationOnce(async () => ({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| // Empty stream for this test | ||
| // Yield minimal content to avoid triggering the empty response error | ||
| yield { | ||
| choices: [{ delta: { content: "test" } }], | ||
| } | ||
| }, | ||
| })) | ||
|
|
||
|
|
@@ -376,11 +379,22 @@ describe("ChutesHandler", () => { | |
|
|
||
| mockCreate.mockImplementationOnce(() => { | ||
| return { | ||
| [Symbol.asyncIterator]: () => ({ | ||
| async next() { | ||
| return { done: true } | ||
| }, | ||
| }), | ||
| [Symbol.asyncIterator]: () => { | ||
| let called = false | ||
| return { | ||
| async next() { | ||
| if (!called) { | ||
| called = true | ||
| // Return minimal content to avoid triggering the empty response error | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concern here - this test modification obscures the original test intent. Consider keeping the original test and adding a new one for the empty response case. |
||
| return { | ||
| done: false, | ||
| value: { choices: [{ delta: { content: "test" } }] }, | ||
| } | ||
| } | ||
| return { done: true } | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
|
|
@@ -421,4 +435,86 @@ describe("ChutesHandler", () => { | |
| const model = handlerWithModel.getModel() | ||
| expect(model.info.temperature).toBe(0.5) | ||
| }) | ||
|
|
||
| it("should throw an error when API returns no content", async () => { | ||
| // Mock a stream that returns no content chunks | ||
| const mockStream = { | ||
| async *[Symbol.asyncIterator]() { | ||
| // Only yield usage data, no content | ||
| yield { | ||
| choices: [{ delta: {} }], | ||
| usage: { | ||
| prompt_tokens: 100, | ||
| completion_tokens: 0, | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| mockCreate.mockResolvedValueOnce(mockStream) | ||
|
|
||
| const systemPrompt = "Test system prompt" | ||
| const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Test message" }] | ||
|
|
||
| const generator = handler.createMessage(systemPrompt, messages) | ||
| const chunks: any[] = [] | ||
|
|
||
| await expect(async () => { | ||
| for await (const chunk of generator) { | ||
| chunks.push(chunk) | ||
| } | ||
| }).rejects.toThrow("Chutes API did not return any content") | ||
|
|
||
| // Should have yielded usage before throwing | ||
| expect(chunks).toHaveLength(1) | ||
| expect(chunks[0]).toEqual({ | ||
| type: "usage", | ||
| inputTokens: 100, | ||
| outputTokens: 0, | ||
| }) | ||
| }) | ||
|
|
||
| it("should throw an error for DeepSeek R1 models when API returns no content", async () => { | ||
| const modelId: ChutesModelId = "deepseek-ai/DeepSeek-R1" | ||
| const handlerWithModel = new ChutesHandler({ | ||
| apiModelId: modelId, | ||
| chutesApiKey: "test-chutes-api-key", | ||
| }) | ||
|
|
||
| // Mock a stream that returns no content chunks | ||
| const mockStream = { | ||
| async *[Symbol.asyncIterator]() { | ||
| // Only yield usage data, no content | ||
| yield { | ||
| choices: [{ delta: {} }], | ||
| usage: { | ||
| prompt_tokens: 100, | ||
| completion_tokens: 0, | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| mockCreate.mockResolvedValueOnce(mockStream) | ||
|
|
||
| const systemPrompt = "Test system prompt" | ||
| const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Test message" }] | ||
|
|
||
| const generator = handlerWithModel.createMessage(systemPrompt, messages) | ||
| const chunks: any[] = [] | ||
|
|
||
| await expect(async () => { | ||
| for await (const chunk of generator) { | ||
| chunks.push(chunk) | ||
| } | ||
| }).rejects.toThrow("Chutes API did not return any content") | ||
|
|
||
| // Should have yielded usage before throwing | ||
| expect(chunks).toHaveLength(1) | ||
| expect(chunks[0]).toEqual({ | ||
| type: "usage", | ||
| inputTokens: 100, | ||
| outputTokens: 0, | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ export class ChutesHandler extends BaseOpenAiCompatibleProvider<ChutesModelId> { | |
|
|
||
| override async *createMessage(systemPrompt: string, messages: Anthropic.Messages.MessageParam[]): ApiStream { | ||
| const model = this.getModel() | ||
| let hasContent = false | ||
|
|
||
| if (model.id.includes("DeepSeek-R1")) { | ||
| const stream = await this.client.chat.completions.create({ | ||
|
|
@@ -66,6 +67,7 @@ export class ChutesHandler extends BaseOpenAiCompatibleProvider<ChutesModelId> { | |
| const delta = chunk.choices[0]?.delta | ||
|
|
||
| if (delta?.content) { | ||
| hasContent = true | ||
| for (const processedChunk of matcher.update(delta.content)) { | ||
| yield processedChunk | ||
| } | ||
|
|
@@ -82,10 +84,47 @@ export class ChutesHandler extends BaseOpenAiCompatibleProvider<ChutesModelId> { | |
|
|
||
| // Process any remaining content | ||
| for (const processedChunk of matcher.final()) { | ||
| hasContent = true | ||
| yield processedChunk | ||
| } | ||
|
|
||
| // If no content was received, throw an error | ||
| if (!hasContent) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice both branches have identical error handling logic. Could we extract this into a shared helper method to follow DRY principles? Something like: private throwIfNoContent(hasContent: boolean): void {
if (!hasContent) {
throw new Error(
`${this.providerName} API did not return any content. This may indicate an issue with the API, model configuration, or request parameters.`,
)
}
} |
||
| throw new Error( | ||
| `${this.providerName} API did not return any content. This may indicate an issue with the API, model configuration, or request parameters.`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate error message text appears in both branches. Consider extracting the error string into a shared constant to reduce duplication and ease future maintenance.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make the error message more actionable? For example: "Check your API key validity, model availability, or request parameters (e.g., max_tokens, temperature)." |
||
| ) | ||
| } | ||
| } else { | ||
| yield* super.createMessage(systemPrompt, messages) | ||
| // For non-DeepSeek models, we reimplement the stream handling instead of calling | ||
| // super.createMessage() to ensure consistent error handling for empty responses | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific reason we're reimplementing the stream handling instead of wrapping the parent's implementation? We could potentially do something like: const parentStream = super.createMessage(systemPrompt, messages);
let hasContent = false;
for await (const chunk of parentStream) {
if (chunk.type === 'text') hasContent = true;
yield chunk;
}
if (!hasContent) throw new Error(...);This would reduce code duplication and maintenance burden. |
||
| const stream = await this.createStream(systemPrompt, messages) | ||
|
|
||
| for await (const chunk of stream) { | ||
| const delta = chunk.choices[0]?.delta | ||
|
|
||
| if (delta?.content) { | ||
| hasContent = true | ||
| yield { | ||
| type: "text", | ||
| text: delta.content, | ||
| } | ||
| } | ||
|
|
||
| if (chunk.usage) { | ||
| yield { | ||
| type: "usage", | ||
| inputTokens: chunk.usage.prompt_tokens || 0, | ||
| outputTokens: chunk.usage.completion_tokens || 0, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If no content was received, throw an error | ||
| if (!hasContent) { | ||
| throw new Error( | ||
| `${this.providerName} API did not return any content. This may indicate an issue with the API, model configuration, or request parameters.`, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Would it be cleaner to keep the original test unchanged and create a separate test specifically for the empty stream scenario? This modification makes the test less clear about its original intent.