feat(AI Transform Node): Reduce payload size #11965
Conversation
michael-radency
added
node/improvement
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
…improve-error-handling-request-entity-too-large-error
| return Math.ceil(JSON.stringify(item).length / averageTokenLength); | ||
| }; | ||
|
|
||
| export function reducePayloadSizeOrThrow( |
There was a problem hiding this comment.
Can we add JSDoc comment on what this function does?
| payload: AskAiRequest.RequestPayload, | ||
| error: Error, | ||
| averageTokenLength = 4, | ||
| ) { |
There was a problem hiding this comment.
This function is too long and has high cyclomatic complexity. Let's please split it in parts
| question: 'What is node1 and prop1?', | ||
| }) as unknown as AskAiRequest.RequestPayload; | ||
|
|
||
| describe('reducePayloadSizeOrThrow', () => { |
…improve-error-handling-request-entity-too-large-error
|
thanks for review @tomi, updated as suggested |
| const maxTokens = parseInt(tokens[0]); | ||
| const currentTokens = parseInt(tokens[1]); |
There was a problem hiding this comment.
We should explicitly define the radix, as otherwise it might get detected to something else
| const maxTokens = parseInt(tokens[0]); | |
| const currentTokens = parseInt(tokens[1]); | |
| const maxTokens = parseInt(tokens[0], 10); | |
| const currentTokens = parseInt(tokens[1], 10); |
| }; | ||
|
|
||
| const calculateRemainingTokens = (error: Error) => { | ||
| const tokens = error.message.match(/\d+/g); |
There was a problem hiding this comment.
We are assuming the error message contains two integer numbers. Could we have a comment telling what the expected error message would look like?
Also, is there a chance that some other error would be thrown that could contain 2 numbers?
There was a problem hiding this comment.
updated as suggested, regarding other messages - we are checking message to contain specific subsisting 'maximum context length'
| @@ -57,6 +57,128 @@ export function getSchemas() { | |||
| }; | |||
| } | |||
|
|
|||
| //------ Reduce payload ------ | |||
|
|
|||
| const calculateTokens = (item: object, averageTokenLength: number): number => { | |||
There was a problem hiding this comment.
Better to use unknown than object. Also since the calculation is not precise but only an estimation, we could make that explicit
| const calculateTokens = (item: object, averageTokenLength: number): number => { | |
| const estimateNumberOfTokens = (item: unknown, averageTokenLength: number): number => { |
…improve-error-handling-request-entity-too-large-error
…improve-error-handling-request-entity-too-large-error
|
thanks again @tomi , updated as suggested |
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
node-1866-improve-error-handling-request-entity-too-large-error
|
| Run status |
|
| Run duration | 04m 30s |
| Commit |
|
| Committer | Michael Kret |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
480
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
michael-radency
deleted the
node-1866-improve-error-handling-request-entity-too-large-error
branch
|
Got released with |
Summary
if request returned 'maximum context length' error reduce payload size and try again
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-1866/improve-error-handling-request-entity-too-large-error