Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: resolve model list merge issue during app update #77

Merged
merged 2 commits into from
Nov 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions android/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ android {
applicationId "com.pocketpalai"
minSdkVersion rootProject.ext.minSdkVersion
targetSdkVersion rootProject.ext.targetSdkVersion
versionCode 11
versionName "1.4.4"
versionCode 12
versionName "1.4.5"
ndk {
abiFilters "arm64-v8a", "x86_64"
}
Expand Down
8 changes: 4 additions & 4 deletions ios/PocketPal.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@
buildSettings = {
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
CLANG_ENABLE_MODULES = YES;
CURRENT_PROJECT_VERSION = 11;
CURRENT_PROJECT_VERSION = 12;
DEVELOPMENT_TEAM = MYXGXY23Y6;
ENABLE_BITCODE = NO;
INFOPLIST_FILE = PocketPal/Info.plist;
Expand All @@ -488,7 +488,7 @@
"$(inherited)",
"@executable_path/Frameworks",
);
MARKETING_VERSION = 1.4.4;
MARKETING_VERSION = 1.4.5;
OTHER_CPLUSPLUSFLAGS = (
"$(OTHER_CFLAGS)",
"-DFOLLY_NO_CONFIG",
Expand Down Expand Up @@ -521,15 +521,15 @@
buildSettings = {
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
CLANG_ENABLE_MODULES = YES;
CURRENT_PROJECT_VERSION = 11;
CURRENT_PROJECT_VERSION = 12;
DEVELOPMENT_TEAM = MYXGXY23Y6;
INFOPLIST_FILE = PocketPal/Info.plist;
IPHONEOS_DEPLOYMENT_TARGET = 14.0;
LD_RUNPATH_SEARCH_PATHS = (
"$(inherited)",
"@executable_path/Frameworks",
);
MARKETING_VERSION = 1.4.4;
MARKETING_VERSION = 1.4.5;
OTHER_CPLUSPLUSFLAGS = (
"$(OTHER_CFLAGS)",
"-DFOLLY_NO_CONFIG",
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "PocketPal",
"version": "1.4.4",
"version": "1.4.5",
"private": true,
"scripts": {
"prepare": "husky",
Expand Down
28 changes: 16 additions & 12 deletions src/store/ModelStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {computed, makeAutoObservable, ObservableMap, runInAction} from 'mobx';
import {CompletionParams, LlamaContext, initLlama} from '@pocketpalai/llama.rn';
import AsyncStorage from '@react-native-async-storage/async-storage';

import {bytesToGB, hasEnoughSpace} from '../utils';
import {bytesToGB, deepMerge, hasEnoughSpace} from '../utils';
import {defaultModels, MODEL_LIST_VERSION} from './defaultModels';

import {chatTemplates} from '../utils/chat';
Expand Down Expand Up @@ -78,20 +78,24 @@ class ModelStore {
const existingModelIndex = mergedModels.findIndex(
m => m.id === defaultModel.id,
);
// console.log('existingModelIndex: ', existingModelIndex);

if (existingModelIndex !== -1) {
// Merge existing model with new defaults
mergedModels[existingModelIndex] = {
...defaultModel, // Defaults take precedence (for new fields or updates)
...mergedModels[existingModelIndex], // User changes override defaults
chatTemplate:
mergedModels[existingModelIndex].chatTemplate ||
defaultModel.chatTemplate,
completionSettings:
mergedModels[existingModelIndex].completionSettings ||
defaultModel.completionSettings,
};
const existingModel = mergedModels[existingModelIndex];

// Deep merge chatTemplate and completionSettings
existingModel.chatTemplate = deepMerge(
existingModel.chatTemplate || {},
defaultModel.chatTemplate || {},
);

existingModel.completionSettings = deepMerge(
existingModel.completionSettings || {},
defaultModel.completionSettings || {},
);

// Merge other properties
mergedModels[existingModelIndex] = existingModel;
} else {
// Add new model if it doesn't exist
mergedModels.push(defaultModel);
Expand Down
80 changes: 80 additions & 0 deletions src/store/__tests__/ModelStore.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
jest.unmock('../ModelStore'); // This is not really needed, as only importing from store is mocked.
import {modelStore} from '../ModelStore';
import {runInAction} from 'mobx';
import {defaultModels} from '../defaultModels';

describe('ModelStore', () => {
beforeEach(() => {
jest.clearAllMocks();
modelStore.models = []; // Clear models before each test
});

describe('mergeModelLists', () => {
it('should add missing default models to the existing model list', () => {
modelStore.models = []; // Start with no existing models

runInAction(() => {
modelStore.mergeModelLists();
});

expect(modelStore.models.length).toBeGreaterThan(0);
expect(modelStore.models).toEqual(expect.arrayContaining(defaultModels));
});

it('should merge existing models with default models, adding any that are missing', () => {
const notExistedModel = defaultModels[0];
modelStore.models = defaultModels.slice(1); // Start with all but the first model, so we can test if it's added back
expect(modelStore.models.length).toBe(defaultModels.length - 1);
expect(modelStore.models).not.toContainEqual(notExistedModel);

runInAction(() => {
modelStore.mergeModelLists();
});

expect(modelStore.models.length).toBeGreaterThan(0);
expect(modelStore.models).toContainEqual(notExistedModel);
});

it('should retain value of existing variables while merging new variables', () => {
const newDefaultModel = defaultModels[0];
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const {temperature, ...completionSettingsWithoutTemperature} =
newDefaultModel.completionSettings; // Exclude temperature

// Apply changes to the existing model:
// - chatTemplate.template: existing variable with a value different from the default
// - completionSettings.n_predict: existing variable with a value different from the default
// - temperature: new variable in the default model - not present in the existing model
const existingModel = {
...newDefaultModel,
chatTemplate: {
...newDefaultModel.chatTemplate,
template: 'existing',
},
completionSettings: {
...completionSettingsWithoutTemperature, // Use the completionSettings without temperature - simulates new parameters
n_predict: 101010,
},
};

modelStore.models[0] = existingModel;

runInAction(() => {
modelStore.mergeModelLists();
});

expect(modelStore.models[0].chatTemplate).toEqual(
expect.objectContaining({
template: 'existing', // Existing value should remain
}),
);

expect(modelStore.models[0].completionSettings).toEqual(
expect.objectContaining({
n_predict: 101010, // Existing value should remain
temperature: newDefaultModel.completionSettings.temperature, // Non-existing value should be merged
}),
);
});
});
});
2 changes: 1 addition & 1 deletion src/store/defaultModels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {Model} from '../utils/types';
import {chatTemplates, defaultCompletionParams} from '../utils/chat';
import {Platform} from 'react-native';

export const MODEL_LIST_VERSION = 5;
export const MODEL_LIST_VERSION = 6;

export const defaultModels: Model[] = [
{
Expand Down
75 changes: 74 additions & 1 deletion src/utils/__tests__/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {formatBytes, getTextSizeInBytes, unwrap} from '..';
import {deepMerge, formatBytes, getTextSizeInBytes, unwrap} from '..';

describe('formatBytes', () => {
it('formats bytes correctly when the size is 0', () => {
Expand Down Expand Up @@ -38,3 +38,76 @@ describe('unwrap', () => {
expect(unwrap(prop)).toStrictEqual(prop);
});
});

describe('deepMerge', () => {
it('should merge two flat objects', () => {
const target = {a: 1, b: 2};
const source = {b: 3, c: 4};
const result = deepMerge(target, source);
expect(result).toEqual({a: 1, b: 2, c: 4}); // b should remain 2
});

it('should merge nested objects', () => {
const target = {a: {b: 1}};
const source = {a: {c: 2}};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: 1, c: 2}}); // c should be added
});

it('should overwrite nested properties', () => {
const target = {a: {b: 1, c: 2}};
const source = {a: {b: 3}};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: 1, c: 2}}); // b should remain 1
});

it('should handle arrays correctly', () => {
const target = {a: [1, 2]};
const source = {a: [3, 4]};
const result = deepMerge(target, source);
expect(result).toEqual({a: [1, 2]});
});

it('should handle null values', () => {
const target = {a: null};
const source = {a: {b: 1}};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: 1}}); // Replaces null with the object
});

it('should handle flat to nested', () => {
const target = {a: 1, c: {d: 2, e: 3}};
const source = {a: {b: 1}, c: 4};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: 1}, c: 4});
});

it('should not modify the original objects', () => {
const target = {a: 1};
const source = {b: 2};
deepMerge(target, source);
expect(target).toEqual({a: 1, b: 2});
expect(source).toEqual({b: 2}); // Source should remain unchanged
});

it('should handle empty objects', () => {
const target = {};
const source = {a: 1};
const result = deepMerge(target, source);
expect(result).toEqual({a: 1}); // Merges from source
});

it('should handle deeply nested objects', () => {
const target = {a: {b: {c: 1}}};
const source = {a: {b: {d: 2}}};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: {c: 1, d: 2}}});
});

