Skip to content
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

fix(Auto-fixing Output Parser Node): Only run retry chain on parsing errors #11569

Merged
merged 5 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,28 @@ export async function toolsAgentExecute(this: IExecuteFunctions): Promise<INodeE
// If the steps are an AgentFinish and the outputParser is defined it must mean that the LLM didn't use `format_final_response` tool so we will try to parse the output manually
if (outputParser && typeof steps === 'object' && (steps as AgentFinish).returnValues) {
const finalResponse = (steps as AgentFinish).returnValues;
const returnValues = (await outputParser.parse(finalResponse as unknown as string)) as Record<
string,
unknown
>;
let parserInput: string;

if (finalResponse instanceof Object) {
if ('output' in finalResponse) {
try {
// If the output is an object, we will try to parse it as JSON
// this is because parser expects stringified JSON object like { "output": { .... } }
// so we try to parse the output before wrapping it and then stringify it
parserInput = JSON.stringify({ output: jsonParse(finalResponse.output) });
} catch (error) {
// If parsing of the output fails, we will use the raw output
parserInput = finalResponse.output;
}
} else {
// If the output is not an object, we will stringify it as it is
parserInput = JSON.stringify(finalResponse);
}
} else {
parserInput = finalResponse;
}

const returnValues = (await outputParser.parse(parserInput)) as Record<string, unknown>;
return handleParsedStepOutput(returnValues);
}
return handleAgentFinishOutput(steps);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import type { BaseLanguageModel } from '@langchain/core/language_models/base';
import { NodeConnectionType } from 'n8n-workflow';
import { PromptTemplate } from '@langchain/core/prompts';
import { NodeConnectionType, NodeOperationError } from 'n8n-workflow';
import type {
ISupplyDataFunctions,
INodeType,
INodeTypeDescription,
SupplyData,
} from 'n8n-workflow';

import { NAIVE_FIX_PROMPT } from './prompt';
import {
N8nOutputFixingParser,
type N8nStructuredOutputParser,
Expand Down Expand Up @@ -65,6 +67,27 @@ export class OutputParserAutofixing implements INodeType {
default: '',
},
getConnectionHintNoticeField([NodeConnectionType.AiChain, NodeConnectionType.AiAgent]),
{
displayName: 'Options',
name: 'options',
type: 'collection',
placeholder: 'Add Option',
default: {},
options: [
{
displayName: 'Retry Prompt',
name: 'prompt',
type: 'string',
default: NAIVE_FIX_PROMPT,
typeOptions: {
rows: 10,
},
hint: 'Should include "{error}", "{instructions}", and "{completion}" placeholders',
description:
'Prompt template used for fixing the output. Uses placeholders: "{instructions}" for parsing rules, "{completion}" for the failed attempt, and "{error}" for the validation error message.',
},
],
},
],
};

Expand All @@ -77,8 +100,20 @@ export class OutputParserAutofixing implements INodeType {
NodeConnectionType.AiOutputParser,
itemIndex,
)) as N8nStructuredOutputParser;
const prompt = this.getNodeParameter('options.prompt', itemIndex, NAIVE_FIX_PROMPT) as string;

