Update ragknowledge.ts #2528
Update ragknowledge.ts #2528
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/core/src/ragknowledge.tsOops! Something went wrong! :( ESLint: 9.18.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 📝 WalkthroughWalkthroughThe pull request modifies the Changes
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/ragknowledge.ts (2)
Line range hint
550-591: Enhance error handling for batch processing.The batch processing implementation looks good, but could benefit from more robust error handling for individual batch failures.
Consider this enhancement:
for (let i = 0; i < chunks.length; i += BATCH_SIZE) { const batchStart = Date.now(); const batch = chunks.slice( i, Math.min(i + BATCH_SIZE, chunks.length) ); - // Process embeddings in parallel - const embeddings = await Promise.all( - batch.map((chunk) => embed(this.runtime, chunk)) - ); + // Process embeddings in parallel with error handling + const embeddings = await Promise.allSettled( + batch.map((chunk) => embed(this.runtime, chunk)) + ); + + // Filter out failed embeddings + const successfulEmbeddings = embeddings + .filter((result): result is PromiseFulfilledResult<number[]> => + result.status === 'fulfilled') + .map(result => result.value); + + if (successfulEmbeddings.length < batch.length) { + elizaLogger.warn( + `Failed to process ${batch.length - successfulEmbeddings.length} chunks in batch` + ); + }
Line range hint
550-591: Consider adding progress metrics.The batch processing implementation would benefit from performance metrics to help tune the batch size.
Add metrics collection:
+ let totalBatchTime = 0; + let totalEmbeddingTime = 0; + let totalStorageTime = 0; + for (let i = 0; i < chunks.length; i += BATCH_SIZE) { const batchStart = Date.now(); + const embeddingStart = Date.now(); const embeddings = await Promise.all( batch.map((chunk) => embed(this.runtime, chunk)) ); + totalEmbeddingTime += Date.now() - embeddingStart; + const storageStart = Date.now(); await Promise.all(/* ... */); + totalStorageTime += Date.now() - storageStart; + totalBatchTime += Date.now() - batchStart; } + + elizaLogger.info( + `Performance metrics - ` + + `Avg embedding time: ${(totalEmbeddingTime/totalChunks).toFixed(2)}ms per chunk, ` + + `Avg storage time: ${(totalStorageTime/totalChunks).toFixed(2)}ms per chunk` + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/ragknowledge.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/ragknowledge.ts (1)
550-550: Consider memory implications of increased chunk parameters.The changes to chunk parameters (size: 512→1500, count: 20→100) significantly increase memory usage. While this preserves more context, it might impact performance on resource-constrained environments.
Let's analyze the potential impact:
|
can you specify why you need this change and benefits? reopen if you still think its valuable |
Relates to
Rag improvements
Risks
Low
Background
synchronise chunking & overlap with increased sizing in [generation.ts]
(https://github.com/elizaOS/eliza/pull/2525/files#diff-b68254579d690900478f82405c1f5e9eab387a8637ffddaacbfadb2ca31369da)
What does this PR do?
What kind of change is this?
Improvements (misc. changes to existing features)
synchronise chunking & overlap with increased sizing in [generation.ts]
Documentation changes needed?
My changes do not require a change to the project documentation
Testing
Where should a reviewer start?
Detailed testing steps
Summary by CodeRabbit