Merge pull request #5568 from menloresearch/fix/min_p-validation-on-model-load

fix: min p validation on model load

Author: louis-jan

core/src/browser/models/manager.test.ts

@@ -0,0 +1,133 @@
+import { ModelManager } from './manager'
+import { Model, ModelEvent } from '../../types'
+import { events } from '../events'
+
+jest.mock('../events', () => ({
+  events: {
+    emit: jest.fn(),
+  },
+}))
+
+Object.defineProperty(global, 'window', {
+  value: {
+    core: {},
+  },
+  writable: true,
+})
+
+describe('ModelManager', () => {
+  let modelManager: ModelManager
+  let mockModel: Model
+
+  beforeEach(() => {
+    jest.clearAllMocks()
+    ;(global.window as any).core = {}
+    modelManager = new ModelManager()
+    mockModel = {
+      id: 'test-model-1',
+      name: 'Test Model',
+      version: '1.0.0',
+    } as Model
+  })
+
+  describe('constructor', () => {
+    it('should set itself on window.core.modelManager when window exists', () => {
+      expect((global.window as any).core.modelManager).toBe(modelManager)
+    })
+  })
+
+  describe('register', () => {
+    it('should register a new model', () => {
+      modelManager.register(mockModel)
+
+      expect(modelManager.models.has('test-model-1')).toBe(true)
+      expect(modelManager.models.get('test-model-1')).toEqual(mockModel)
+      expect(events.emit).toHaveBeenCalledWith(ModelEvent.OnModelsUpdate, {})
+    })
+
+    it('should merge existing model with new model data', () => {
+      const existingModel: Model = {
+        id: 'test-model-1',
+        name: 'Existing Model',
+        description: 'Existing description',
+      } as Model
+
+      const updatedModel: Model = {
+        id: 'test-model-1',
+        name: 'Updated Model',
+        version: '2.0.0',
+      } as Model
+
+      modelManager.register(existingModel)
+      modelManager.register(updatedModel)
+
+      const registeredModel = modelManager.models.get('test-model-1')
+      expect(registeredModel).toEqual({
+        id: 'test-model-1',
+        name: 'Existing Model',
+        description: 'Existing description',
+        version: '2.0.0',
+      })
+      expect(events.emit).toHaveBeenCalledTimes(2)
+    })
+  })
+
+  describe('get', () => {
+    it('should retrieve a registered model by id', () => {
+      modelManager.register(mockModel)
+
+      const retrievedModel = modelManager.get('test-model-1')
+      expect(retrievedModel).toEqual(mockModel)
+    })
+
+    it('should return undefined for non-existent model', () => {
+      const retrievedModel = modelManager.get('non-existent-model')
+      expect(retrievedModel).toBeUndefined()
+    })
+
+    it('should return correctly typed model', () => {
+      modelManager.register(mockModel)
+
+      const retrievedModel = modelManager.get<Model>('test-model-1')
+      expect(retrievedModel?.id).toBe('test-model-1')
+      expect(retrievedModel?.name).toBe('Test Model')
+    })
+  })
+
+  describe('instance', () => {
+    it('should create a new instance when none exists on window.core', () => {
+      ;(global.window as any).core = {}
+      
+      const instance = ModelManager.instance()
+      expect(instance).toBeInstanceOf(ModelManager)
+      expect((global.window as any).core.modelManager).toBe(instance)
+    })
+
+    it('should return existing instance when it exists on window.core', () => {
+      const existingManager = new ModelManager()
+      ;(global.window as any).core.modelManager = existingManager
+
+      const instance = ModelManager.instance()
+      expect(instance).toBe(existingManager)
+    })
+  })
+
+  describe('models property', () => {
+    it('should initialize with empty Map', () => {
+      expect(modelManager.models).toBeInstanceOf(Map)
+      expect(modelManager.models.size).toBe(0)
+    })
+
+    it('should maintain multiple models', () => {
+      const model1: Model = { id: 'model-1', name: 'Model 1' } as Model
+      const model2: Model = { id: 'model-2', name: 'Model 2' } as Model
+
+      modelManager.register(model1)
+      modelManager.register(model2)
+
+      expect(modelManager.models.size).toBe(2)
+      expect(modelManager.models.get('model-1')).toEqual(model1)
+      expect(modelManager.models.get('model-2')).toEqual(model2)
+    })
+  })
+})