const parser = new N8nOutputFixingParser(this, model, outputParser);
if (prompt.length === 0 || !prompt.includes('{error}')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the other placeholders (instructions, completion) not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how much we should restrict this; perhaps users would use it with fully custom prompts that provided the completion from other parts of the workflow. But of course, with the default prompt, providing the other two placeholders is semi-required, so it'll work more reliably.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it doesn't error, apparently, but it does not yield good results, it looks like (removed the "instructions" bit, but that left an empty object, instead of something that actually matches the output parser...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add these placeholders as a hint underneath the prompt-window?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Did that + added more descriptive description
CleanShot 2024-11-06 at 12 10 09

throw new NodeOperationError(
this.getNode(),
'Auto-fixing parser prompt has to contain {error} placeholder',
);
}
const parser = new N8nOutputFixingParser(
this,
model,
outputParser,
PromptTemplate.fromTemplate(prompt),
);

return {
response: parser,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export const NAIVE_FIX_PROMPT = `Instructions:
--------------
{instructions}
--------------
Completion:
--------------
{completion}
--------------
Above, the Completion did not satisfy the constraints given in the Instructions.
Error:
--------------
{error}
--------------
Please try again. Please only respond with an answer that satisfies the constraints laid out in the Instructions:`;
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
/* eslint-disable @typescript-eslint/unbound-method */
/* eslint-disable @typescript-eslint/no-unsafe-call */
import type { BaseLanguageModel } from '@langchain/core/language_models/base';
import { OutputParserException } from '@langchain/core/output_parsers';
import type { MockProxy } from 'jest-mock-extended';
import { mock } from 'jest-mock-extended';
import { normalizeItems } from 'n8n-core';
import type { IExecuteFunctions, IWorkflowDataProxyData } from 'n8n-workflow';
import { ApplicationError, NodeConnectionType } from 'n8n-workflow';
import { ApplicationError, NodeConnectionType, NodeOperationError } from 'n8n-workflow';

import { N8nOutputFixingParser } from '../../../../utils/output_parsers/N8nOutputParser';
import type { N8nStructuredOutputParser } from '../../../../utils/output_parsers/N8nOutputParser';
import type {
N8nOutputFixingParser,
N8nStructuredOutputParser,
} from '../../../../utils/output_parsers/N8nOutputParser';
import { OutputParserAutofixing } from '../OutputParserAutofixing.node';
import { NAIVE_FIX_PROMPT } from '../prompt';

describe('OutputParserAutofixing', () => {
let outputParser: OutputParserAutofixing;
Expand All @@ -34,6 +38,13 @@ describe('OutputParserAutofixing', () => {

throw new ApplicationError('Unexpected connection type');
});
thisArg.getNodeParameter.mockReset();
thisArg.getNodeParameter.mockImplementation((parameterName) => {
if (parameterName === 'options.prompt') {
return NAIVE_FIX_PROMPT;
}
throw new ApplicationError('Not implemented');
});
});

afterEach(() => {
Expand All @@ -48,73 +59,132 @@ describe('OutputParserAutofixing', () => {
});
}

it('should successfully parse valid output without needing to fix it', async () => {
const validOutput = { name: 'Alice', age: 25 };

mockStructuredOutputParser.parse.mockResolvedValueOnce(validOutput);
describe('Configuration', () => {
it('should throw error when prompt template does not contain {error} placeholder', async () => {
thisArg.getNodeParameter.mockImplementation((parameterName) => {
if (parameterName === 'options.prompt') {
return 'Invalid prompt without error placeholder';
}
throw new ApplicationError('Not implemented');
});

await expect(outputParser.supplyData.call(thisArg, 0)).rejects.toThrow(
new NodeOperationError(
thisArg.getNode(),
'Auto-fixing parser prompt has to contain {error} placeholder',
),
);
});

const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};
it('should throw error when prompt template is empty', async () => {
thisArg.getNodeParameter.mockImplementation((parameterName) => {
if (parameterName === 'options.prompt') {
return '';
}
throw new ApplicationError('Not implemented');
});

await expect(outputParser.supplyData.call(thisArg, 0)).rejects.toThrow(
new NodeOperationError(
thisArg.getNode(),
'Auto-fixing parser prompt has to contain {error} placeholder',
),
);
});

// Ensure the response contains the output-fixing parser
expect(response).toBeDefined();
expect(response).toBeInstanceOf(N8nOutputFixingParser);
it('should use default prompt when none specified', async () => {
thisArg.getNodeParameter.mockImplementation((parameterName) => {
if (parameterName === 'options.prompt') {
return NAIVE_FIX_PROMPT;
}
throw new ApplicationError('Not implemented');
});

const result = await response.parse('{"name": "Alice", "age": 25}');
const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

// Validate that the parser succeeds without retry
expect(result).toEqual(validOutput);
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(1); // Only one call to parse
expect(response).toBeDefined();
});
});

it('should throw an error when both structured parser and fixing parser fail', async () => {
mockStructuredOutputParser.parse
.mockRejectedValueOnce(new Error('Invalid JSON')) // First attempt fails
.mockRejectedValueOnce(new Error('Fixing attempt failed')); // Second attempt fails
describe('Parsing', () => {
it('should successfully parse valid output without needing to fix it', async () => {
const validOutput = { name: 'Alice', age: 25 };

const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};
mockStructuredOutputParser.parse.mockResolvedValueOnce(validOutput);

response.getRetryChain = getMockedRetryChain('{}');
const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

await expect(response.parse('Invalid JSON string')).rejects.toThrow('Fixing attempt failed');
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(2);
});
const result = await response.parse('{"name": "Alice", "age": 25}');

it('should reject on the first attempt and succeed on retry with the parsed content', async () => {
const validOutput = { name: 'Bob', age: 28 };
expect(result).toEqual(validOutput);
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(1);
});

mockStructuredOutputParser.parse.mockRejectedValueOnce(new Error('Invalid JSON'));
it('should not retry on non-OutputParserException errors', async () => {
const error = new Error('Some other error');
mockStructuredOutputParser.parse.mockRejectedValueOnce(error);

const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};
const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

response.getRetryChain = getMockedRetryChain(JSON.stringify(validOutput));
await expect(response.parse('Invalid JSON string')).rejects.toThrow(error);
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(1);
});

mockStructuredOutputParser.parse.mockResolvedValueOnce(validOutput);
it('should retry on OutputParserException and succeed', async () => {
const validOutput = { name: 'Bob', age: 28 };

const result = await response.parse('Invalid JSON string');
mockStructuredOutputParser.parse
.mockRejectedValueOnce(new OutputParserException('Invalid JSON'))
.mockResolvedValueOnce(validOutput);

expect(result).toEqual(validOutput);
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(2); // First fails, second succeeds
});
const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

it('should handle non-JSON formatted response from fixing parser', async () => {
mockStructuredOutputParser.parse.mockRejectedValueOnce(new Error('Invalid JSON'));
response.getRetryChain = getMockedRetryChain(JSON.stringify(validOutput));

const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};
const result = await response.parse('Invalid JSON string');

response.getRetryChain = getMockedRetryChain('This is not JSON');
expect(result).toEqual(validOutput);
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(2);
});

mockStructuredOutputParser.parse.mockRejectedValueOnce(new Error('Unexpected token'));
it('should handle failed retry attempt', async () => {
mockStructuredOutputParser.parse
.mockRejectedValueOnce(new OutputParserException('Invalid JSON'))
.mockRejectedValueOnce(new Error('Still invalid JSON'));

// Expect the structured parser to throw an error on invalid JSON from retry
await expect(response.parse('Invalid JSON string')).rejects.toThrow('Unexpected token');
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(2); // First fails, second tries and fails
const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

response.getRetryChain = getMockedRetryChain('Still not valid JSON');

await expect(response.parse('Invalid JSON string')).rejects.toThrow('Still invalid JSON');
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(2);
});

it('should throw non-OutputParserException errors immediately without retry', async () => {
const customError = new Error('Database connection error');
const retryChainSpy = jest.fn();

mockStructuredOutputParser.parse.mockRejectedValueOnce(customError);

const { response } = (await outputParser.supplyData.call(thisArg, 0)) as {
response: N8nOutputFixingParser;
};

response.getRetryChain = retryChainSpy;

await expect(response.parse('Some input')).rejects.toThrow('Database connection error');
expect(mockStructuredOutputParser.parse).toHaveBeenCalledTimes(1);
expect(retryChainSpy).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import type { Callbacks } from '@langchain/core/callbacks/manager';
import type { BaseLanguageModel } from '@langchain/core/language_models/base';
import type { AIMessage } from '@langchain/core/messages';
import { BaseOutputParser } from '@langchain/core/output_parsers';
import { BaseOutputParser, OutputParserException } from '@langchain/core/output_parsers';
import type { PromptTemplate } from '@langchain/core/prompts';
import type { ISupplyDataFunctions } from 'n8n-workflow';
import { NodeConnectionType } from 'n8n-workflow';

import type { N8nStructuredOutputParser } from './N8nStructuredOutputParser';
import { NAIVE_FIX_PROMPT } from './prompt';
import { logAiEvent } from '../helpers';

export class N8nOutputFixingParser extends BaseOutputParser {
Expand All @@ -16,12 +16,13 @@ export class N8nOutputFixingParser extends BaseOutputParser {
private context: ISupplyDataFunctions,
private model: BaseLanguageModel,
private outputParser: N8nStructuredOutputParser,
private fixPromptTemplate: PromptTemplate,
) {
super();
}

getRetryChain() {
return NAIVE_FIX_PROMPT.pipe(this.model);
return this.fixPromptTemplate.pipe(this.model);
}

/**
Expand All @@ -47,11 +48,14 @@ export class N8nOutputFixingParser extends BaseOutputParser {

return response;
} catch (error) {
if (!(error instanceof OutputParserException)) {
throw error;
}
try {
// Second attempt: use retry chain to fix the output
const result = (await this.getRetryChain().invoke({
completion,
error,
error: error.message,
instructions: this.getFormatInstructions(),
})) as AIMessage;

Expand Down
Loading