it('should merge multiple levels of nesting', () => {
const target = {a: {b: {c: {d: 1, e: 3}}}};
const source = {a: {b: {c: {e: 2, f: 4}}}};
const result = deepMerge(target, source);
expect(result).toEqual({a: {b: {c: {d: 1, e: 3, f: 4}}}});
});
});
28 changes: 28 additions & 0 deletions src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,4 +313,32 @@ export async function hasEnoughSpace(model: Model): Promise<boolean> {
}
}

/**
* Merges properties from the source object into the target object deeply.
* Only sets properties that do not already exist in the target or if the types differ.
*
* @param target - The target object to merge properties into.
* @param source - The source object from which properties are taken.
* @returns The updated target object after merging.
*/
export const deepMerge = (target: any, source: any): any => {
for (const key in source) {
if (source.hasOwnProperty(key)) {
if (typeof source[key] === 'object' && source[key] !== null) {
// If the property is an object, recursively merge.
// If target[key] is not an object, it means the property type is different so we will replace it.
target[key] =
target[key] && typeof target[key] === 'object' ? target[key] : {};
deepMerge(target[key], source[key]);
} else {
// Set the property in the target only if it doesn't exist or if the types differ
if (!(key in target) || typeof target[key] !== typeof source[key]) {
target[key] = source[key];
}
}
}
}
return target;
};

export const randId = () => Math.random().toString(36).substring(2, 11);
Loading