feat(backend): Backend config convention and pull model #54
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 enhancement to the backend's model management system by adding new dependencies and implementing a robust configuration and model downloading mechanism. The changes include integrating Hugging Face libraries, Axios, and Lodash, creating a Changes
Sequence DiagramsequenceDiagram
participant Main as Main Application
participant ConfigLoader as Config Loader
participant ModelDownloader as Model Downloader
participant HuggingFace as Hugging Face
Main->>ConfigLoader: Load Chat Configurations
ConfigLoader-->>Main: Return Configuration
Main->>ModelDownloader: Initialize Downloader
ModelDownloader->>HuggingFace: Download Models
HuggingFace-->>ModelDownloader: Return Downloaded Models
ModelDownloader-->>Main: Models Ready
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
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 (
|
backend/src/config/config.service.ts
Outdated
There was a problem hiding this comment.
the name of this file should be something different, it's not file relate to nestjs, we only do nestjs convention when we doing something relate to nestjs
change to ConfigLoader also with default-config-type.d.ts
Sma1lboy
changed the title
…oy/GeneratorAI into backend-config-convention
…oy/codefox into backend-config-convention
…and adjust ChatsConfig structure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
backend/src/main.ts (1)
20-21: Consider graceful startup handlingThe current implementation blocks server startup until all models are downloaded. This could lead to:
- Long startup times affecting deployment and scaling
- No way to handle partial model download failures
Consider:
- Implementing a health check endpoint that reports model download status
- Starting the server immediately and downloading models in the background
- Adding a retry mechanism for failed model downloads
backend/src/model/model-downloader.ts (2)
16-21: Add model management and validationThe downloadModel method needs improvements in several areas:
- Memory management for loaded models
- Validation of task types
- Error handling for invalid models
Consider:
- Implementing a model cache with size limits
- Adding task type validation
- Implementing proper cleanup for unused models
- Adding timeout handling for downloads
Would you like me to provide a detailed implementation for any of these improvements?
23-30: Enhance local model retrieval with validationThe getLocalModel method should validate the existence of local models before attempting to load them.
Consider adding validation:
public async getLocalModel(task: string, model: string): Promise<any> { + const modelPath = getModelPath(model); + if (!fs.existsSync(modelPath)) { + throw new Error(`Local model not found: ${model}`); + } const pipelineInstance = await pipeline(task as PipelineType, model, { local_files_only: true, revision: 'local', }); return pipelineInstance; }backend/src/config/config-loader.ts (1)
5-11: Add JSDoc documentation and consider required fieldsThe interface would benefit from documentation explaining the purpose of each field. Additionally, consider making the
taskfield required since it appears to be essential for model loading based on the test file.+/** + * Configuration interface for chat models + */ export interface ChatConfig { model: string; endpoint?: string; token?: string; default?: boolean; - task?: string; + task: string; // Required for model loading }backend/src/model/__tests__/loadAllChatsModels.spec.ts (1)
22-36: Remove commented-out codeRemove the commented-out mock implementation as it's no longer needed and clutters the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
backend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
backend/package.json(2 hunks)backend/src/config/common-path.ts(1 hunks)backend/src/config/config-loader.ts(1 hunks)backend/src/main.ts(2 hunks)backend/src/model/__tests__/app.e2e-spec.ts(1 hunks)backend/src/model/__tests__/loadAllChatsModels.spec.ts(1 hunks)backend/src/model/model-downloader.ts(1 hunks)backend/src/model/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/src/model/tests/app.e2e-spec.ts
- backend/src/config/common-path.ts
🧰 Additional context used
🪛 Biome (1.9.4)
backend/src/model/__tests__/loadAllChatsModels.spec.ts
[error] 12-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
backend/src/model/utils.ts (1)
7-7: Verify the empty configuration path
The empty string path in configLoader.get<ChatConfig[]>('') seems suspicious. This might be a mistake or needs documentation explaining why an empty path is valid.
backend/src/model/__tests__/loadAllChatsModels.spec.ts (1)
52-52: Review test timeouts and consider performance optimization
The test has a very long timeout (600000ms = 10 minutes). This could indicate performance issues with model loading.
Let's analyze the model loading time:
Consider:
- Implementing model caching
- Loading models asynchronously during application startup
- Adding progress indicators for better test feedback
Also applies to: 85-85
✅ Verification successful
Long test timeout is justified for model download and inference
The 10-minute timeout is appropriate in this context as the test:
- Downloads an ML model (
downloadAllModels()) - Loads the model into memory
- Performs actual inference with the model to generate text
These operations are inherently time-consuming for ML models, and the timeout isn't indicative of a performance issue but rather the nature of the test which performs real model operations. The test also includes proper error handling and logging to help diagnose any issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Measure model loading time
time node -e "
const { ModelDownloader } = require('./dist/model/model-downloader');
const downloader = ModelDownloader.getInstance();
(async () => {
console.time('modelLoad');
await downloader.getLocalModel(
'text2text-generation',
'Felladrin/onnx-flan-alpaca-base'
);
console.timeEnd('modelLoad');
})()
"
Length of output: 1179
Script:
#!/bin/bash
# Let's examine the test file and its context to understand the long timeout
cat backend/src/model/__tests__/loadAllChatsModels.spec.ts
Length of output: 2640
| const loadPromises = chats.map(async (chatConfig: ChatConfig) => { | ||
| const { model, task } = chatConfig; | ||
| try { | ||
| downloader.logger.log(model, task); | ||
| const pipelineInstance = await downloader.downloadModel(task, model); | ||
| } catch (error) { | ||
| downloader.logger.error(`Failed to load model ${model}:`, error.message); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add error handling and type safety improvements
Several issues need attention in the model loading logic:
- The unused
pipelineInstancevariable - No type safety for the
taskparameter - Errors are caught but the function continues silently
Consider this improved implementation:
- const loadPromises = chats.map(async (chatConfig: ChatConfig) => {
+ type SupportedTask = 'text-generation' | 'sentiment-analysis'; // add all supported tasks
+ const loadPromises = chats.map(async (chatConfig: ChatConfig) => {
const { model, task } = chatConfig;
+ if (!isValidTask(task)) {
+ throw new Error(`Unsupported task: ${task}`);
+ }
try {
downloader.logger.log(model, task);
- const pipelineInstance = await downloader.downloadModel(task, model);
+ await downloader.downloadModel(task as SupportedTask, model);
} catch (error) {
downloader.logger.error(`Failed to load model ${model}:`, error.message);
+ throw error; // re-throw to indicate failure
}
});
- await Promise.all(loadPromises);
+ const results = await Promise.allSettled(loadPromises);
+ const failures = results.filter(r => r.status === 'rejected');
+ if (failures.length > 0) {
+ throw new Error(`Failed to load ${failures.length} models`);
+ }Committable suggestion skipped: line range outside the PR's diff.
| public static getInstance(): ModelDownloader { | ||
| if (!ModelDownloader.instance) { | ||
| ModelDownloader.instance = new ModelDownloader(); | ||
| } | ||
| return ModelDownloader.instance; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve singleton implementation for thread safety
The current singleton implementation might have race conditions in a concurrent environment.
Consider using a more robust singleton pattern:
- private static instance: ModelDownloader;
- public static getInstance(): ModelDownloader {
- if (!ModelDownloader.instance) {
- ModelDownloader.instance = new ModelDownloader();
- }
- return ModelDownloader.instance;
- }
+ private static instance: ModelDownloader | null = null;
+ private static instanceLock = false;
+ public static getInstance(): ModelDownloader {
+ if (!ModelDownloader.instance) {
+ if (ModelDownloader.instanceLock) {
+ throw new Error('Instance creation in progress');
+ }
+ ModelDownloader.instanceLock = true;
+ try {
+ ModelDownloader.instance = new ModelDownloader();
+ } finally {
+ ModelDownloader.instanceLock = false;
+ }
+ }
+ return ModelDownloader.instance;
+ }Committable suggestion skipped: line range outside the PR's diff.
backend/src/config/config-loader.ts
Outdated
| validateConfig() { | ||
| if (!this.chatsConfig) { | ||
| throw new Error("Invalid configuration: 'chats' section is missing."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance configuration validation
The current validation only checks if chatsConfig exists. Consider adding more comprehensive validation.
validateConfig() {
if (!this.chatsConfig) {
throw new Error("Invalid configuration: 'chats' section is missing.");
}
+ if (!Array.isArray(this.chatsConfig)) {
+ throw new Error("Invalid configuration: 'chats' must be an array.");
+ }
+ for (const config of this.chatsConfig) {
+ if (!config.model) {
+ throw new Error("Invalid configuration: 'model' is required.");
+ }
+ if (!config.task) {
+ throw new Error("Invalid configuration: 'task' is required.");
+ }
+ }
}📝 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.
| validateConfig() { | |
| if (!this.chatsConfig) { | |
| throw new Error("Invalid configuration: 'chats' section is missing."); | |
| } | |
| } | |
| validateConfig() { | |
| if (!this.chatsConfig) { | |
| throw new Error("Invalid configuration: 'chats' section is missing."); | |
| } | |
| if (!Array.isArray(this.chatsConfig)) { | |
| throw new Error("Invalid configuration: 'chats' must be an array."); | |
| } | |
| for (const config of this.chatsConfig) { | |
| if (!config.model) { | |
| throw new Error("Invalid configuration: 'model' is required."); | |
| } | |
| if (!config.task) { | |
| throw new Error("Invalid configuration: 'task' is required."); | |
| } | |
| } | |
| } |
backend/src/config/config-loader.ts
Outdated
| get<T>(path: string) { | ||
| if (!path) { | ||
| return this.chatsConfig as unknown as T; | ||
| } | ||
| return _.get(this.chatsConfig, path) as T; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in get method
The generic get method could be made more type-safe by constraining the return type based on the path.
- get<T>(path: string) {
+ get<T extends keyof ChatConfig>(path: T): ChatConfig[T];
+ get<T>(path: string): T;
if (!path) {
- return this.chatsConfig as unknown as T;
+ return this.chatsConfig as T;
}
return _.get(this.chatsConfig, path) as T;
}Committable suggestion skipped: line range outside the PR's diff.
backend/src/config/config-loader.ts
Outdated
| private loadConfig() { | ||
| const file = fs.readFileSync(this.configPath, 'utf-8'); | ||
|
|
||
| this.chatsConfig = JSON.parse(file); | ||
| console.log('Raw file content:', this.chatsConfig); | ||
| } |
There was a problem hiding this comment.
Add error handling and consider async file operations
The loadConfig method uses synchronous file operations and lacks error handling. This could cause issues in production.
- private loadConfig() {
+ private loadConfig(): void {
+ try {
const file = fs.readFileSync(this.configPath, 'utf-8');
this.chatsConfig = JSON.parse(file);
console.log('Raw file content:', this.chatsConfig);
+ } catch (error) {
+ console.error('Failed to load config:', error);
+ this.chatsConfig = [];
+ throw new Error(`Failed to load 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 loadConfig() { | |
| const file = fs.readFileSync(this.configPath, 'utf-8'); | |
| this.chatsConfig = JSON.parse(file); | |
| console.log('Raw file content:', this.chatsConfig); | |
| } | |
| private loadConfig(): void { | |
| try { | |
| const file = fs.readFileSync(this.configPath, 'utf-8'); | |
| this.chatsConfig = JSON.parse(file); | |
| console.log('Raw file content:', this.chatsConfig); | |
| } catch (error) { | |
| console.error('Failed to load config:', error); | |
| this.chatsConfig = []; | |
| throw new Error(`Failed to load configuration: ${error.message}`); | |
| } | |
| } |
backend/package.json
Outdated
| "@huggingface/hub": "latest", | ||
| "@huggingface/transformers": "latest", |
There was a problem hiding this comment.
Pin Hugging Face dependencies to specific versions
Using "latest" for production dependencies is risky as it can lead to unexpected breaking changes. Consider pinning to specific versions.
- "@huggingface/hub": "latest",
- "@huggingface/transformers": "latest",
+ "@huggingface/hub": "^0.14.1",
+ "@huggingface/transformers": "^2.15.0",Committable suggestion skipped: line range outside the PR's diff.
| const originalIsArray = Array.isArray; | ||
|
|
||
| Array.isArray = jest.fn((type: any): type is any[] => { | ||
| if ( | ||
| type && | ||
| type.constructor && | ||
| (type.constructor.name === 'Float32Array' || | ||
| type.constructor.name === 'BigInt64Array') | ||
| ) { | ||
| return true; | ||
| } | ||
| return originalIsArray(type); | ||
| }) as unknown as (arg: any) => arg is any[]; |
There was a problem hiding this comment.
Avoid modifying global Array.isArray
Modifying the global Array.isArray is risky and could affect other tests. Consider using a more isolated approach.
- const originalIsArray = Array.isArray;
- Array.isArray = jest.fn((type: any): type is any[] => {
+ const isCustomArray = (type: any): type is any[] => {
if (
type &&
type.constructor &&
(type.constructor.name === 'Float32Array' ||
type.constructor.name === 'BigInt64Array')
) {
return true;
}
- return originalIsArray(type);
+ return Array.isArray(type);
- }) as unknown as (arg: any) => arg is any[];
+ };Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Summary by CodeRabbit
New Features
Bug Fixes
Tests