refactor(Basic LLM Chain Node): Refactor Basic LLM Chain & add tests #13850
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
| noDataExpression: true, | ||
| displayOptions: { | ||
| hide: { | ||
| '@version': [1, 1.1, 1.3], |
There was a problem hiding this comment.
Is 1.2 skipped intentionally here?
| name: 'AI', | ||
| value: AIMessagePromptTemplate.lc_name(), | ||
| }, | ||
| { | ||
| name: 'System', | ||
| value: SystemMessagePromptTemplate.lc_name(), | ||
| }, | ||
| { | ||
| name: 'User', | ||
| value: HumanMessagePromptTemplate.lc_name(), | ||
| }, | ||
| ], | ||
| default: SystemMessagePromptTemplate.lc_name(), |
There was a problem hiding this comment.
Using LangChain constants for values might break nodes in the future, if LangChain for some reason decides to change them and we update the langchain dependency. As users will have old values stored as node props in their workflows.
There was a problem hiding this comment.
But I see now that was already the case before refactoring, please disregard :)
| }); | ||
|
|
||
| // Ensure response is always returned as an array | ||
| return [response]; |
There was a problem hiding this comment.
Does it make sense to use the same isArray check here, as in executeChain?
| }; | ||
|
|
||
| describe('imageUtils', () => { | ||
| // The dataUriFromImageData tests are already covered in the existing utils.test.ts |
There was a problem hiding this comment.
Not sure I understand this comment, can you elaborate? utils.test.ts is deleted in this PR
| } | ||
|
|
||
| // If multiple parsers, combine them | ||
| return new CombiningOutputParser(...outputParsers) as unknown as N8nOutputParser; |
There was a problem hiding this comment.
I found this use case a bit unreliable (something is off, at least on a logs panel). I understand it was already there before. Just wonder do we need to support this at all? Can we remove it and limit with a single parser? Seems like super-rare use-case.
Summary
chainExecutor.ts,promptUtils.ts,imageUtils.ts,responseFormatter.ts) under a newmethodsdirectory.ChainLlm.node.ts) to use these utility modules.chainExecutor,promptUtils,imageUtils,responseFormatter).Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)