From 9bff00fce736093e3034cbbdec10794c0e9a72a1 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 28 Jan 2025 12:11:38 -0500 Subject: [PATCH 01/10] Filter empty text parts when streaming --- .../src/requests/stream-reader.test.ts | 29 ++++++++++++++++++- .../vertexai/src/requests/stream-reader.ts | 29 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index eea72f9c7a6..948f3d507ac 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -18,7 +18,8 @@ import { aggregateResponses, getResponseStream, - processStream + processStream, + filterEmptyTextParts } from './stream-reader'; import { expect, use } from 'chai'; import { restore } from 'sinon'; @@ -404,3 +405,29 @@ describe('aggregateResponses', () => { }); }); }); + +describe('filterEmptyTextParts', () => { + it('Removes only empty text parts', () => { + const parts = [ + { + text: 'foo' + }, + { + text: '' + }, + { + text: 'bar' + } + ]; + + const filteredParts = filterEmptyTextParts(parts); + expect(filteredParts).to.deep.equal([ + { + text: 'foo' + }, + { + text: 'bar' + } + ]); + }); +}); diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index 8162407d90b..8624acedd85 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -62,6 +62,16 @@ async function getResponsePromise( ); return enhancedResponse; } + + // The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there + // will be an error. To prevent this, filter out the empty text part from responses. + if (value.candidates && value.candidates.length > 0) { + value.candidates.forEach(candidate => { + if (candidate.content) { + candidate.content.parts = candidate.content.parts.filter(part => part.text !== ''); + } + }); + } allResponses.push(value); } } @@ -76,6 +86,15 @@ async function* generateResponseSequence( break; } + // The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there + // will be an error. To prevent this, filter out the empty text part from responses. + if (value.candidates && value.candidates.length > 0) { + value.candidates.forEach(candidate => { + if (candidate.content) { + candidate.content.parts = candidate.content.parts.filter(part => part.text !== ''); + } + }); + } const enhancedResponse = createEnhancedContentResponse(value); yield enhancedResponse; } @@ -203,3 +222,13 @@ export function aggregateResponses( } return aggregatedResponse; } + +/** + * The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there + * will be an error. To prevent this, filter out the empty text part from responses. + * + * @internal + */ +export function filterEmptyTextParts(parts: Part[]): Part[] { + return parts?.filter(part => part.text !== ''); +} From ee216fc9d6a19400a6e284c58758417130c98b0d Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 28 Jan 2025 13:59:24 -0500 Subject: [PATCH 02/10] Add changeset --- .changeset/seven-oranges-care.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/seven-oranges-care.md diff --git a/.changeset/seven-oranges-care.md b/.changeset/seven-oranges-care.md new file mode 100644 index 00000000000..41b59355bc7 --- /dev/null +++ b/.changeset/seven-oranges-care.md @@ -0,0 +1,5 @@ +--- +'@firebase/vertexai': patch +--- + +Filter out empty text parts from streaming responses. From f317d46a69dd3fb01d45e7fe5ee3845a38986422 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 28 Jan 2025 14:00:41 -0500 Subject: [PATCH 03/10] Remove unused filterEmptyTextParts() --- .../src/requests/stream-reader.test.ts | 29 +------------------ .../vertexai/src/requests/stream-reader.ts | 18 ++++-------- 2 files changed, 7 insertions(+), 40 deletions(-) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index 948f3d507ac..eea72f9c7a6 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -18,8 +18,7 @@ import { aggregateResponses, getResponseStream, - processStream, - filterEmptyTextParts + processStream } from './stream-reader'; import { expect, use } from 'chai'; import { restore } from 'sinon'; @@ -405,29 +404,3 @@ describe('aggregateResponses', () => { }); }); }); - -describe('filterEmptyTextParts', () => { - it('Removes only empty text parts', () => { - const parts = [ - { - text: 'foo' - }, - { - text: '' - }, - { - text: 'bar' - } - ]; - - const filteredParts = filterEmptyTextParts(parts); - expect(filteredParts).to.deep.equal([ - { - text: 'foo' - }, - { - text: 'bar' - } - ]); - }); -}); diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index 8624acedd85..db8a2326ccf 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -68,7 +68,9 @@ async function getResponsePromise( if (value.candidates && value.candidates.length > 0) { value.candidates.forEach(candidate => { if (candidate.content) { - candidate.content.parts = candidate.content.parts.filter(part => part.text !== ''); + candidate.content.parts = candidate.content.parts.filter( + part => part.text !== '' + ); } }); } @@ -91,7 +93,9 @@ async function* generateResponseSequence( if (value.candidates && value.candidates.length > 0) { value.candidates.forEach(candidate => { if (candidate.content) { - candidate.content.parts = candidate.content.parts.filter(part => part.text !== ''); + candidate.content.parts = candidate.content.parts.filter( + part => part.text !== '' + ); } }); } @@ -222,13 +226,3 @@ export function aggregateResponses( } return aggregatedResponse; } - -/** - * The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there - * will be an error. To prevent this, filter out the empty text part from responses. - * - * @internal - */ -export function filterEmptyTextParts(parts: Part[]): Part[] { - return parts?.filter(part => part.text !== ''); -} From 6769c1048590cd1ea2bbbbfac789372d8b340a86 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 28 Jan 2025 14:10:15 -0500 Subject: [PATCH 04/10] Move logic into a function --- .../vertexai/src/requests/stream-reader.ts | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index db8a2326ccf..ac8d6415a42 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -63,17 +63,7 @@ async function getResponsePromise( return enhancedResponse; } - // The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there - // will be an error. To prevent this, filter out the empty text part from responses. - if (value.candidates && value.candidates.length > 0) { - value.candidates.forEach(candidate => { - if (candidate.content) { - candidate.content.parts = candidate.content.parts.filter( - part => part.text !== '' - ); - } - }); - } + deleteEmptyTextParts(value); allResponses.push(value); } } @@ -88,17 +78,7 @@ async function* generateResponseSequence( break; } - // The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there - // will be an error. To prevent this, filter out the empty text part from responses. - if (value.candidates && value.candidates.length > 0) { - value.candidates.forEach(candidate => { - if (candidate.content) { - candidate.content.parts = candidate.content.parts.filter( - part => part.text !== '' - ); - } - }); - } + deleteEmptyTextParts(value); const enhancedResponse = createEnhancedContentResponse(value); yield enhancedResponse; } @@ -226,3 +206,21 @@ export function aggregateResponses( } return aggregatedResponse; } + +/** + * The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there + * will be an error. To prevent this, filter out the empty text part from responses. + * + * See: https://github.com/firebase/firebase-js-sdk/issues/8714 + */ +export function deleteEmptyTextParts(response: GenerateContentResponse): void { + if (response.candidates) { + response.candidates.forEach(candidate => { + if (candidate.content && candidate.content.parts) { + candidate.content.parts = candidate.content.parts.filter( + part => part.text !== '' + ); + } + }); + } +} From 708892a79c984738e500b11150d2e18f0705aae3 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 29 Jan 2025 12:34:15 -0500 Subject: [PATCH 05/10] final --- .../src/requests/stream-reader.test.ts | 120 ++++++++++++++++++ packages/vertexai/test-utils/mock-response.ts | 3 + 2 files changed, 123 insertions(+) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index eea72f9c7a6..b2ab735f62d 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -17,6 +17,7 @@ import { aggregateResponses, + deleteEmptyTextParts, getResponseStream, processStream } from './stream-reader'; @@ -33,6 +34,7 @@ import { GenerateContentResponse, HarmCategory, HarmProbability, + Part, SafetyRating } from '../types'; @@ -220,6 +222,23 @@ describe('processStream', () => { } expect(foundCitationMetadata).to.be.true; }); + it('removes empty text parts', async () => { + const fakeResponse = getMockResponseStreaming( + 'streaming-success-empty-text-part.txt' + ); + const result = processStream(fakeResponse as Response); + const aggregatedResponse = await result.response; + expect(aggregatedResponse.text()).to.equal('1'); + expect(aggregatedResponse.candidates?.length).to.equal(1); + expect(aggregatedResponse.candidates?.[0].content.parts.length).to.equal(1); + + // The chunk with the empty text part will still go through the stream + let numChunks = 0; + for await (const _ of result.stream) { + numChunks++; + } + expect(numChunks).to.equal(2); + }); }); describe('aggregateResponses', () => { @@ -404,3 +423,104 @@ describe('aggregateResponses', () => { }); }); }); + +describe('deleteEmptyTextParts', () => { + it('removes empty text parts from a single candidate', () => { + const parts: Part[] = [ + { + text: '' + }, + { + text: 'foo' + } + ]; + const generateContentResponse: GenerateContentResponse = { + candidates: [ + { + index: 0, + content: { + role: 'model', + parts + } + } + ] + }; + + deleteEmptyTextParts(generateContentResponse); + expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( + [ + { + text: 'foo' + } + ] + ); + }); + it('removes empty text parts from all candidates', () => { + const parts: Part[] = [ + { + text: '' + }, + { + text: 'foo' + } + ]; + const generateContentResponse: GenerateContentResponse = { + candidates: [ + { + index: 0, + content: { + role: 'model', + parts + } + }, + { + index: 1, + content: { + role: 'model', + parts + } + } + ] + }; + + deleteEmptyTextParts(generateContentResponse); + expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( + [ + { + text: 'foo' + } + ] + ); + expect(generateContentResponse.candidates?.[1].content.parts).to.deep.equal( + [ + { + text: 'foo' + } + ] + ); + }); + it('does not remove candidate even if all parts are removed', () => { + const parts: Part[] = [ + { + text: '' + } + ]; + const generateContentResponse: GenerateContentResponse = { + candidates: [ + { + index: 0, + content: { + role: 'model', + parts + } + } + ] + }; + + deleteEmptyTextParts(generateContentResponse); + expect(generateContentResponse.candidates?.length).to.equal(1); + expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( + [] + ); + }); +}); diff --git a/packages/vertexai/test-utils/mock-response.ts b/packages/vertexai/test-utils/mock-response.ts index 8332d9eb36e..747ac4dec7d 100644 --- a/packages/vertexai/test-utils/mock-response.ts +++ b/packages/vertexai/test-utils/mock-response.ts @@ -49,6 +49,9 @@ export function getMockResponseStreaming( filename: string, chunkLength: number = 20 ): Partial { + if (!(filename in mocksLookup)) { + throw Error(`Mock response file not found: '${filename}'`); + } const fullText = mocksLookup[filename]; return { From faf4df6bd865403a9febc2f7ecb03afbf0e1c485 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 29 Jan 2025 13:55:42 -0500 Subject: [PATCH 06/10] dont throw if mock response file isn't found --- packages/vertexai/test-utils/mock-response.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/vertexai/test-utils/mock-response.ts b/packages/vertexai/test-utils/mock-response.ts index 747ac4dec7d..8332d9eb36e 100644 --- a/packages/vertexai/test-utils/mock-response.ts +++ b/packages/vertexai/test-utils/mock-response.ts @@ -49,9 +49,6 @@ export function getMockResponseStreaming( filename: string, chunkLength: number = 20 ): Partial { - if (!(filename in mocksLookup)) { - throw Error(`Mock response file not found: '${filename}'`); - } const fullText = mocksLookup[filename]; return { From 043cfad37ff843886a6580c738239c95c92b8a00 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 29 Jan 2025 13:58:23 -0500 Subject: [PATCH 07/10] update responses version to 6 --- scripts/update_vertexai_responses.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/update_vertexai_responses.sh b/scripts/update_vertexai_responses.sh index 101eac90d9f..20b9082861e 100755 --- a/scripts/update_vertexai_responses.sh +++ b/scripts/update_vertexai_responses.sh @@ -17,7 +17,7 @@ # This script replaces mock response files for Vertex AI unit tests with a fresh # clone of the shared repository of Vertex AI test data. -RESPONSES_VERSION='v5.*' # The major version of mock responses to use +RESPONSES_VERSION='v6.*' # The major version of mock responses to use REPO_NAME="vertexai-sdk-test-data" REPO_LINK="https://github.com/FirebaseExtended/$REPO_NAME.git" From 634cb0c4fa405bbd2be44137f1f55d16f770f8bf Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Fri, 31 Jan 2025 12:16:21 -0500 Subject: [PATCH 08/10] Only ignore empty text parts in the aggregated response. --- .../src/requests/stream-reader.test.ts | 104 +----------------- .../vertexai/src/requests/stream-reader.ts | 28 ++--- 2 files changed, 8 insertions(+), 124 deletions(-) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index b2ab735f62d..88e566683ae 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -17,7 +17,6 @@ import { aggregateResponses, - deleteEmptyTextParts, getResponseStream, processStream } from './stream-reader'; @@ -34,7 +33,6 @@ import { GenerateContentResponse, HarmCategory, HarmProbability, - Part, SafetyRating } from '../types'; @@ -228,6 +226,7 @@ describe('processStream', () => { ); const result = processStream(fakeResponse as Response); const aggregatedResponse = await result.response; + console.log(aggregatedResponse.candidates?.[0].content.parts); expect(aggregatedResponse.text()).to.equal('1'); expect(aggregatedResponse.candidates?.length).to.equal(1); expect(aggregatedResponse.candidates?.[0].content.parts.length).to.equal(1); @@ -423,104 +422,3 @@ describe('aggregateResponses', () => { }); }); }); - -describe('deleteEmptyTextParts', () => { - it('removes empty text parts from a single candidate', () => { - const parts: Part[] = [ - { - text: '' - }, - { - text: 'foo' - } - ]; - const generateContentResponse: GenerateContentResponse = { - candidates: [ - { - index: 0, - content: { - role: 'model', - parts - } - } - ] - }; - - deleteEmptyTextParts(generateContentResponse); - expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( - [ - { - text: 'foo' - } - ] - ); - }); - it('removes empty text parts from all candidates', () => { - const parts: Part[] = [ - { - text: '' - }, - { - text: 'foo' - } - ]; - const generateContentResponse: GenerateContentResponse = { - candidates: [ - { - index: 0, - content: { - role: 'model', - parts - } - }, - { - index: 1, - content: { - role: 'model', - parts - } - } - ] - }; - - deleteEmptyTextParts(generateContentResponse); - expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( - [ - { - text: 'foo' - } - ] - ); - expect(generateContentResponse.candidates?.[1].content.parts).to.deep.equal( - [ - { - text: 'foo' - } - ] - ); - }); - it('does not remove candidate even if all parts are removed', () => { - const parts: Part[] = [ - { - text: '' - } - ]; - const generateContentResponse: GenerateContentResponse = { - candidates: [ - { - index: 0, - content: { - role: 'model', - parts - } - } - ] - }; - - deleteEmptyTextParts(generateContentResponse); - expect(generateContentResponse.candidates?.length).to.equal(1); - expect(generateContentResponse.candidates?.[0].content.parts).to.deep.equal( - [] - ); - }); -}); diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index ac8d6415a42..4266bcc1753 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -63,7 +63,6 @@ async function getResponsePromise( return enhancedResponse; } - deleteEmptyTextParts(value); allResponses.push(value); } } @@ -78,7 +77,6 @@ async function* generateResponseSequence( break; } - deleteEmptyTextParts(value); const enhancedResponse = createEnhancedContentResponse(value); yield enhancedResponse; } @@ -187,7 +185,13 @@ export function aggregateResponses( } const newPart: Partial = {}; for (const part of candidate.content.parts) { - if (part.text) { + if (part.text !== undefined) { + // The backend can send empty text parts. If these are sent back + // (e.g. in chat history), the backend will respond with an error. + // To prevent this, ignore empty text parts. + if (part.text === '') { + continue; + } newPart.text = part.text; } if (part.functionCall) { @@ -206,21 +210,3 @@ export function aggregateResponses( } return aggregatedResponse; } - -/** - * The backend can send empty text parts, but if they are sent back (e.g. in a chat history) there - * will be an error. To prevent this, filter out the empty text part from responses. - * - * See: https://github.com/firebase/firebase-js-sdk/issues/8714 - */ -export function deleteEmptyTextParts(response: GenerateContentResponse): void { - if (response.candidates) { - response.candidates.forEach(candidate => { - if (candidate.content && candidate.content.parts) { - candidate.content.parts = candidate.content.parts.filter( - part => part.text !== '' - ); - } - }); - } -} From 86827d1da85d478ec5a3809e0c94a1f880a816a5 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Mon, 3 Feb 2025 14:04:16 -0500 Subject: [PATCH 09/10] review fixes --- packages/vertexai/src/requests/stream-reader.test.ts | 1 - packages/vertexai/src/requests/stream-reader.ts | 3 --- 2 files changed, 4 deletions(-) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index 88e566683ae..64d01bc5a48 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -226,7 +226,6 @@ describe('processStream', () => { ); const result = processStream(fakeResponse as Response); const aggregatedResponse = await result.response; - console.log(aggregatedResponse.candidates?.[0].content.parts); expect(aggregatedResponse.text()).to.equal('1'); expect(aggregatedResponse.candidates?.length).to.equal(1); expect(aggregatedResponse.candidates?.[0].content.parts.length).to.equal(1); diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index 4266bcc1753..f824a0567be 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -197,9 +197,6 @@ export function aggregateResponses( if (part.functionCall) { newPart.functionCall = part.functionCall; } - if (Object.keys(newPart).length === 0) { - newPart.text = ''; - } aggregatedResponse.candidates[i].content.parts.push( newPart as Part ); From 0c1098466833c721c64555b00d3364e89e6f1c81 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Fri, 7 Feb 2025 16:18:42 -0500 Subject: [PATCH 10/10] Throw in `aggregateResponses` if `newPart` has no properties --- .../src/requests/stream-reader.test.ts | 49 ++++++++++++++++++- .../vertexai/src/requests/stream-reader.ts | 7 +++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/vertexai/src/requests/stream-reader.test.ts b/packages/vertexai/src/requests/stream-reader.test.ts index 64d01bc5a48..b68c2423066 100644 --- a/packages/vertexai/src/requests/stream-reader.test.ts +++ b/packages/vertexai/src/requests/stream-reader.test.ts @@ -33,8 +33,10 @@ import { GenerateContentResponse, HarmCategory, HarmProbability, - SafetyRating + SafetyRating, + VertexAIErrorCode } from '../types'; +import { VertexAIError } from '../errors'; use(sinonChai); @@ -420,4 +422,49 @@ describe('aggregateResponses', () => { ).to.equal(150); }); }); + + it('throws if a part has no properties', () => { + const responsesToAggregate: GenerateContentResponse[] = [ + { + candidates: [ + { + index: 0, + content: { + role: 'user', + parts: [{} as any] // Empty + }, + finishReason: FinishReason.STOP, + finishMessage: 'something', + safetyRatings: [ + { + category: HarmCategory.HARM_CATEGORY_HARASSMENT, + probability: HarmProbability.NEGLIGIBLE + } as SafetyRating + ] + } + ], + promptFeedback: { + blockReason: BlockReason.SAFETY, + safetyRatings: [ + { + category: HarmCategory.HARM_CATEGORY_DANGEROUS_CONTENT, + probability: HarmProbability.LOW + } as SafetyRating + ] + } + } + ]; + + try { + aggregateResponses(responsesToAggregate); + } catch (e) { + expect((e as VertexAIError).code).includes( + VertexAIErrorCode.INVALID_CONTENT + ); + expect((e as VertexAIError).message).to.include( + 'Part should have at least one property, but there are none. This is likely caused ' + + 'by a malformed response from the backend.' + ); + } + }); }); diff --git a/packages/vertexai/src/requests/stream-reader.ts b/packages/vertexai/src/requests/stream-reader.ts index f824a0567be..5c419d114e0 100644 --- a/packages/vertexai/src/requests/stream-reader.ts +++ b/packages/vertexai/src/requests/stream-reader.ts @@ -197,6 +197,13 @@ export function aggregateResponses( if (part.functionCall) { newPart.functionCall = part.functionCall; } + if (Object.keys(newPart).length === 0) { + throw new VertexAIError( + VertexAIErrorCode.INVALID_CONTENT, + 'Part should have at least one property, but there are none. This is likely caused ' + + 'by a malformed response from the backend.' + ); + } aggregatedResponse.candidates[i].content.parts.push( newPart as Part );