core/src/browser/models/utils.test.ts

@@ -152,6 +152,33 @@ describe('validationRules', () => {
     expect(validationRules.text_model('true')).toBe(false)
     expect(validationRules.text_model(1)).toBe(false)
   })
+
+  it('should validate repeat_last_n correctly', () => {
+    expect(validationRules.repeat_last_n(5)).toBe(true)
+    expect(validationRules.repeat_last_n(-5)).toBe(true)
+    expect(validationRules.repeat_last_n(0)).toBe(true)
+    expect(validationRules.repeat_last_n(1.5)).toBe(true)
+    expect(validationRules.repeat_last_n('5')).toBe(false)
+    expect(validationRules.repeat_last_n(null)).toBe(false)
+  })
+
+  it('should validate repeat_penalty correctly', () => {
+    expect(validationRules.repeat_penalty(1.1)).toBe(true)
+    expect(validationRules.repeat_penalty(0.9)).toBe(true)
+    expect(validationRules.repeat_penalty(0)).toBe(true)
+    expect(validationRules.repeat_penalty(-1)).toBe(true)
+    expect(validationRules.repeat_penalty('1.1')).toBe(false)
+    expect(validationRules.repeat_penalty(null)).toBe(false)
+  })
+
+  it('should validate min_p correctly', () => {
+    expect(validationRules.min_p(0.1)).toBe(true)
+    expect(validationRules.min_p(0)).toBe(true)
+    expect(validationRules.min_p(-0.1)).toBe(true)
+    expect(validationRules.min_p(1.5)).toBe(true)
+    expect(validationRules.min_p('0.1')).toBe(false)
+    expect(validationRules.min_p(null)).toBe(false)
+  })
 })
 
 it('should normalize invalid values for keys not listed in validationRules', () => {
@@ -192,18 +219,125 @@ describe('normalizeValue', () => {
     expect(normalizeValue('cpu_threads', '4')).toBe(4)
     expect(normalizeValue('cpu_threads', 0)).toBe(0)
   })
-})
 
-it('should handle invalid values correctly by falling back to originParams', () => {
-  const modelParams = { temperature: 'invalid', token_limit: -1 }
-  const originParams = { temperature: 0.5, token_limit: 100 }
-  expect(extractInferenceParams(modelParams as any, originParams)).toEqual(originParams)
+  it('should handle edge cases for normalization', () => {
+    expect(normalizeValue('ctx_len', -5.7)).toBe(-6)
+    expect(normalizeValue('token_limit', 'abc')).toBeNaN()
+    expect(normalizeValue('max_tokens', null)).toBe(0)
+    expect(normalizeValue('ngl', undefined)).toBeNaN()
+    expect(normalizeValue('n_parallel', Infinity)).toBe(Infinity)
+    expect(normalizeValue('cpu_threads', -Infinity)).toBe(-Infinity)
+  })
+
+  it('should not normalize non-integer parameters', () => {
+    expect(normalizeValue('temperature', 1.5)).toBe(1.5)
+    expect(normalizeValue('top_p', 0.9)).toBe(0.9)
+    expect(normalizeValue('stream', true)).toBe(true)
+    expect(normalizeValue('prompt_template', 'template')).toBe('template')
+  })
 })
 
