feat(backend): enhance config loader with embedding support and impro… #74
Conversation
|
Caution Review failedThe pull request is closed. 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
backend/src/config/common-path.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-prettier". (The package "eslint-plugin-prettier" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-prettier" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a comprehensive refactoring of the configuration management, model downloading, and embedding systems across the backend and server components. The changes focus on creating a more robust, flexible, and manageable approach to handling model configurations, downloads, and embeddings. Key improvements include implementing singleton patterns, enhancing error handling, introducing status tracking for model downloads, and providing more structured interfaces for configuration and model management. Changes
Sequence DiagramsequenceDiagram
participant ConfigLoader
participant UniversalDownloader
participant UniversalStatusManager
participant EmbeddingDownloader
ConfigLoader->>ConfigLoader: Initialize Configuration
ConfigLoader->>UniversalDownloader: Request Model Download
UniversalDownloader->>UniversalStatusManager: Check Download Status
UniversalStatusManager-->>UniversalDownloader: Return Status
UniversalDownloader->>EmbeddingDownloader: Download Embedding Models
EmbeddingDownloader->>UniversalStatusManager: Update Download Status
UniversalDownloader->>UniversalStatusManager: Update Chat Model Status
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Nitpick comments (4)
backend/src/config/config-loader.ts (2)
88-108: Avoid blocking the event loop with synchronous file operationsThe use of synchronous file system methods like
fs.readFileSyncandfs.writeFileSynccan block the event loop, impacting the performance of the application. Consider using asynchronous versions (fs.readFile,fs.writeFile) with Promises orasync/awaitto improve responsiveness.Example refactoring:
-private loadConfig() { +private async loadConfig() { try { - const file = fs.readFileSync(this.configPath, 'utf-8'); + const file = await fs.promises.readFile(this.configPath, 'utf-8'); ... - fs.writeFileSync(this.configPath, exampleConfigContent, 'utf-8'); + await fs.promises.writeFile(this.configPath, exampleConfigContent, 'utf-8');
110-114: Improve type safety in thegetmethodUsing double type assertion
as unknown as Tcan bypass type checking and may lead to runtime errors. To enhance type safety, consider definingAppConfigmore precisely or adjusting the method to safely cast the configuration.Example adjustment:
-get<T>(path?: string): T { +get<T = AppConfig>(path?: string): T { if (!path) { - return this.config as unknown as T; + return this.config as T; } return _.get(this.config, path) as T; }backend/src/model/utils.ts (2)
7-7: Complete the TODO comment for clarityThe TODO comment appears incomplete:
// TODO: verify or download embedding model to. Please provide a clear description of the intended task to ensure it is properly addressed in the future.
12-12: Adjust logging statements to correctly format messagesThe
Logger.logmethod accepts a single string message. Passing multiple arguments may not produce the expected output. Please format the messages to include variable values.Update line 12:
-logger.log('Loaded config:', chats); +logger.log(`Loaded config: ${JSON.stringify(chats)}`);Update line 17:
-downloader.logger.log(model, task); +downloader.logger.log(`Model: ${model}, Task: ${task}`);Also applies to: 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/config/config-loader.spec.ts(1 hunks)backend/src/config/config-loader.ts(2 hunks)backend/src/model/utils.ts(1 hunks)
🔇 Additional comments (1)
backend/src/config/config-loader.spec.ts (1)
1-242: Well-structured and comprehensive test suite
The unit tests are thorough and cover various scenarios for the ConfigLoader class, ensuring robust validation of its functionality.
backend/src/config/config-loader.ts
Outdated
| const jsonContent = file.replace( | ||
| /\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g, | ||
| (m, g) => (g ? '' : m), | ||
| ); | ||
| this.config = JSON.parse(jsonContent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a library to parse JSON with comments
The custom regex used to strip comments from the JSON content may not cover all edge cases and can be error-prone. Instead of manually handling comments, consider using a library like strip-json-comments or json5 to safely parse JSON files with comments.
Apply this change using strip-json-comments:
+import stripJsonComments from 'strip-json-comments';
...
-const jsonContent = file.replace(
- /\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g,
- (m, g) => (g ? '' : m),
-);
+const jsonContent = stripJsonComments(file);Remember to install the package:
npm install strip-json-comments…ad status and update config structure for chat models
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
backend/src/model/model-status.ts (2)
60-66: Optimize disk writes inupdateStatus()Currently,
updateStatus()callssaveStatus()every time it's invoked, resulting in disk I/O on each status update. IfupdateStatus()is called frequently, this might impact performance. Consider batching updates or implementing a debounce mechanism to reduce the number of disk writes.
72-74: Prevent external mutation of internal state ingetAllStatus()The
getAllStatus()method returns a shallow copy ofthis.status, but theModelStatusobjects within it containDateinstances, which are mutable. Modifying the returnedDateobjects could unintentionally alter the internal state ofModelStatusManager.Consider deep cloning the status data before returning:
return { ...this.status }; + // Deep clone the status data to prevent external mutations + return JSON.parse(JSON.stringify(this.status));backend/src/config/config-loader.spec.ts (2)
90-90: Consider using asynchronous file operations to improve performanceThe use of synchronous file operations like
fs.readFileSyncandfs.writeFileSynccan block the event loop, especially if the configuration file is large or on a slow file system. Refactoring to use asynchronous methods (fs.readFile,fs.writeFile) can enhance application responsiveness and performance.Also applies to: 123-132
54-237: Add unit tests for theConfigLoaderclassUnit tests are crucial for ensuring the reliability of the
ConfigLoaderclass. Since the file is currently namedconfig-loader.spec.ts, it implies the presence of tests. Please add comprehensive unit tests covering all methods, including edge cases and error handling scenarios, to ensure the class behaves as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/config/config-loader.spec.ts(1 hunks)backend/src/config/config-loader.ts(2 hunks)backend/src/model/__tests__/app.e2e-spec.ts(0 hunks)backend/src/model/__tests__/jest-e2e.json(0 hunks)backend/src/model/model-status.ts(1 hunks)backend/src/model/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- backend/src/model/tests/app.e2e-spec.ts
- backend/src/model/tests/jest-e2e.json
🔇 Additional comments (1)
backend/src/config/config-loader.ts (1)
91-95: Consider using a library to parse JSON with comments
The custom regex used to strip comments from the JSON content may not cover all edge cases and can be error-prone. Instead of manually handling comments, consider using a library like strip-json-comments or json5 to safely parse JSON files with comments.
Apply this change using strip-json-comments:
+ import stripJsonComments from 'strip-json-comments';
...
- const jsonContent = file.replace(
- /\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g,
- (m, g) => (g ? '' : m),
- );
+ const jsonContent = stripJsonComments(file);Remember to install the package:
npm install strip-json-comments
backend/src/model/model-status.ts
Outdated
| } catch (error) { | ||
| this.status = {}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log errors when loading model status fails
In the loadStatus() method, errors are silently ignored by initializing this.status = {}; without logging the exception. This can make debugging difficult if there's an issue reading or parsing the status file.
Apply this diff to log the error:
} catch (error) {
- this.status = {};
+ console.error(`Failed to load model status: ${error.message}`);
+ this.status = {};
}This will help in diagnosing issues related to loading the model status.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| this.status = {}; | |
| } | |
| } catch (error) { | |
| console.error(`Failed to load model status: ${error.message}`); | |
| this.status = {}; | |
| } |
backend/src/model/model-status.ts
Outdated
| private saveStatus() { | ||
| const statusDir = path.dirname(this.statusPath); | ||
| if (!fs.existsSync(statusDir)) { | ||
| fs.mkdirSync(statusDir, { recursive: true }); | ||
| } | ||
| fs.writeFileSync( | ||
| this.statusPath, | ||
| JSON.stringify(this.status, null, 2), | ||
| 'utf-8', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for file write operations in saveStatus()
The saveStatus() method writes to the file system without error handling. If an error occurs during fs.writeFileSync, it could cause the application to crash or lead to data loss without any indication.
Consider wrapping the write operation in a try-catch block and logging any errors:
private saveStatus() {
const statusDir = path.dirname(this.statusPath);
if (!fs.existsSync(statusDir)) {
fs.mkdirSync(statusDir, { recursive: true });
}
- fs.writeFileSync(
+ try {
+ fs.writeFileSync(
this.statusPath,
JSON.stringify(this.status, null, 2),
'utf-8',
);
+ } catch (error) {
+ console.error(`Failed to save model status: ${error.message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private saveStatus() { | |
| const statusDir = path.dirname(this.statusPath); | |
| if (!fs.existsSync(statusDir)) { | |
| fs.mkdirSync(statusDir, { recursive: true }); | |
| } | |
| fs.writeFileSync( | |
| this.statusPath, | |
| JSON.stringify(this.status, null, 2), | |
| 'utf-8', | |
| ); | |
| } | |
| private saveStatus() { | |
| const statusDir = path.dirname(this.statusPath); | |
| if (!fs.existsSync(statusDir)) { | |
| fs.mkdirSync(statusDir, { recursive: true }); | |
| } | |
| try { | |
| fs.writeFileSync( | |
| this.statusPath, | |
| JSON.stringify(this.status, null, 2), | |
| 'utf-8', | |
| ); | |
| } catch (error) { | |
| console.error(`Failed to save model status: ${error.message}`); | |
| } | |
| } |
backend/src/model/utils.ts
Outdated
| } catch (error) { | ||
| downloader.logger.error(`Failed to load model ${model}:`, error.message); | ||
| logger.error(`Failed to download model ${model}:`, error.message); |
There was a problem hiding this comment.
Correct the usage of logger.error to include error trace
The logger.error method is used incorrectly by passing error.message as the second argument, which does not align with the expected method signature. The error method accepts an optional stackTrace parameter for including stack traces.
Apply this diff to correct the logging:
- logger.error(`Failed to download model ${model}:`, error.message);
+ logger.error(`Failed to download model ${model}: ${error.message}`, error.stack);This ensures that the error message and stack trace are logged appropriately.
Also applies to: 70-70
backend/src/model/utils.ts
Outdated
| const loadPromises = chats.map(async (chatConfig: ChatConfig) => { | ||
|
|
||
| const chatConfigs = configLoader.getAllChatConfigs(); | ||
| logger.log('Loaded chat configurations:', chatConfigs); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust logging statement to correctly output chat configurations
The logger.log method may not support multiple arguments as intended. To ensure the chat configurations are logged correctly, consider serializing the configurations into the log message.
Apply this diff:
- logger.log('Loaded chat configurations:', chatConfigs);
+ logger.log(`Loaded chat configurations: ${JSON.stringify(chatConfigs)}`);This will include the configurations in the log output as a string.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.log('Loaded chat configurations:', chatConfigs); | |
| logger.log(`Loaded chat configurations: ${JSON.stringify(chatConfigs)}`); |
backend/src/config/config-loader.ts
Outdated
| private saveConfig() { | ||
| const configDir = path.dirname(this.configPath); | ||
| if (!fs.existsSync(configDir)) { | ||
| fs.mkdirSync(configDir, { recursive: true }); | ||
| } | ||
| fs.writeFileSync( | ||
| this.configPath, | ||
| JSON.stringify(this.chatsConfig, null, 4), | ||
| JSON.stringify(this.config, null, 2), | ||
| 'utf-8', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for file write operations in saveConfig()
The saveConfig() method writes to the file system without error handling. If an error occurs during fs.writeFileSync, it could lead to application crashes or silent failures.
Consider wrapping the write operation in a try-catch block and logging any errors:
private saveConfig() {
const configDir = path.dirname(this.configPath);
if (!fs.existsSync(configDir)) {
fs.mkdirSync(configDir, { recursive: true });
}
- fs.writeFileSync(
+ try {
+ fs.writeFileSync(
this.configPath,
JSON.stringify(this.config, null, 2),
'utf-8',
);
+ } catch (error) {
+ console.error(`Failed to save configuration: ${error.message}`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private saveConfig() { | |
| const configDir = path.dirname(this.configPath); | |
| if (!fs.existsSync(configDir)) { | |
| fs.mkdirSync(configDir, { recursive: true }); | |
| } | |
| fs.writeFileSync( | |
| this.configPath, | |
| JSON.stringify(this.chatsConfig, null, 4), | |
| JSON.stringify(this.config, null, 2), | |
| 'utf-8', | |
| ); | |
| } | |
| private saveConfig() { | |
| const configDir = path.dirname(this.configPath); | |
| if (!fs.existsSync(configDir)) { | |
| fs.mkdirSync(configDir, { recursive: true }); | |
| } | |
| try { | |
| fs.writeFileSync( | |
| this.configPath, | |
| JSON.stringify(this.config, null, 2), | |
| 'utf-8', | |
| ); | |
| } catch (error) { | |
| console.error(`Failed to save configuration: ${error.message}`); | |
| } | |
| } |
| const jsonContent = file.replace( | ||
| /\\"|"(?:\\"|[^"])*"|(\/\/.*|\/\*[\s\S]*?\*\/)/g, | ||
| (m, g) => (g ? '' : m), | ||
| ); | ||
| this.config = JSON.parse(jsonContent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using regex to remove comments from JSON
Using regular expressions to strip comments from JSON can be error-prone and may not handle all edge cases correctly. Instead, consider using a JSON parser that supports comments, such as jsonc-parser, or switch to a configuration format that inherently supports comments like YAML or JSON5.
| if ( | ||
| error.code === 'ENOENT' || | ||
| error.message.includes('Unexpected end of JSON input') | ||
| ) { | ||
| this.config = {}; | ||
| this.saveConfig(); | ||
| } else { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent data loss by not overwriting configuration on parse errors
When a JSON parsing error occurs, the current implementation initializes an empty configuration and saves it back to the file (lines 102-104). This can lead to permanent loss of the original configuration data. It's safer to notify the user about the parsing error without overwriting the existing configuration file, allowing them to fix the issue manually.
| @@ -0,0 +1,237 @@ | |||
| // config-loader.ts | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename file to reflect implementation code
The file config-loader.spec.ts is conventionally used for test files, but it contains implementation code for the ConfigLoader class. Consider renaming it to config-loader.ts to accurately represent its contents. Additionally, create a corresponding test file config-loader.spec.ts to include unit tests for the ConfigLoader class.
There was a problem hiding this comment.
Should we consider the attribution like isLocal to separate remote model or local model in chatconfig?
There was a problem hiding this comment.
@NarwhalChen, adding an "isLocal" (or similarly named) attribute in the chat configuration is a reasonable approach if you’d like to distinguish between loading a model from a local path versus relying on a remote API endpoint. This attribute can help clarify configuration options in the code, guide conditional logic (e.g., deciding between file-system checks or network requests), and keep future expansions flexible (for instance, if you add multiple remote endpoints or different local model directories).
If you proceed with this, you could define something like:
interface ChatConfig {
modelName: string;
isDefault?: boolean;
isLocal?: boolean; // Example attribute
// ...other fields
}
That way, when you load or operate on the configuration, you can easily apply different behaviors based on the value of “isLocal.”
| @@ -0,0 +1,237 @@ | |||
| // config-loader.ts | |||
There was a problem hiding this comment.
Should we consider the attribution like isLocal to separate remote model or local model in chatconfig?
backend/src/model/model-status.ts
Outdated
| import * as path from 'path'; | ||
| import { getConfigPath } from 'src/config/common-path'; | ||
|
|
||
| export interface ModelStatus { |
There was a problem hiding this comment.
Could we add modelName: string in ModelStatus？
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
llm-server/src/embedding/openai-embbeding-provider.ts (5)
1-4: Remove unused imports or utilize them.
systemPromptsandChatCompletionMessageParamare imported but never used in this file. Unused imports can clutter the code and cause confusion.-import { systemPrompts } from '../prompt/systemPrompt'; -import { ChatCompletionMessageParam } from 'openai/resources/chat/completions';
23-28: Consider using consistent headers for SSE or custom events.You’re setting
'text/event-embedding'as the content type. While this may be intentional, many SSE services use'text/event-stream'. If this is meant to integrate with standard SSE libraries, consider standardizing the content type.
51-57: DRY principle for header configuration.
getEmbListandgenerateEmbResponseshare very similar response header configuration. Consider refactoring a shared helper to set these headers uniformly, reducing duplication.
59-73: Validate fetch performance and caching approach.Repeatedly calling
this.openai.models.list()may impact performance if triggered frequently without any caching. Assess whether a caching mechanism (in-memory or external) is beneficial.
[performance]
74-85: Improve error reporting specificity.You provide a generic code
'FETCH_MODELS_ERROR'and message'Failed to fetch models'. Consider adding hints or using standard error codes for consumers to handle more accurately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
llm-server/src/embedding/embedding-provider.ts(1 hunks)llm-server/src/embedding/openai-embbeding-provider.ts(1 hunks)
🔇 Additional comments (4)
llm-server/src/embedding/openai-embbeding-provider.ts (3)
9-9: Class naming aligns well with "EmbProvider".
The name OpenAIEmbProvider clearly indicates its purpose and extends the EmbProvider class appropriately.
13-17: Check concurrency or multi-thread access to openai.
If multiple requests initialize embeddings in parallel, ensure that reusing the same openai instance is safe. Typically, the client is thread-safe, but confirm to avoid potential concurrency issues.
30-42: Handle edge cases for large requests or invalid inputs.
If an extremely large message is passed, the code might face performance issues. Consider adding input validation or using streaming with chunked requests to handle large payloads gracefully.
llm-server/src/embedding/embedding-provider.ts (1)
3-3: Abstract class name is coherent.
EmbProvider is straightforward and aligns with the file’s purpose, clarifying that all embedding providers should implement these methods.
| abstract generateEmbResponse( | ||
| params: GenerateMessageParams, | ||
| res: Response, | ||
| ): Promise<void>; | ||
| abstract getEmbList(res: Response): Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revisit the interface to reduce coupling with Express.
Each method directly depends on the Express Response object. Consider returning data (or streams) from these methods instead, so you can remain framework-agnostic and allow the controller layer to handle the response.
…1lboy/codefox into feat-adding-embedding-config
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
backend/src/downloader/universal-status.ts (4)
15-19: Consider asynchronous file operations.Constructors that implicitly perform synchronous I/O (via
loadStatus()) can block the main thread if file operations take a long time. Switching to asynchronous reads might improve performance in larger-scale environments.private constructor(path: string) { this.statusPath = path; - this.loadStatus(); + // Consider an async approach, e.g.: + // await this.loadStatusAsync(); }
31-50: Log errors during status loading.While the try/catch block gracefully defaults the status to an empty object on error, logging the exception can help debug file or JSON parse issues.
} catch (error) { + console.error(`Failed to load status file: ${error}`); this.status = {}; }
52-62: Synchronous file writes could block under heavy load.Similar to the constructor note, consider asynchronous methods for saving status if you expect larger files or many writes.
64-70: Minor parameter naming style.
UniversalNameis capitalized. Typically, parameters use lowerCamelCase (e.g.,universalName), which might be more consistent with TypeScript conventions.backend/src/downloader/__tests__/loadAllChatsModels.spec.ts (1)
70-70: Mock external calls for test speed and reliability (optional).Running real model inference in a test environment can be slow and prone to external failures. Consider mocking or caching responses for unit tests if performance or reliability is an issue.
backend/src/downloader/universal-downloader.ts (1)
16-28: Returningnullon failure might reduce debuggability.In
getPipeline, consider throwing an error or returning a specialized error object. This makes downstream error handling more explicit rather than returningnull.backend/src/downloader/universal-utils.ts (1)
30-36: Warning message references "chat models" only.
logger.warn('No chat models configured')might be misleading iftypeisEMBEDDINGS. Consider adjusting the message to reflect the actual config type to avoid confusion.if (!chatConfigs.length) { - logger.warn('No chat models configured'); + logger.warn(`No configurations found for type: ${type}`); return; }backend/src/config/config-loader.ts (3)
80-91:initConfigFilehandles creation conflicts well.
You throw an error if the file already exists, preventing accidental overwrites. Consider logging more details about the existing file for troubleshooting.
118-118: Logging the entire config.
Printing the entire config to logs might expose sensitive tokens. Consider sanitizing or omitting sensitive fields before logging.
129-129:setmethod triggers persist immediately.
Automatically saving the config is convenient but can be a performance concern if used frequently. Consider batching writes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
backend/src/config/common-path.ts(1 hunks)backend/src/config/config-loader.ts(1 hunks)backend/src/downloader/__tests__/loadAllChatsModels.spec.ts(2 hunks)backend/src/downloader/universal-downloader.ts(1 hunks)backend/src/downloader/universal-status.ts(1 hunks)backend/src/downloader/universal-utils.ts(1 hunks)backend/src/main.ts(2 hunks)backend/src/model/model-downloader.ts(0 hunks)backend/src/model/utils.ts(0 hunks)
💤 Files with no reviewable changes (2)
- backend/src/model/utils.ts
- backend/src/model/model-downloader.ts
🔇 Additional comments (22)
backend/src/downloader/universal-status.ts (2)
5-8: Interface naming consistency looks good.
The UniversalStatus interface is well-defined and straightforward. It neatly captures essential properties for download status.
72-78: Getter methods look correct.
Returning a copy of the status record in getAllStatus() is a good practice to prevent direct mutation by callers.
backend/src/downloader/__tests__/loadAllChatsModels.spec.ts (2)
3-9: Imports align with new configuration and embedding logic.
The new imports from ConfigLoader, ModelConfig, EmbeddingConfig, and UniversalDownloader are consistent with the refactored architecture.
43-66: Test setup covers both model and embedding configurations.
Your test setup populates configurations and attempts downloads for both chats and embeddings. Ensure external dependencies (e.g., actual Hugging Face model endpoints) are reachable in CI or testing environments.
backend/src/main.ts (2)
4-4: Imported function name mirrors its broader scope.
downloadAllConfig() accurately reflects that it handles both chat and embedding downloads. This improves clarity versus the older, narrower downloadAllModels() naming.
20-20: Consider handling download failures.
If downloadAllConfig() fails, the server start continues. Evaluate whether you prefer startup to abort on download failures or proceed with partial availability.
backend/src/downloader/universal-downloader.ts (1)
1-5: Environment-based local model loading.
Setting env.allowLocalModels = true and env.localModelPath = getModelsDir() is a clear approach for local model usage. Just ensure that these environment settings don’t inadvertently affect other modules that rely on default Hugging Face settings.
backend/src/downloader/universal-utils.ts (2)
48-58: Logging approach effectively tracks downloads.
The log messages for downloading a model, marking status, and success are good for debugging. They help confirm each model is being processed as expected.
74-101: Review forced casting to ModelConfig.
In downloadModel, you cast config to ModelConfig if type is not EMBEDDINGS. Make sure the task property always exists in the config for advanced tasks or variants.
backend/src/config/config-loader.ts (13)
6-8: Imports look good.
The addition of ConfigType and Logger are coherent with the new architecture.
9-16: Optional properties in ModelConfig.
The interface is clearly defined; be sure to provide enough documentation comments if this is part of a public API.
17-22: Clarity on EmbeddingConfig.
Similar to ModelConfig, the structure is well-defined. Consider adding doc comments for consistency.
24-28: AppConfig structure is clear.
This top-level interface for the entire config is straightforward and helps keep model and embedding configurations organized.
29-55: Consider using a JSON-with-comments library.
Manually including comment lines in your JSON can be error-prone. This mirrors a previously given recommendation.
57-68: Constructor ensures immediate config loading.
The constructor logic, using getConfigPath('config') when none is provided, is reasonable.
70-79: Singleton pattern usage.
Storing instances in a static map keyed by ConfigType is a neat approach to manage separate configs per type.
93-93: reload method reuses loadConfig.
Boarding all logic in one place ensures consistency.
98-116: Manual comment-stripping logic could be replaced with a library.
This approach has been flagged in a previous review. A library like strip-json-comments or json5 would ensure more robust parsing.
121-125: Generic get accessor.
Returning the full config if path is not provided is intuitive. Great use of lodash’s _.get for nested retrieval.
134-140: No error-handling for fs.writeFileSync.
You might want to wrap this in a try/catch to handle file-system write errors, consistent with prior reviewer feedback.
185-201: getAllConfigs and getConfig appear consistent.
Returning null for missing config arrays or items is a clean way of signaling absence.
202-202: Start of validateConfig method.
The presence of a validation step upon loading ensures a stable config structure.
| public async getLocalModel(task: string, model: string): Promise<any> { | ||
| try { | ||
| const pipelineInstance = await pipeline(task as PipelineType, model, { | ||
| local_files_only: true, | ||
| revision: 'local', | ||
| }); | ||
| return pipelineInstance; | ||
| } catch (error) { | ||
| this.logger.error( | ||
| `Failed to load local model: ${model} with error: ${error.message || error}`, | ||
| ); | ||
| throw null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid throwing null to preserve error context.
Using throw null; loses the error context for troubleshooting. Typically, an Error object is thrown with a descriptive message.
} catch (error) {
- throw null;
+ throw new Error(`Failed to load local model: ${error.message || error}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async getLocalModel(task: string, model: string): Promise<any> { | |
| try { | |
| const pipelineInstance = await pipeline(task as PipelineType, model, { | |
| local_files_only: true, | |
| revision: 'local', | |
| }); | |
| return pipelineInstance; | |
| } catch (error) { | |
| this.logger.error( | |
| `Failed to load local model: ${model} with error: ${error.message || error}`, | |
| ); | |
| throw null; | |
| } | |
| } | |
| public async getLocalModel(task: string, model: string): Promise<any> { | |
| try { | |
| const pipelineInstance = await pipeline(task as PipelineType, model, { | |
| local_files_only: true, | |
| revision: 'local', | |
| }); | |
| return pipelineInstance; | |
| } catch (error) { | |
| this.logger.error( | |
| `Failed to load local model: ${model} with error: ${error.message || error}`, | |
| ); | |
| throw new Error(`Failed to load local model: ${error.message || error}`); | |
| } | |
| } |
| export async function downloadAllConfig(): Promise<void> { | ||
| downloadAll(ConfigType.CHATS); | ||
| downloadAll(ConfigType.EMBEDDINGS); | ||
| } |
There was a problem hiding this comment.
Await the parallel downloads to avoid immediate function return.
Currently, downloadAllConfig() schedules downloads and then returns without await. This may lead to incomplete downloads if the function caller expects everything to be done.
export async function downloadAllConfig(): Promise<void> {
- downloadAll(ConfigType.CHATS);
- downloadAll(ConfigType.EMBEDDINGS);
+ await Promise.all([
+ downloadAll(ConfigType.CHATS),
+ downloadAll(ConfigType.EMBEDDINGS),
+ ]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function downloadAllConfig(): Promise<void> { | |
| downloadAll(ConfigType.CHATS); | |
| downloadAll(ConfigType.EMBEDDINGS); | |
| } | |
| export async function downloadAllConfig(): Promise<void> { | |
| await Promise.all([ | |
| downloadAll(ConfigType.CHATS), | |
| downloadAll(ConfigType.EMBEDDINGS), | |
| ]); | |
| } |
| // Embs Directory | ||
| export const getEmbDir = (): string => | ||
| ensureDir(path.join(getRootDir(), 'embeddings')); | ||
| export const getEmbPath = (modelName: string): string => | ||
| path.join(getModelsDir(), modelName); | ||
|
|
There was a problem hiding this comment.
Use getEmbDir() instead of getModelsDir() in getEmbPath to avoid confusion.
Currently, getEmbPath points to the main models directory rather than the dedicated embeddings directory, which might lead to unintended overwriting or mixing of files.
export const getEmbPath = (modelName: string): string =>
- path.join(getModelsDir(), modelName);
+ path.join(getEmbDir(), modelName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Embs Directory | |
| export const getEmbDir = (): string => | |
| ensureDir(path.join(getRootDir(), 'embeddings')); | |
| export const getEmbPath = (modelName: string): string => | |
| path.join(getModelsDir(), modelName); | |
| // Embs Directory | |
| export const getEmbDir = (): string => | |
| ensureDir(path.join(getRootDir(), 'embeddings')); | |
| export const getEmbPath = (modelName: string): string => | |
| path.join(getEmbDir(), modelName); |
| removeConfig(modelName: string): boolean { | ||
| if (!ConfigLoader.config[this.type]) { | ||
| return false; | ||
| } | ||
|
|
||
| const initialLength = ConfigLoader.config[this.type].length; | ||
| ConfigLoader.config.models = ConfigLoader.config[this.type].filter( | ||
| (chat) => chat.model !== modelName, | ||
| ); | ||
|
|
||
| if (ConfigLoader.config[this.type].length !== initialLength) { | ||
| this.saveConfig(); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Inconsistent usage of this.type in removeConfig.
Updating ConfigLoader.config.models even if this.type is 'embeddings' poses a risk of accidentally removing clone items from models.
- ConfigLoader.config.models = ConfigLoader.config[this.type].filter(
- (chat) => chat.model !== modelName,
- );
+ ConfigLoader.config[this.type] = ConfigLoader.config[this.type].filter(
+ (chat: ModelConfig | EmbeddingConfig) => chat.model !== modelName,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| removeConfig(modelName: string): boolean { | |
| if (!ConfigLoader.config[this.type]) { | |
| return false; | |
| } | |
| const initialLength = ConfigLoader.config[this.type].length; | |
| ConfigLoader.config.models = ConfigLoader.config[this.type].filter( | |
| (chat) => chat.model !== modelName, | |
| ); | |
| if (ConfigLoader.config[this.type].length !== initialLength) { | |
| this.saveConfig(); | |
| return true; | |
| } | |
| return false; | |
| } | |
| removeConfig(modelName: string): boolean { | |
| if (!ConfigLoader.config[this.type]) { | |
| return false; | |
| } | |
| const initialLength = ConfigLoader.config[this.type].length; | |
| ConfigLoader.config[this.type] = ConfigLoader.config[this.type].filter( | |
| (chat: ModelConfig | EmbeddingConfig) => chat.model !== modelName, | |
| ); | |
| if (ConfigLoader.config[this.type].length !== initialLength) { | |
| this.saveConfig(); | |
| return true; | |
| } | |
| return false; | |
| } |
| addConfig(config: ModelConfig | EmbeddingConfig) { | ||
| if (!ConfigLoader.config[this.type]) { | ||
| ConfigLoader.config[this.type] = []; | ||
| } | ||
| this.logger.log(ConfigLoader.config); | ||
| const index = ConfigLoader.config[this.type].findIndex( | ||
| (chat) => chat.model === config.model, | ||
| ); | ||
| if (index !== -1) { | ||
| ConfigLoader.config[this.type].splice(index, 1); | ||
| } | ||
|
|
||
| if (config.default) { | ||
| ConfigLoader.config.models.forEach((chat) => { | ||
| chat.default = false; | ||
| }); | ||
| } | ||
|
|
||
| ConfigLoader.config[this.type].push(config); | ||
| this.saveConfig(); | ||
| } |
There was a problem hiding this comment.
Potential mix-up with this.type and models.
When adding a default embedding, the loop explicitly sets ConfigLoader.config.models.forEach(...) to disable existing defaults. This might unintentionally alter models when this.type is ConfigType.EMBEDDINGS.
+ // If adding an embedding config, ensure we only reset defaults among embeddings
if (config.default) {
- ConfigLoader.config.models.forEach((chat) => {
+ (ConfigLoader.config[this.type] as (ModelConfig | EmbeddingConfig)[]).forEach((c) => {
c.default = false;
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addConfig(config: ModelConfig | EmbeddingConfig) { | |
| if (!ConfigLoader.config[this.type]) { | |
| ConfigLoader.config[this.type] = []; | |
| } | |
| this.logger.log(ConfigLoader.config); | |
| const index = ConfigLoader.config[this.type].findIndex( | |
| (chat) => chat.model === config.model, | |
| ); | |
| if (index !== -1) { | |
| ConfigLoader.config[this.type].splice(index, 1); | |
| } | |
| if (config.default) { | |
| ConfigLoader.config.models.forEach((chat) => { | |
| chat.default = false; | |
| }); | |
| } | |
| ConfigLoader.config[this.type].push(config); | |
| this.saveConfig(); | |
| } | |
| addConfig(config: ModelConfig | EmbeddingConfig) { | |
| if (!ConfigLoader.config[this.type]) { | |
| ConfigLoader.config[this.type] = []; | |
| } | |
| this.logger.log(ConfigLoader.config); | |
| const index = ConfigLoader.config[this.type].findIndex( | |
| (chat) => chat.model === config.model, | |
| ); | |
| if (index !== -1) { | |
| ConfigLoader.config[this.type].splice(index, 1); | |
| } | |
| if (config.default) { | |
| (ConfigLoader.config[this.type] as (ModelConfig | EmbeddingConfig)[]).forEach((c) => { | |
| c.default = false; | |
| }); | |
| } | |
| ConfigLoader.config[this.type].push(config); | |
| this.saveConfig(); | |
| } |
backend/src/config/config-loader.ts
Outdated
| if (!ConfigLoader.config) { | ||
| ConfigLoader.config = {}; | ||
| } | ||
|
|
||
| if (typeof ConfigLoader.config !== 'object') { | ||
| throw new Error('Invalid configuration: Must be an object'); | ||
| } | ||
|
|
||
| if (ConfigLoader.config.models) { | ||
| if (!Array.isArray(ConfigLoader.config.models)) { | ||
| throw new Error("Invalid configuration: 'chats' must be an array"); | ||
| } | ||
|
|
||
| ConfigLoader.config.models.forEach((chat, index) => { | ||
| if (!chat.model) { | ||
| throw new Error( | ||
| `Invalid chat configuration at index ${index}: 'model' is required`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| const defaultChats = ConfigLoader.config.models.filter( | ||
| (chat) => chat.default, | ||
| ); | ||
| if (defaultChats.length > 1) { | ||
| throw new Error( | ||
| 'Invalid configuration: Multiple default chat configurations found', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (ConfigLoader.config[ConfigType.EMBEDDINGS]) { | ||
| this.logger.log(ConfigLoader.config[ConfigType.EMBEDDINGS]); | ||
| if (!Array.isArray(ConfigLoader.config[ConfigType.EMBEDDINGS])) { | ||
| throw new Error("Invalid configuration: 'embeddings' must be an array"); | ||
| } | ||
|
|
||
| ConfigLoader.config.models.forEach((emb, index) => { | ||
| if (!emb.model) { | ||
| throw new Error( | ||
| `Invalid chat configuration at index ${index}: 'model' is required`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| const defaultChats = ConfigLoader.config[ConfigType.EMBEDDINGS].filter( | ||
| (chat) => chat.default, | ||
| ); | ||
| if (defaultChats.length > 1) { | ||
| throw new Error( | ||
| 'Invalid configuration: Multiple default emb configurations found', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Mis-targeted iteration for embeddings.
Inside the ConfigType.EMBEDDINGS block, you iterate over ConfigLoader.config.models at line 241. This likely should iterate over ConfigLoader.config.embeddings.
- ConfigLoader.config.models.forEach((emb, index) => {
+ ConfigLoader.config[ConfigType.EMBEDDINGS].forEach((emb, index) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ConfigLoader.config) { | |
| ConfigLoader.config = {}; | |
| } | |
| if (typeof ConfigLoader.config !== 'object') { | |
| throw new Error('Invalid configuration: Must be an object'); | |
| } | |
| if (ConfigLoader.config.models) { | |
| if (!Array.isArray(ConfigLoader.config.models)) { | |
| throw new Error("Invalid configuration: 'chats' must be an array"); | |
| } | |
| ConfigLoader.config.models.forEach((chat, index) => { | |
| if (!chat.model) { | |
| throw new Error( | |
| `Invalid chat configuration at index ${index}: 'model' is required`, | |
| ); | |
| } | |
| }); | |
| const defaultChats = ConfigLoader.config.models.filter( | |
| (chat) => chat.default, | |
| ); | |
| if (defaultChats.length > 1) { | |
| throw new Error( | |
| 'Invalid configuration: Multiple default chat configurations found', | |
| ); | |
| } | |
| } | |
| if (ConfigLoader.config[ConfigType.EMBEDDINGS]) { | |
| this.logger.log(ConfigLoader.config[ConfigType.EMBEDDINGS]); | |
| if (!Array.isArray(ConfigLoader.config[ConfigType.EMBEDDINGS])) { | |
| throw new Error("Invalid configuration: 'embeddings' must be an array"); | |
| } | |
| ConfigLoader.config.models.forEach((emb, index) => { | |
| if (!emb.model) { | |
| throw new Error( | |
| `Invalid chat configuration at index ${index}: 'model' is required`, | |
| ); | |
| } | |
| }); | |
| const defaultChats = ConfigLoader.config[ConfigType.EMBEDDINGS].filter( | |
| (chat) => chat.default, | |
| ); | |
| if (defaultChats.length > 1) { | |
| throw new Error( | |
| 'Invalid configuration: Multiple default emb configurations found', | |
| ); | |
| } | |
| if (!ConfigLoader.config) { | |
| ConfigLoader.config = {}; | |
| } | |
| if (typeof ConfigLoader.config !== 'object') { | |
| throw new Error('Invalid configuration: Must be an object'); | |
| } | |
| if (ConfigLoader.config.models) { | |
| if (!Array.isArray(ConfigLoader.config.models)) { | |
| throw new Error("Invalid configuration: 'chats' must be an array"); | |
| } | |
| ConfigLoader.config.models.forEach((chat, index) => { | |
| if (!chat.model) { | |
| throw new Error( | |
| `Invalid chat configuration at index ${index}: 'model' is required`, | |
| ); | |
| } | |
| }); | |
| const defaultChats = ConfigLoader.config.models.filter( | |
| (chat) => chat.default, | |
| ); | |
| if (defaultChats.length > 1) { | |
| throw new Error( | |
| 'Invalid configuration: Multiple default chat configurations found', | |
| ); | |
| } | |
| } | |
| if (ConfigLoader.config[ConfigType.EMBEDDINGS]) { | |
| this.logger.log(ConfigLoader.config[ConfigType.EMBEDDINGS]); | |
| if (!Array.isArray(ConfigLoader.config[ConfigType.EMBEDDINGS])) { | |
| throw new Error("Invalid configuration: 'embeddings' must be an array"); | |
| } | |
| ConfigLoader.config[ConfigType.EMBEDDINGS].forEach((emb, index) => { | |
| if (!emb.model) { | |
| throw new Error( | |
| `Invalid chat configuration at index ${index}: 'model' is required`, | |
| ); | |
| } | |
| }); | |
| const defaultChats = ConfigLoader.config[ConfigType.EMBEDDINGS].filter( | |
| (chat) => chat.default, | |
| ); | |
| if (defaultChats.length > 1) { | |
| throw new Error( | |
| 'Invalid configuration: Multiple default emb configurations found', | |
| ); | |
| } |
Sma1lboy
force-pushed
the
feat-adding-embedding-config
branch
from
bb3148f to
1340532
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
backend/src/model/model-status.ts (1)
5-8: Consider storing modelName in the status object
A previous reviewer suggested adding amodelNamefield toModelStatus. Storing the model name explicitly might improve clarity and debugging, especially when serializing or referencing statuses externally.backend/src/model/downloader/const.ts (1)
1-70: Consider externalizing the remote model list
Maintaining this large array in code may complicate updates. Storing it in a dedicated config file or database and loading it dynamically could improve maintainability.backend/src/model/downloader/downloader.ts (2)
26-46: Avoid marking remote models as "downloaded"
Here, remote models are assignedisDownloaded = truewithout actual download steps. Consider introducing an explicit remote/tracked state if needed, to avoid confusion.
48-68: Similar concern for "remote" detection
IngetLocalModel, remote models are also flagged as downloaded. Consider refining this distinction to prevent misleading status.backend/src/config/common-path.ts (1)
7-7: Consider a more robust root directory approach
Usingpath.resolve(__dirname, '../../../../')may break if the folder structure changes. Provide a more stable mechanism or enrich the existing TODO note.backend/src/config/config-loader.ts (1)
208-246: Consider adding type validation for config propertiesWhile the structure validation is good, consider adding type validation for properties like
endpoint,token, etc.validateConfig() { // ... existing validation ... if (this.config.chats) { if (!Array.isArray(this.config.chats)) { throw new Error("Invalid configuration: 'chats' must be an array"); } this.config.chats.forEach((chat, index) => { if (!chat.model) { throw new Error( `Invalid chat configuration at index ${index}: 'model' is required`, ); } + if (chat.endpoint && typeof chat.endpoint !== 'string') { + throw new Error( + `Invalid chat configuration at index ${index}: 'endpoint' must be a string`, + ); + } + if (chat.token && typeof chat.token !== 'string') { + throw new Error( + `Invalid chat configuration at index ${index}: 'token' must be a string`, + ); + } }); } if (this.config.embeddings) { if (!this.config.embeddings.model) { throw new Error("Invalid embedding configuration: 'model' is required"); } + if (this.config.embeddings.endpoint && typeof this.config.embeddings.endpoint !== 'string') { + throw new Error("Invalid embedding configuration: 'endpoint' must be a string"); + } + if (this.config.embeddings.token && typeof this.config.embeddings.token !== 'string') { + throw new Error("Invalid embedding configuration: 'token' must be a string"); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/src/config/common-path.ts(2 hunks)backend/src/config/config-loader.ts(2 hunks)backend/src/main.ts(2 hunks)backend/src/model/__tests__/app.e2e-spec.ts(0 hunks)backend/src/model/__tests__/jest-e2e.json(0 hunks)backend/src/model/__tests__/loadAllChatsModels.spec.ts(1 hunks)backend/src/model/downloader/const.ts(1 hunks)backend/src/model/downloader/downloader.ts(1 hunks)backend/src/model/model-downloader.ts(0 hunks)backend/src/model/model-status.ts(1 hunks)backend/src/model/utils.ts(1 hunks)
💤 Files with no reviewable changes (3)
- backend/src/model/tests/app.e2e-spec.ts
- backend/src/model/tests/jest-e2e.json
- backend/src/model/model-downloader.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/model/__tests__/loadAllChatsModels.spec.ts
[error] 6-6: Declarations inside of a import declaration may not have duplicates
a second declaration of getConfigPath is not allowed
getConfigPath is first declared here
(parse)
🔇 Additional comments (16)
backend/src/model/model-status.ts (5)
1-4: Imports look good
All necessary dependencies are present, and importing from 'src/config/common-path' matches the usage within this file.
27-46: Log errors when loading model status
This logic silently discards filesystem errors by setting this.status = {} without logging. This was also raised previously. Consider logging the error:
} catch (error) {
+ console.error(`Failed to load model status: ${error.message}`);
this.status = {};
}48-58: Add error handling when saving model status
Currently, no errors are caught when writing. Consider wrapping fs.writeFileSync in a try-catch and logging errors. This was noted in a past review as well.
private saveStatus() {
const statusDir = path.dirname(this.statusPath);
if (!fs.existsSync(statusDir)) {
fs.mkdirSync(statusDir, { recursive: true });
}
- fs.writeFileSync(
+ try {
+ fs.writeFileSync(
this.statusPath,
JSON.stringify(this.status, null, 2),
'utf-8',
);
+ } catch (error) {
+ console.error(`Failed to save model status: ${error.message}`);
+ }
}60-66: Watch for potential race conditions
Repeatedly calling updateStatus in quick succession from multiple async calls could inadvertently overwrite status changes. Consider a queue or locking mechanism if concurrency is expected.
68-74: Simple and clear retrieval
The getStatus and getAllStatus methods are straightforward and behave as expected.
backend/src/model/downloader/const.ts (1)
72-73: Function implementation is concise and correct
isRemoteModel cleanly checks membership in the array.
backend/src/model/downloader/downloader.ts (2)
1-5: Imports are appropriate
All necessary modules are imported correctly.
7-9: Confirm no conflicts with shared directories
For multiple parallel processes or container instances, ensure env.localModelPath = getModelsDir() does not introduce concurrency conflicts in the filesystem.
backend/src/config/common-path.ts (2)
2-2: Move from fs to fs-extra
Adopting fs-extra provides additional convenience methods for file operations, which can be beneficial.
30-33: Verify existing usage of getConfigPath
Previously, this function could accept a parameter. Now it always returns config.json. Confirm that no other code relies on a dynamic config filename.
backend/src/model/utils.ts (2)
1-6: LGTM! Dependencies and logger initialization are well structured.
The imports and logger setup follow NestJS best practices.
46-47: Include stack traces in error logging
Error logging should include stack traces for better debugging.
Also applies to: 83-86
backend/src/config/config-loader.ts (4)
15-52: LGTM! Well-structured interfaces and helpful example configuration.
The interfaces are clearly defined and the example configuration provides good guidance for users.
98-102: Consider using a library to parse JSON with comments
129-137: Add error handling for file write operations
141-202: LGTM! Well-implemented configuration management methods.
The methods provide robust handling of chat configurations with proper validation and default management.
| import { downloadAllModels } from '../utils'; | ||
| import { getConfigDir, getConfigPath } from 'src/config/common-path'; | ||
| import { getConfigPath, getConfigPath } from 'src/config/common-path'; |
There was a problem hiding this comment.
Remove duplicate import of getConfigPath
Importing getConfigPath twice is a syntax error.
-import { getConfigPath, getConfigPath } from 'src/config/common-path';
+import { getConfigPath } from 'src/config/common-path';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { getConfigPath, getConfigPath } from 'src/config/common-path'; | |
| import { getConfigPath } from 'src/config/common-path'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: Declarations inside of a import declaration may not have duplicates
a second declaration of getConfigPath is not allowed
getConfigPath is first declared here
(parse)
backend/src/config/common-path.ts
Outdated
| export const getModelStatusPath = (): string => { | ||
| const rootPath = ensureDir(getRootDir()); | ||
| const modelStatusPath = path.join(rootPath, 'model-status.json'); | ||
| writeFileSync(modelStatusPath, '{}'); | ||
| return modelStatusPath; | ||
| }; |
There was a problem hiding this comment.
Potential overwrite of existing model-status.json
Calling getModelStatusPath always writes '{}', even if the file exists. This can wipe out existing statuses. Implement a check to write only when missing the file.
export const getModelStatusPath = (): string => {
const rootPath = ensureDir(getRootDir());
const modelStatusPath = path.join(rootPath, 'model-status.json');
- writeFileSync(modelStatusPath, '{}');
+ if (!existsSync(modelStatusPath)) {
+ writeFileSync(modelStatusPath, '{}');
+ }
return modelStatusPath;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getModelStatusPath = (): string => { | |
| const rootPath = ensureDir(getRootDir()); | |
| const modelStatusPath = path.join(rootPath, 'model-status.json'); | |
| writeFileSync(modelStatusPath, '{}'); | |
| return modelStatusPath; | |
| }; | |
| export const getModelStatusPath = (): string => { | |
| const rootPath = ensureDir(getRootDir()); | |
| const modelStatusPath = path.join(rootPath, 'model-status.json'); | |
| if (!existsSync(modelStatusPath)) { | |
| writeFileSync(modelStatusPath, '{}'); | |
| } | |
| return modelStatusPath; | |
| }; |
backend/src/model/utils.ts
Outdated
| logger.log( | ||
| `Embedding model ${model} is already downloaded and verified, skipping...`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Remove early return in embedding section
The early return in the embedding section could prevent the function from completing if there are both chat and embedding models.
- return;
+ continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return; | |
| continue; |
backend/src/model/utils.ts
Outdated
| export async function downloadModel( | ||
| modelName: string, | ||
| isEmbedding = false, | ||
| ): Promise<void> { | ||
| const configLoader = ConfigLoader.getInstance(); | ||
| const statusManager = ModelStatusManager.getInstance(); | ||
| const downloader = ModelDownloader.getInstance(); | ||
| console.log('Loaded config:', chats); | ||
| const loadPromises = chats.map(async (chatConfig: ChatConfig) => { | ||
| const { model, task } = chatConfig; | ||
|
|
||
| let modelConfig; | ||
| let task: string; | ||
|
|
||
| if (isEmbedding) { | ||
| modelConfig = configLoader.getEmbeddingConfig(); | ||
| task = 'feature-extraction'; | ||
| } else { | ||
| modelConfig = configLoader.getChatConfig(modelName); | ||
| task = modelConfig?.task || 'chat'; | ||
| } | ||
|
|
||
| if (!modelConfig) { | ||
| throw new Error(`Model configuration not found for: ${modelName}`); | ||
| } | ||
|
|
||
| const status = statusManager.getStatus(modelName); | ||
| if (status?.isDownloaded) { | ||
| try { | ||
| downloader.logger.log(model, task); | ||
| const pipelineInstance = await downloader.downloadModel(task, model); | ||
| await downloader.getLocalModel(task, modelName); | ||
| logger.log( | ||
| `Model ${modelName} is already downloaded and verified, skipping...`, | ||
| ); | ||
| return; | ||
| } catch (error) { | ||
| downloader.logger.error(`Failed to load model ${model}:`, error.message); | ||
| logger.warn( | ||
| `Model ${modelName} was marked as downloaded but not found locally, re-downloading...`, | ||
| ); | ||
| } | ||
| }); | ||
| await Promise.all(loadPromises); | ||
| } | ||
|
|
||
| downloader.logger.log('All models loaded.'); | ||
| try { | ||
| logger.log(`Downloading model: ${modelName}`); | ||
| await downloader.downloadModel(task, modelName); | ||
| logger.log(`Successfully downloaded model: ${modelName}`); | ||
| } catch (error) { | ||
| logger.error(`Failed to download model ${modelName}:`, error.message); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to use shared helper function
This function contains similar logic to checkAndDownloadAllModels. Consider using the same shared helper function to reduce duplication.
export async function downloadModel(
modelName: string,
isEmbedding = false,
): Promise<void> {
const configLoader = ConfigLoader.getInstance();
const statusManager = ModelStatusManager.getInstance();
const downloader = ModelDownloader.getInstance();
let modelConfig;
let task: string;
if (isEmbedding) {
modelConfig = configLoader.getEmbeddingConfig();
task = 'feature-extraction';
} else {
modelConfig = configLoader.getChatConfig(modelName);
task = modelConfig?.task || 'chat';
}
if (!modelConfig) {
throw new Error(`Model configuration not found for: ${modelName}`);
}
- const status = statusManager.getStatus(modelName);
- // ... remove existing verification and download logic ...
+ await verifyAndDownloadModel(modelName, task, downloader, statusManager);
}Committable suggestion skipped: line range outside the PR's diff.
backend/src/model/utils.ts
Outdated
| export async function checkAndDownloadAllModels(): Promise<void> { | ||
| const configLoader = ConfigLoader.getInstance(); | ||
| const statusManager = ModelStatusManager.getInstance(); | ||
| const downloader = ModelDownloader.getInstance(); | ||
|
|
||
| const chatConfigs = configLoader.getAllChatConfigs(); | ||
| const embeddingConfig = configLoader.getEmbeddingConfig(); | ||
|
|
||
| logger.log('Checking and downloading configured models...'); | ||
|
|
||
| if (!chatConfigs.length) { | ||
| logger.warn('No chat models configured'); | ||
| } else { | ||
| const chatDownloadTasks = chatConfigs.map(async (chatConfig) => { | ||
| const { model, task } = chatConfig; | ||
| const status = statusManager.getStatus(model); | ||
|
|
||
| if (status?.isDownloaded) { | ||
| try { | ||
| await downloader.getLocalModel(task || 'chat', model); | ||
| logger.log( | ||
| `Model ${model} is already downloaded and verified, skipping...`, | ||
| ); | ||
| return; | ||
| } catch (error) { | ||
| logger.warn( | ||
| `Model ${model} was marked as downloaded but not found locally, re-downloading...`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| logger.log( | ||
| `Downloading chat model: ${model} for task: ${task || 'chat'}`, | ||
| ); | ||
| await downloader.downloadModel(task || 'chat', model); | ||
| logger.log(`Successfully downloaded model: ${model}`); | ||
| } catch (error) { | ||
| logger.error(`Failed to download model ${model}:`, error.message); | ||
| throw error; | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| await Promise.all(chatDownloadTasks); | ||
| logger.log('All chat models downloaded successfully.'); | ||
| } catch (error) { | ||
| logger.error('One or more chat models failed to download'); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| if (embeddingConfig) { | ||
| const { model } = embeddingConfig; | ||
| const status = statusManager.getStatus(model); | ||
|
|
||
| if (status?.isDownloaded) { | ||
| try { | ||
| await downloader.getLocalModel('feature-extraction', model); | ||
| logger.log( | ||
| `Embedding model ${model} is already downloaded and verified, skipping...`, | ||
| ); | ||
| return; | ||
| } catch (error) { | ||
| logger.warn( | ||
| `Embedding model ${model} was marked as downloaded but not found locally, re-downloading...`, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| logger.log(`Downloading embedding model: ${model}`); | ||
| await downloader.downloadModel('feature-extraction', model); | ||
| logger.log(`Successfully downloaded embedding model: ${model}`); | ||
| } catch (error) { | ||
| logger.error( | ||
| `Failed to download embedding model ${model}:`, | ||
| error.message, | ||
| ); | ||
| throw error; | ||
| } | ||
| } else { | ||
| logger.warn('No embedding model configured'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication
The chat and embedding model download logic contains significant duplication. Consider extracting the common verification and download logic into a shared helper function.
Here's a suggested refactor:
+ async function verifyAndDownloadModel(
+ model: string,
+ task: string,
+ downloader: ModelDownloader,
+ statusManager: ModelStatusManager
+ ): Promise<void> {
+ const status = statusManager.getStatus(model);
+
+ if (status?.isDownloaded) {
+ try {
+ await downloader.getLocalModel(task, model);
+ logger.log(`Model ${model} is already downloaded and verified, skipping...`);
+ return;
+ } catch (error) {
+ logger.warn(
+ `Model ${model} was marked as downloaded but not found locally, re-downloading...`
+ );
+ }
+ }
+
+ try {
+ logger.log(`Downloading model: ${model} for task: ${task}`);
+ await downloader.downloadModel(task, model);
+ logger.log(`Successfully downloaded model: ${model}`);
+ } catch (error) {
+ logger.error(`Failed to download model ${model}: ${error.message}`, error.stack);
+ throw error;
+ }
+ }
export async function checkAndDownloadAllModels(): Promise<void> {
// ... existing setup code ...
if (!chatConfigs.length) {
logger.warn('No chat models configured');
} else {
const chatDownloadTasks = chatConfigs.map(async (chatConfig) => {
const { model, task } = chatConfig;
- const status = statusManager.getStatus(model);
- // ... remove existing verification and download logic ...
+ await verifyAndDownloadModel(model, task || 'chat', downloader, statusManager);
});
try {
await Promise.all(chatDownloadTasks);
logger.log('All chat models downloaded successfully.');
} catch (error) {
logger.error('One or more chat models failed to download');
throw error;
}
}
if (embeddingConfig) {
const { model } = embeddingConfig;
- const status = statusManager.getStatus(model);
- // ... remove existing verification and download logic ...
+ await verifyAndDownloadModel(model, 'feature-extraction', downloader, statusManager);
} else {
logger.warn('No embedding model configured');
}
}Committable suggestion skipped: line range outside the PR's diff.
backend/src/config/config-loader.ts
Outdated
| public initConfigFile(): void { | ||
| Logger.log('Creating example config file', 'ConfigLoader'); | ||
|
|
||
| this.chatsConfig = JSON.parse(file); | ||
| console.log('Raw file content:', this.chatsConfig); | ||
| const config = getConfigPath(); | ||
| if (fs.existsSync(config)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!fs.existsSync(config)) { | ||
| //make file | ||
| fs.writeFileSync(config, exampleConfigContent, 'utf-8'); | ||
| } | ||
| Logger.log('Creating example config file', 'ConfigLoader'); |
There was a problem hiding this comment.
Fix duplicate log message and redundant existence check
The initConfigFile method has two issues:
- Duplicate log message at lines 73 and 84
- Redundant file existence check at lines 80-83
public initConfigFile(): void {
Logger.log('Creating example config file', 'ConfigLoader');
const config = getConfigPath();
if (fs.existsSync(config)) {
return;
}
- if (!fs.existsSync(config)) {
- //make file
- fs.writeFileSync(config, exampleConfigContent, 'utf-8');
- }
- Logger.log('Creating example config file', 'ConfigLoader');
+ fs.writeFileSync(config, exampleConfigContent, 'utf-8');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public initConfigFile(): void { | |
| Logger.log('Creating example config file', 'ConfigLoader'); | |
| this.chatsConfig = JSON.parse(file); | |
| console.log('Raw file content:', this.chatsConfig); | |
| const config = getConfigPath(); | |
| if (fs.existsSync(config)) { | |
| return; | |
| } | |
| if (!fs.existsSync(config)) { | |
| //make file | |
| fs.writeFileSync(config, exampleConfigContent, 'utf-8'); | |
| } | |
| Logger.log('Creating example config file', 'ConfigLoader'); | |
| public initConfigFile(): void { | |
| Logger.log('Creating example config file', 'ConfigLoader'); | |
| const config = getConfigPath(); | |
| if (fs.existsSync(config)) { | |
| return; | |
| } | |
| fs.writeFileSync(config, exampleConfigContent, 'utf-8'); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
backend/src/config/config-loader.ts (3)
77-90:⚠️ Potential issueFix duplicate log message and redundant existence check
The
initConfigFilemethod contains duplicate logging and redundant file existence checks.public initConfigFile(): void { Logger.log('Creating example config file', 'ConfigLoader'); const config = getConfigPath(); if (fs.existsSync(config)) { return; } - if (!fs.existsSync(config)) { - //make file - fs.writeFileSync(config, exampleConfigContent, 'utf-8'); - } - Logger.log('Creating example config file', 'ConfigLoader'); + fs.writeFileSync(config, exampleConfigContent, 'utf-8'); }
103-106: 🛠️ Refactor suggestionConsider using a library to parse JSON with comments
The custom regex for stripping JSON comments may not handle all edge cases.
136-145:⚠️ Potential issueAdd error handling for file write operations
The
saveConfigmethod lacks error handling for file system operations.
🧹 Nitpick comments (16)
backend/src/downloader/model-downloader.ts (4)
2-2: Remove unused importcat.The import
catfrom@huggingface/transformersis never used in this file. Consider removing it to keep dependencies and imports minimal.- import { PipelineType, pipeline, env, cat } from '@huggingface/transformers'; + import { PipelineType, pipeline, env } from '@huggingface/transformers';
3-3: Remove unused importgetEmbDir.
getEmbDiris imported but not used. Removing unused imports helps maintain a clean codebase.- import { getEmbDir, getModelPath, getModelsDir } from 'src/config/common-path'; + import { getModelPath, getModelsDir } from 'src/config/common-path';
24-24: Use logger instead ofconsole.logfor consistency.You already use NestJS' Logger for logging. Consider replacing
console.logwiththis.logger.debugor an appropriate log level for consistent logging.- console.log(this.statusManager); + this.logger.debug(`Current status manager state: ${JSON.stringify(this.statusManager)}`);
31-31: Use logger instead ofconsole.logfor debugging output.Similarly, replace
console.log(path)with logger statements for consistent logging and better log level management.- console.log(path); + this.logger.debug(`Cache directory path: ${path}`);backend/src/embedding/__tests__/loadAllEmbModels.spec.ts (1)
8-9: Use optional chaining to simplify checks.Static analysis suggests using optional chaining for safety and cleaner code. For instance, replace:
type && type.constructor && type.constructor.name === 'Float32Array'with optional chaining:
- type && - type.constructor && - (type.constructor.name === 'Float32Array' || type.constructor.name === 'BigInt64Array') + (type?.constructor?.name === 'Float32Array' || type?.constructor?.name === 'BigInt64Array')🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/src/downloader/embedding-downloader.ts (1)
18-18: Return type clarity.
getPipelinereturns either an embedding model object ornull. Consider documenting or narrowing down the return type to avoid confusion in consumer code.backend/src/downloader/const.ts (1)
1-70: Consider removing duplicates or using a Set for “REMOTE_MODEL_LISTS”.
It appears that entries like'claude-2.0'and'claude-instant-1.2'are repeated (e.g., lines 15 and 37, lines 16 and 40). Such duplicates may cause confusion or maintenance overhead. Using aSetwould help avoid accidental duplicates and speed up membership checks.- export const REMOTE_MODEL_LISTS = [ - 'claude-2.0', - // ... - 'claude-2.0', - // ... - ]; + export const REMOTE_MODEL_SET = new Set([ + 'claude-2.0', + // ... + ]);backend/src/downloader/universal-status.ts (2)
44-45: Log the file-read error to help with troubleshooting.
When JSON parsing or file reading fails, the code silently falls back on an empty object. Consider logging the caught error for clearer debugging.} catch (error) { + console.error(`Error loading status from ${this.statusPath}:`, error); this.status = {}; }
60-66: Potential concurrency or multi-process issue on status updates.
If multiple processes or threads callupdateStatussimultaneously, writes to the same file risk collisions. You may need locks, a queue mechanism, or a shared data store to avoid race conditions.backend/src/downloader/universal-utils.ts (3)
21-27: Encapsulate parallel downloads for both chats and embeddings.
downloadAllcallscheckAndDownloadAllModelssequentially for chats first, then embeddings. If desired, you could parallelize them withawait Promise.all(...). But if your system or external services have resource constraints, sequential downloads may be preferable.
28-55: Handle partial model presence or missing pipeline differences.
When re-downloading or checking for model presence, consider how you handle partial/incomplete downloads. Possibly add validations for file integrity or more granular checks so that if a partial download is corrupted, you gracefully recover.
70-76: Add a “force re-download” option.
Currently, if a model is already marked as downloaded, it skips re-downloading unless an error is detected. Some users may want to refresh or force re-download a model. Having a “force” parameter would provide additional flexibility.backend/src/downloader/__tests__/loadAllChatsModels.spec.ts (1)
96-96: Extended test timeout is appropriate for large model downloads.
Allowing additional time for end-to-end tests on real model loading is helpful to prevent spurious timeouts.backend/src/config/config-loader.ts (3)
28-54: Consider using placeholder values for sensitive informationThe example configuration contains template API tokens that might be mistakenly committed. Consider using clearly marked placeholder values instead.
- "token": "your-openai-token", // Replace with your OpenAI token + "token": "<YOUR_OPENAI_TOKEN>", // Replace with your actual OpenAI API key
203-205: Fix incorrect error message in models validationThe error message references 'chats' instead of 'models'.
if (!Array.isArray(ConfigLoader.config.models)) { - throw new Error("Invalid configuration: 'chats' must be an array"); + throw new Error("Invalid configuration: 'models' must be an array"); }
56-251: Consider enhancing type safety with genericsThe current implementation could benefit from using TypeScript generics to ensure type safety when handling different configuration types.
Consider refactoring the class to use generics:
export class ConfigLoader<T extends ModelConfig | EmbeddingConfig> { private type: ConfigType; private static instances: Map<ConfigType, ConfigLoader<any>> = new Map(); private static config: AppConfig; public static getInstance<T extends ModelConfig | EmbeddingConfig>( type: ConfigType ): ConfigLoader<T> { if (!ConfigLoader.instances.has(type)) { ConfigLoader.instances.set(type, new ConfigLoader<T>(type)); } return ConfigLoader.instances.get(type)!; } // ... rest of the implementation with proper type constraints }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
backend/src/config/common-path.ts(2 hunks)backend/src/config/config-loader.ts(1 hunks)backend/src/downloader/__tests__/loadAllChatsModels.spec.ts(3 hunks)backend/src/downloader/const.ts(1 hunks)backend/src/downloader/embedding-downloader.ts(1 hunks)backend/src/downloader/model-downloader.ts(1 hunks)backend/src/downloader/universal-status.ts(1 hunks)backend/src/downloader/universal-utils.ts(1 hunks)backend/src/embedding/__tests__/loadAllEmbModels.spec.ts(1 hunks)backend/src/embedding/local-embbeding-provider.ts(1 hunks)backend/src/embedding/openai-embbeding-provider.ts(1 hunks)backend/src/main.ts(2 hunks)backend/src/model/utils.ts(0 hunks)llm-server/src/model/openai-model-provider.ts(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/model/utils.ts
✅ Files skipped from review due to trivial changes (1)
- llm-server/src/model/openai-model-provider.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main.ts
- backend/src/config/common-path.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/embedding/__tests__/loadAllEmbModels.spec.ts
[error] 8-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
backend/src/embedding/openai-embbeding-provider.ts
[error] 6-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 26-26: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
backend/src/embedding/local-embbeding-provider.ts
[error] 5-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (11)
backend/src/downloader/embedding-downloader.ts (1)
1-4: Validate directories before usage in FlagEmbedding.init.
Consider validating or ensuring the embeddings directory exists or is writable before calling FlagEmbedding.init. If the directory is missing or inaccessible, the operation may fail unexpectedly.
backend/src/downloader/const.ts (1)
72-73: Verify case sensitivity for isRemoteModel().
Currently, isRemoteModel uses a direct .includes() check with the original case. Ensure that all model references consistently use the same casing, or consider lowercasing both sides if needed to avoid accidental mismatches.
backend/src/downloader/universal-status.ts (1)
72-74: Great approach returning a shallow copy of status.
Returning a spread copy of this.status helps prevent external code from mutating the internal status state. This is a solid practice for immutability.
backend/src/downloader/universal-utils.ts (1)
100-106: Fallback logic for taskType is good.
Automatically defaulting to TaskType.EMBEDDING or TaskType.CHAT based on the config type is a clean and maintainable approach. This is well-implemented.
backend/src/downloader/__tests__/loadAllChatsModels.spec.ts (4)
3-7: Imports look consistent with the new code structure.
These imports align with recent refactors in ConfigLoader and match the new approach in universal downloader utilities.
42-47: Singleton usage for config loaders is correct.
Fetching separate instances for CHATS vs. EMBEDDINGS provides clarity. Ensure that ConfigLoader is indeed returning the same instance if the same type is requested multiple times.
48-63: Test coverage for adding chat and embedding configs is good.
You are verifying both chat and embedding models, showing thorough test coverage. Keep it up!
67-70: Verifying the local model usage properly.
Retrieving the local model from the UniversalDownloader and then expecting a valid model and tokenizer object is a solid check. This ensures correct model load.
backend/src/config/config-loader.ts (3)
160-164:
Fix incorrect array reference when setting default config
When adding a new config with default=true, the code incorrectly references the models array regardless of the config type.
if (config.default) {
- ConfigLoader.config.models.forEach((chat) => {
+ ConfigLoader.config[this.type].forEach((item) => {
- chat.default = false;
+ item.default = false;
});
}Likely invalid or redundant comment.
231-237:
Fix incorrect array reference in embeddings validation
The validation for embeddings incorrectly iterates over models array instead of embeddings.
- ConfigLoader.config.models.forEach((emb, index) => {
+ ConfigLoader.config[ConfigType.EMBEDDINGS].forEach((emb, index) => {
if (!emb.model) {
throw new Error(
- `Invalid chat configuration at index ${index}: 'model' is required`,
+ `Invalid embedding configuration at index ${index}: 'model' is required`,
);
}
});Likely invalid or redundant comment.
176-178:
Fix incorrect array update in removeConfig
The method incorrectly updates the models array instead of using the type-specific array.
- ConfigLoader.config.models = ConfigLoader.config[this.type].filter(
+ ConfigLoader.config[this.type] = ConfigLoader.config[this.type].filter(
(chat) => chat.model !== modelName,
);Likely invalid or redundant comment.
| export class localEmbProvider { | ||
| private static logger = new Logger(localEmbProvider.name); | ||
|
|
||
| static async generateEmbResponse(model: string, message: string[]) { | ||
| const embLoader = EmbeddingDownloader.getInstance(); | ||
| try { | ||
| const embeddingModel = await embLoader.getPipeline(model); | ||
| const embeddings = embeddingModel.embed(message); | ||
|
|
||
| for await (const batch of embeddings) { | ||
| console.log(batch); | ||
| } | ||
| } catch (error) { | ||
| this.logger.log(`error when using ${model} api`); | ||
| } | ||
| } | ||
|
|
||
| static async getEmbList() { | ||
| Object.values(EmbeddingModel).forEach((model) => { | ||
| this.logger.log(model); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert class with only static members to a module of helper functions or remove static usage of this.
Static analysis has flagged that this class has only static methods and uses this in a static context. This can be confusing and is considered a code smell. You could:
- Convert the class into a simple module of helper functions.
- Use the class name directly instead of
this.logger.
Below is an example of using the class name for logging:
- this.logger.log(`error when using ${model} api`);
+ localEmbProvider.logger.log(`error when using ${model} api`);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 5-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| export class openAIEmbProvider { | ||
| private static logger = new Logger(openAIEmbProvider.name); | ||
|
|
||
| private static openai = () => { | ||
| return new OpenAI({ | ||
| apiKey: process.env.OPEN_API_KEY, | ||
| }); | ||
| }; | ||
|
|
||
| static async generateEmbResponse(model: string, message: string) { | ||
| const embedding = await this.openai().embeddings.create({ | ||
| model: model, | ||
| input: message, | ||
| encoding_format: 'float', | ||
| }); | ||
| console.log(embedding.data[0].embedding); | ||
| } | ||
|
|
||
| static async getEmbList() { | ||
| try { | ||
| const models = await this.openai().models.list(); | ||
| Object.values(models).filter((model) => model.object === 'embedding'); | ||
| this.logger.log(`Models fetched: ${models.data.length}`); | ||
| } catch (error) { | ||
| this.logger.error('Error fetching models:', error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor static-only class to a module or replace this with class references.
Similar to localEmbProvider, this class contains only static members. Replace this references in static methods with openAIEmbProvider or refactor the class into plain functions for clarity.
- const embedding = await this.openai().embeddings.create({
+ const embedding = await openAIEmbProvider.openai().embeddings.create({Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 26-26: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 30-30: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
llm-server/src/embedding/openai-embedding-provider.ts (1)
31-33: Validate environment variable earlier or default strictly.Right now, the
apiKeyis pulled fromprocess.env.OPEN_API_KEYoroptions.apiKey. Consider throwing an error or logging a warning earlier if neither is found, so users can be guided to provide one before other logic proceeds.llm-server/src/emb-provider.ts (3)
43-52: Extend or streamline provider creation logic.In the
createModelProvidermethod, there's a switch statement for model types. If additional providers are expected, consider modularizing it so new providers can be easily plugged in without large switch statements.
55-66: Enhance error reporting during initialization.When initialization fails, the error is normalized and rethrown. Consider providing user-friendly messages or instructions for recovery (e.g., checking environment variables or network connectivity).
74-84: Validate response stream before calling provider.Since the response is heavily dependent on the provider, you might want to check if the stream has ended before or during long-running processes. Although you do so in the catch block, earlier validation or timeouts could further safeguard the user experience.
llm-server/src/main.ts (2)
26-26: Provide a dedicated route prefix.Embedding requests are posted to
/embedding. If you're planning to extend embedding-related routes (e.g., listing embeddings, retrieving stats, etc.), consider using a dedicated route prefix like/embeddings/…to keep it extensible and RESTful.
31-57: Ensure consistent error responses for the embedding endpoint.In
handleEmbRequest, ifcontentormodelis missing, you send a 400 with an error message. For other exceptions, you set a 500 status, which is consistent. However, if you anticipate different error categories (e.g., provider initialization issues vs. invalid request data), consider returning more descriptive HTTP status codes or structured error objects.backend/src/embedding/local-embedding-provider.ts (3)
5-5: Consider using plain functions instead of a static-only class
Having only static members in a class is a code smell and can reduce clarity. Converting this to standalone utility functions defined in a module or using an object with functions can simplify usage.🧰 Tools
🪛 Biome (1.9.4)
[error] 5-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
18-18: Replacethis.loggerwith explicit class name in static context
Usingthisin a static context may confuse readers. It's clearer to reference the class name directly, for example:localEmbProvider.logger.- this.logger.log(`error when using ${model} api`); + localEmbProvider.logger.log(`error when using ${model} api`);🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
24-24: Replacethis.loggerwith explicit class name in static context
Likewise, replacethis.loggerwithlocalEmbProvider.loggerto avoid confusion in a static context.- this.logger.log(model); + localEmbProvider.logger.log(model);🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
backend/src/common/embedding-provider/index.ts (2)
17-18: Remove use ofthisin static initialization
In static methods or property initializations, referencingthisis confusing. Use the class nameEmbeddingProviderif referencing static fields.- if(!this.instance){ - this.instance = new EmbeddingProvider(new HttpService(), { + if(!EmbeddingProvider.instance){ + EmbeddingProvider.instance = new EmbeddingProvider(new HttpService(), {🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
22-22: Replacethis.instancewithEmbeddingProvider.instance
Again, use the class name explicitly in a static context for better clarity.- return this.instance; + return EmbeddingProvider.instance;🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
backend/src/embedding/__tests__/loadAllEmbModels.spec.ts (1)
6-16: Leverage optional chaining instead of multiple&&checks
You can streamline checks by using optional chaining, though confirm type safety.- if ( - type && - type.constructor && - (type.constructor.name === 'Float32Array' || - type.constructor.name === 'BigInt64Array') - ) { + if ( + type?.constructor?.name === 'Float32Array' + || type?.constructor?.name === 'BigInt64Array' + ) {🧰 Tools
🪛 Biome (1.9.4)
[error] 8-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/src/build-system/__tests__/test.model-provider.spec.ts(1 hunks)backend/src/common/embedding-provider/index.ts(1 hunks)backend/src/embedding/__tests__/loadAllEmbModels.spec.ts(1 hunks)backend/src/embedding/local-embedding-provider.ts(1 hunks)llm-server/package.json(2 hunks)llm-server/src/emb-provider.ts(1 hunks)llm-server/src/embedding/emb-provider.ts(1 hunks)llm-server/src/embedding/openai-embedding-provider.ts(1 hunks)llm-server/src/llm-provider.ts(1 hunks)llm-server/src/main.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/embedding/local-embedding-provider.ts
[error] 5-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 24-24: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
backend/src/common/embedding-provider/index.ts
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
backend/src/embedding/__tests__/loadAllEmbModels.spec.ts
[error] 8-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
llm-server/src/main.ts (1)
118-120: Confirm concurrency between multiple providers.
You’re initializing both the LLM and embedding provider in parallel. Confirm that each provider’s concurrency settings and environment variables do not conflict with each other (e.g., sharing OPEN_API_KEY).
✅ Verification successful
Let me gather more information about the providers' configurations and environment variables.
Let me check the default configurations and queue settings one more time.
No concurrency conflicts found between providers
Based on the code analysis:
- Both providers use separate
PQueueinstances with their own concurrency settings - OpenAI Model Provider defaults to 5 concurrent requests
- Both providers use the same environment variable
OPEN_API_KEYbut this is expected as they're both OpenAI services - Each provider has independent queue configurations (interval, intervalCap) to manage their API rate limits
The parallel initialization and separate queue configurations ensure there won't be conflicts between the providers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Inspect usage of concurrency configuration across all providers
rg --context 3 'concurrency'
Length of output: 2270
Script:
#!/bin/bash
# Check environment variable usage in both providers
rg -A 5 'process.env' llm-server/src/embedding/openai-embedding-provider.ts llm-server/src/model/openai-model-provider.ts
# Check the provider options and initialization
ast-grep --pattern 'class OpenAIEmbeddingProvider {
$$$
initialize() {
$$$
}
$$$
}'
ast-grep --pattern 'class OpenAIModelProvider {
$$$
initialize() {
$$$
}
$$$
}'
# Check the default options for both providers
rg "const DEFAULT_" llm-server/src/embedding/openai-embedding-provider.ts llm-server/src/model/openai-model-provider.ts
Length of output: 1357
Script:
#!/bin/bash
# Check the provider configurations and queue settings
rg -B 3 -A 10 'DEFAULT_' llm-server/src/embedding/openai-embedding-provider.ts llm-server/src/model/openai-model-provider.ts
# Check the provider constructors and their options
ast-grep --pattern 'constructor($$$) {
$$$
}'
# Check queue initialization and usage
rg -B 2 -A 5 'PQueue' llm-server/src/embedding/openai-embedding-provider.ts llm-server/src/model/openai-model-provider.ts
Length of output: 4213
llm-server/src/llm-provider.ts (1)
29-29: Default model choice updated to ‘openai’
Switching the default model from 'llama' to 'openai' could cause downstream logic differences if existing environments expect 'llama'. Double-check existing usage to confirm this new default is desired.
backend/src/embedding/__tests__/loadAllEmbModels.spec.ts (1)
20-32: Test coverage looks good
The test ensures the embedding provider can handle a variety of documents and model references.
llm-server/package.json (1)
21-21: Verify fastembed version compatibility
The addition of fastembed aligns with the embedding support objective. Let's verify the version compatibility.
Run the following script to check for the latest stable version and any potential issues:
✅ Verification successful
✓ fastembed version is up-to-date and dependencies look compatible
The installed version (1.14.1) matches the latest stable version from npm registry. The package's dependencies are well-maintained and use stable versions:
- onnxruntime-node is pinned to 1.15.1
- Other dependencies use appropriate semver ranges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check fastembed package information and compatibility
curl -s https://registry.npmjs.org/fastembed | jq '.["dist-tags"].latest, .versions["1.14.1"].dependencies'
Length of output: 232
| const embedding = await this.openai.embeddings.create({ | ||
| model: params.model, | ||
| input: params.message, | ||
| encoding_format: "float", | ||
| }); | ||
| console.log(embedding.data[0].embedding); |
There was a problem hiding this comment.
Avoid logging full raw embeddings for security and performance.
Logging the entire embedding array could clutter logs and risk unintended exposure of potentially sensitive data. It's safer to log only the length or a truncated version for debugging.
- console.log(embedding.data[0].embedding);
+ this.logger.debug(`Generated an embedding of length: ${embedding.data[0].embedding.length}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const embedding = await this.openai.embeddings.create({ | |
| model: params.model, | |
| input: params.message, | |
| encoding_format: "float", | |
| }); | |
| console.log(embedding.data[0].embedding); | |
| const embedding = await this.openai.embeddings.create({ | |
| model: params.model, | |
| input: params.message, | |
| encoding_format: "float", | |
| }); | |
| this.logger.debug(`Generated an embedding of length: ${embedding.data[0].embedding.length}`); |
| export interface EmbeddingProvider { | ||
| initialize(): Promise<void>; | ||
| generateEmbResponse(params: GenerateMessageParams, | ||
| res: Response,): Promise<void>; | ||
| getEmbList(res: Response): Promise<void>; | ||
| } |
There was a problem hiding this comment.
Confirm alignment with test code usage.
Your interface requires (params, res) arguments in generateEmbResponse, but the new test in test.model-provider.spec.ts calls it with string arguments. Ensure these are aligned to avoid runtime errors or missing responses in actual usage.
| describe('Model Provider Test', () => { | ||
| let embProvider = EmbeddingProvider.getInstance(); | ||
| it('should generate a response from the model provider', async () => { | ||
| let res = await embProvider.generateEmbResponse("Your text string goes here", "text-embedding-3-small"); |
There was a problem hiding this comment.
Mismatch between test usage and interface definition.
The generateEmbResponse method is invoked with two string arguments here, but according to EmbeddingProvider, it requires (GenerateMessageParams, Response). This discrepancy can cause compile-time or runtime errors and indicates that the test may not actually be testing the real interface.
- let res = await embProvider.generateEmbResponse("Your text string goes here", "text-embedding-3-small");
+ // Example fix: if the provider truly expects (params, res)
+ // either mock 'res' or adapt the test to match your real usage:
+ const params = { model: "text-embedding-3-small", message: "Your text string goes here" };
+ let mockRes = makeMockResponse(); // pseudo-code
+ let result = await embProvider.generateEmbResponse(params, mockRes);Committable suggestion skipped: line range outside the PR's diff.
| @@ -31,7 +32,7 @@ | |||
| "eslint": "^8.57.1", | |||
| "eslint-config-prettier": "^9.0.0", | |||
| "eslint-plugin-prettier": "^5.0.0", | |||
| "openai": "^4.68.1", | |||
| "openai": "^4.77.0", | |||
There was a problem hiding this comment.
Critical: Move openai package to main dependencies
The openai package is incorrectly placed in devDependencies. Since it's used for runtime functionality (embeddings), it should be in the main dependencies section to avoid potential production issues.
Apply this change:
"dependencies": {
"@nestjs/common": "^10.4.5",
"express": "^4.21.1",
"fastembed": "^1.14.1",
"node-fetch": "^3.3.2",
"node-llama-cpp": "^3.1.1",
"nodemon": "^3.1.7",
+ "openai": "^4.77.0",
"p-queue": "^8.0.1"
},
"devDependencies": {
"@types/express": "^4.17.13",
"@types/node": "^16.11.12",
"@typescript-eslint/eslint-plugin": "^8.0.0",
"@typescript-eslint/parser": "^8.0.0",
"eslint": "^8.57.1",
"eslint-config-prettier": "^9.0.0",
"eslint-plugin-prettier": "^5.0.0",
- "openai": "^4.77.0",
"prettier": "^3.0.0",
"ts-loader": "^9.5.1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "openai": "^4.77.0", | |
| "dependencies": { | |
| "@nestjs/common": "^10.4.5", | |
| "express": "^4.21.1", | |
| "fastembed": "^1.14.1", | |
| "node-fetch": "^3.3.2", | |
| "node-llama-cpp": "^3.1.1", | |
| "nodemon": "^3.1.7", | |
| "openai": "^4.77.0", | |
| "p-queue": "^8.0.1" | |
| }, | |
| "devDependencies": { | |
| "@types/express": "^4.17.13", | |
| "@types/node": "^16.11.12", | |
| "@typescript-eslint/eslint-plugin": "^8.0.0", | |
| "@typescript-eslint/parser": "^8.0.0", | |
| "eslint": "^8.57.1", | |
| "eslint-config-prettier": "^9.0.0", | |
| "eslint-plugin-prettier": "^5.0.0", | |
| "prettier": "^3.0.0", | |
| "ts-loader": "^9.5.1", |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/src/common/embedding-provider/index.ts (2)
3-4: Remove or rename unused imports to avoid confusion.The imports
rejectsfrom"assert"andresolvefrom"path"do not appear to be used in this file, and the local variables within the promise callback overshadow these imports. This can lead to confusion and potential namespace collisions.-import { rejects } from "assert"; -import { resolve } from "path";
25-48: Avoid mixing console.log with logger and simplify the Observable → Promise conversion.
- Consider using
this.logger.debugorthis.logger.loginstead ofconsole.logfor consistent logging.- Using
res.toPromise()orlastValueFrom(res)(depending on the RxJS version) can streamline code rather than manually bridging the observable to a promise.public async generateEmbResponse(input: string, model: string): Promise<any[] | null>{ try{ const res = this.httpService.post(`${this.config.endpoint}/embedding`, { content: input, model: model, }); - return await new Promise((resolve, rejects) => { - res.subscribe({ - next: (value) => { - console.log(value.data.embedding); - resolve(value.data.embedding); - }, - error: (error) => { - this.logger.error(error); - rejects(null); - } - }) - }) + // Example with lastValueFrom, if RxJS >= 7 + const response = await lastValueFrom(res); + this.logger.debug(response.data.embedding); + return response.data.embedding; } catch (error) { this.logger.error(error); return null; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/build-system/__tests__/test.model-provider.spec.ts(1 hunks)backend/src/common/embedding-provider/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/build-system/tests/test.model-provider.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/common/embedding-provider/index.ts
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (1)
backend/src/common/embedding-provider/index.ts (1)
6-9: Interface name and usage look good.
The ModelProviderConfig interface is concise and typed appropriately, giving future maintainers clarity on the expected configuration properties.
| public static getInstance() { | ||
| if(!this.instance){ | ||
| this.instance = new EmbeddingProvider(new HttpService(), { | ||
| endpoint: 'http://localhost:3001', | ||
| }); | ||
| } | ||
| return this.instance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'this' references in static context with the class name.
Accessing this.instance inside a static method can be unclear. Per lint/complexity rules, prefer using the class name explicitly for improved readability and maintainability.
public static getInstance() {
- if (!this.instance) {
- this.instance = new EmbeddingProvider(new HttpService(), {
+ if (!EmbeddingProvider.instance) {
+ EmbeddingProvider.instance = new EmbeddingProvider(new HttpService(), {
endpoint: 'http://localhost:3001',
});
- }
- return this.instance;
+ }
+ return EmbeddingProvider.instance;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static getInstance() { | |
| if(!this.instance){ | |
| this.instance = new EmbeddingProvider(new HttpService(), { | |
| endpoint: 'http://localhost:3001', | |
| }); | |
| } | |
| return this.instance; | |
| } | |
| public static getInstance() { | |
| if (!EmbeddingProvider.instance) { | |
| EmbeddingProvider.instance = new EmbeddingProvider(new HttpService(), { | |
| endpoint: 'http://localhost:3001', | |
| }); | |
| } | |
| return EmbeddingProvider.instance; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 22-22: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
…ve logging
Summary by CodeRabbit
Release Notes
Configuration Management
Model Download and Management
Embedding Providers
Server Enhancements
Dependency Updates