-it('should return an empty object when no modelParams are provided', () => {
-  expect(extractModelLoadParams()).toEqual({})
+describe('extractInferenceParams', () => {
+  it('should handle invalid values correctly by falling back to originParams', () => {
+    const modelParams = { temperature: 'invalid', token_limit: -1 }
+    const originParams = { temperature: 0.5, token_limit: 100 }
+    expect(extractInferenceParams(modelParams as any, originParams)).toEqual(originParams)
+  })
+
+  it('should return an empty object when no modelParams are provided', () => {
+    expect(extractInferenceParams()).toEqual({})
+  })
+
+  it('should extract and normalize valid inference parameters', () => {
+    const modelParams = {
+      temperature: 1.5,
+      token_limit: 100.7,
+      top_p: 0.9,
+      stream: true,
+      max_tokens: 50.3,
+      invalid_param: 'should_be_ignored'
+    }
+    
+    const result = extractInferenceParams(modelParams as any)
+    expect(result).toEqual({
+      temperature: 1.5,
+      token_limit: 100,
+      top_p: 0.9,
+      stream: true,
+      max_tokens: 50
+    })
+  })
+
+  it('should handle parameters without validation rules', () => {
+    const modelParams = { engine: 'llama' }
+    const result = extractInferenceParams(modelParams as any)
+    expect(result).toEqual({ engine: 'llama' })
+  })
+
+  it('should skip invalid values when no origin params provided', () => {
+    const modelParams = { temperature: 'invalid', top_p: 0.8 }
+    const result = extractInferenceParams(modelParams as any)
+    expect(result).toEqual({ top_p: 0.8 })
+  })
 })
 
-it('should return an empty object when no modelParams are provided', () => {
-  expect(extractInferenceParams()).toEqual({})
+describe('extractModelLoadParams', () => {
+  it('should return an empty object when no modelParams are provided', () => {
+    expect(extractModelLoadParams()).toEqual({})
+  })
+
+  it('should extract and normalize valid model load parameters', () => {
+    const modelParams = {
+      ctx_len: 2048.5,
+      ngl: 12.7,
+      embedding: true,
+      n_parallel: 4.2,
+      cpu_threads: 8.9,
+      prompt_template: 'template',
+      llama_model_path: '/path/to/model',
+      vision_model: false,
+      invalid_param: 'should_be_ignored'
+    }
+    
+    const result = extractModelLoadParams(modelParams as any)
+    expect(result).toEqual({
+      ctx_len: 2048,
+      ngl: 12,
+      embedding: true,
+      n_parallel: 4,
+      cpu_threads: 8,
+      prompt_template: 'template',
+      llama_model_path: '/path/to/model',
+      vision_model: false
+    })
+  })
+
+  it('should handle parameters without validation rules', () => {
+    const modelParams = { 
+      engine: 'llama',
+      pre_prompt: 'System:',
+      system_prompt: 'You are helpful',
+      model_path: '/path'
+    }
+    const result = extractModelLoadParams(modelParams as any)
+    expect(result).toEqual({
+      engine: 'llama',
+      pre_prompt: 'System:',
+      system_prompt: 'You are helpful',
+      model_path: '/path'
+    })
+  })
+
+  it('should fall back to origin params for invalid values', () => {
+    const modelParams = { ctx_len: -1, ngl: 'invalid' }
+    const originParams = { ctx_len: 2048, ngl: 12 }
+    const result = extractModelLoadParams(modelParams as any, originParams)
+    expect(result).toEqual({})
+  })
+
+  it('should skip invalid values when no origin params provided', () => {
+    const modelParams = { ctx_len: -1, embedding: true }
+    const result = extractModelLoadParams(modelParams as any)
+    expect(result).toEqual({ embedding: true })
+  })
 })

core/src/browser/models/utils.ts

@@ -17,9 +17,10 @@ export const validationRules: { [key: string]: (value: any) => boolean } = {
   presence_penalty: (value: any) => typeof value === 'number' && value >= 0 && value <= 1,
   repeat_last_n: (value: any) => typeof value === 'number',
   repeat_penalty: (value: any) => typeof value === 'number',
+  min_p: (value: any) => typeof value === 'number',
 
   ctx_len: (value: any) => Number.isInteger(value) && value >= 0,
-  ngl: (value: any) => Number.isInteger(value),
+  ngl: (value: any) => Number.isInteger(value) && value >= 0,
   embedding: (value: any) => typeof value === 'boolean',
   n_parallel: (value: any) => Number.isInteger(value) && value >= 0,
   cpu_threads: (value: any) => Number.isInteger(value) && value >= 0,