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: user catalog #916

Merged
merged 6 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
6 changes: 3 additions & 3 deletions packages/backend/src/managers/catalogManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ test('expect correct model is returned with valid id when the user catalog is va
vi.spyOn(promises, 'readFile').mockResolvedValue(JSON.stringify(userContent));

catalogManager.init();
await vi.waitUntil(() => catalogManager.getRecipes().length > 0);
await vi.waitUntil(() => catalogManager.getModels().some(model => model.id === 'model1'));

const model = catalogManager.getModelById('model1');
expect(model).toBeDefined();
Expand Down Expand Up @@ -176,7 +176,7 @@ test('expect to call writeFile in addLocalModelsToCatalog with catalog updated',
});
const writeFileMock = vi.spyOn(promises, 'writeFile').mockResolvedValue();

await catalogManager.addLocalModelsToCatalog([
await catalogManager.importUserModels([
{
name: 'custom-model',
path: '/root/path/file.gguf',
Expand All @@ -199,7 +199,7 @@ test('expect to call writeFile in removeLocalModelFromCatalog with catalog updat
const updatedCatalog: ApplicationCatalog = Object.assign({}, userContent);
updatedCatalog.models = updatedCatalog.models.filter(m => m.id !== 'model1');

await catalogManager.removeLocalModelFromCatalog('model1');
await catalogManager.removeUserModel('model1');

expect(writeFileMock).toBeCalledWith('path', JSON.stringify(updatedCatalog, undefined, 2), 'utf-8');
});
189 changes: 147 additions & 42 deletions packages/backend/src/managers/catalogManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
***********************************************************************/

import type { ApplicationCatalog } from '@shared/src/models/IApplicationCatalog';
import { promises } from 'node:fs';
import fs, { promises } from 'node:fs';
import path from 'node:path';
import defaultCatalog from '../assets/ai.json';
import type { Recipe } from '@shared/src/models/IRecipe';
Expand All @@ -30,6 +30,8 @@ import type { LocalModelImportInfo } from '@shared/src/models/ILocalModelInfo';

export type catalogUpdateHandle = () => void;

export const USER_CATALOG = 'user-catalog.json';

export class CatalogManager extends Publisher<ApplicationCatalog> implements Disposable {
private catalog: ApplicationCatalog;
#catalogUpdateListeners: catalogUpdateHandle[];
Expand All @@ -51,31 +53,88 @@ export class CatalogManager extends Publisher<ApplicationCatalog> implements Dis
this.#disposables = [];
}

/**
* The init method will start a watcher on the user catalog.json
*/
init(): void {
// Creating a json watcher
const jsonWatcher: JsonWatcher<ApplicationCatalog> = new JsonWatcher(
path.resolve(this.appUserDirectory, 'catalog.json'),
defaultCatalog,
);
const jsonWatcher: JsonWatcher<ApplicationCatalog> = new JsonWatcher(this.getUserCatalogPath(), {
recipes: [],
models: [],
categories: [],
});
jsonWatcher.onContentUpdated(content => this.onCatalogUpdated(content));
jsonWatcher.init();

this.#disposables.push(jsonWatcher);
}

private loadDefaultCatalog(): void {
this.catalog = defaultCatalog;
this.notify();
}

private onCatalogUpdated(content: ApplicationCatalog): void {
// when reading the content on the catalog, the creation is just a string and we need to convert it back to a Date object
content.models
.filter(m => m.file?.creation)
.forEach(m => {
if (m.file?.creation) {
m.file.creation = new Date(m.file.creation);
if (typeof content !== 'object' || !('models' in content) || typeof content.models !== 'object') {
this.loadDefaultCatalog();
return;
}

const sanitize = this.sanitize(content);
this.catalog = {
models: [...defaultCatalog.models, ...sanitize.models],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we give priority on the bundled catalog, what is user wants to override a recipe or model

Copy link
Contributor Author

@axel7083 axel7083 Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He can't. What would be the use case ?

ℹ️ There is no priority as it is an array. All recipe provided will appear, potential issue could append if multiple recipe/models have the same id however, do you want to fix that by giving the priority to the user's content ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check if there are multiple items with the same id. In the rare case that it may happen we could have issues as we manage the models as a map.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He can't. What would be the use case ?

ℹ️ There is no priority as it is an array. All recipe provided will appear, potential issue could append if multiple recipe/models have the same id however, do you want to fix that by giving the priority to the user's content ?

Overriding a recipe because either you want to test a specific commit (more devs) or add a new model to a recipe

Copy link
Contributor

@lstocchi lstocchi Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the goal it already works this way. If you have in the user catalog an entry with an id = one item in the default catalog you will see the custom one. Atleast for the models

recipes: [...defaultCatalog.recipes, ...sanitize.recipes],
categories: [...defaultCatalog.categories, ...sanitize.categories],
};

this.notify();
}

private sanitize(content: unknown): ApplicationCatalog {
const output: ApplicationCatalog = {
recipes: [],
models: [],
categories: [],
};

if (!content || typeof content !== 'object') {
console.warn('malformed application catalog content');
return output;
}

// ensure user's models are properly formatted
if ('models' in content && typeof content.models === 'object' && Array.isArray(content.models)) {
output.models = content.models.map(model => {
// parse the creation date properly
if (model.file?.creation) {
return {
...model,
file: {
...model.file,
creation: new Date(model.file.creation),
},
};
}
return model;
});
this.catalog = content;
}

// ensure user's recipes are properly formatted
if ('recipes' in content && typeof content.recipes === 'object' && Array.isArray(content.recipes)) {
output.recipes = content.recipes;
}

// ensure user's categories are properly formatted
if ('categories' in content && typeof content.categories === 'object' && Array.isArray(content.categories)) {
output.categories = content.categories;
}

return output;
}

override notify() {
super.notify();
this.#catalogUpdateListeners.forEach(listener => listener());
this.notify();
}

onCatalogUpdate(listener: catalogUpdateHandle): Disposable {
Expand Down Expand Up @@ -117,39 +176,85 @@ export class CatalogManager extends Publisher<ApplicationCatalog> implements Dis
return recipe;
}

async addLocalModelsToCatalog(models: LocalModelImportInfo[]): Promise<void> {
// we copy the current catalog in another object and update it with the model
// then write it to the custom catalog path. If it exists it will be overwritten by default
const tmpCatalog: ApplicationCatalog = Object.assign({}, this.catalog);

for (const model of models) {
const statFile = await promises.stat(model.path);
tmpCatalog.models.push({
id: model.path,
name: model.name,
description: `Model imported from ${model.path}`,
hw: 'CPU',
file: {
path: path.dirname(model.path),
file: path.basename(model.path),
size: statFile.size,
creation: statFile.mtime,
},
memory: statFile.size,
});
/**
* This method is used to imports user's local models.
* @param localModels the models to imports
*/
async importUserModels(localModels: LocalModelImportInfo[]): Promise<void> {
const userCatalogPath = this.getUserCatalogPath();
let content: ApplicationCatalog;

// check if we already have an existing user's catalog
if (fs.existsSync(userCatalogPath)) {
const raw = await promises.readFile(userCatalogPath, 'utf-8');
content = this.sanitize(JSON.parse(raw));
} else {
content = {
recipes: [],
models: [],
categories: [],
};
}

const customCatalog = path.resolve(this.appUserDirectory, 'catalog.json');
return promises.writeFile(customCatalog, JSON.stringify(tmpCatalog, undefined, 2), 'utf-8');
// Transform local models into ModelInfo
const models: ModelInfo[] = await Promise.all(
localModels.map(async local => {
const statFile = await promises.stat(local.path);
return {
id: local.path,
name: local.name,
description: `Model imported from ${local.path}`,
hw: 'CPU',
file: {
path: path.dirname(local.path),
file: path.basename(local.path),
size: statFile.size,
creation: statFile.mtime,
},
memory: statFile.size,
};
}),
);

// Add all our models infos to the user's models catalog
content.models.push(...models);

// overwrite the existing catalog
return promises.writeFile(userCatalogPath, JSON.stringify(content, undefined, 2), 'utf-8');
}

async removeLocalModelFromCatalog(modelId: string): Promise<void> {
// we copy the current catalog in another object and remove from it the model with modelId
// then write it to the custom catalog path.
const tmpCatalog: ApplicationCatalog = Object.assign({}, this.catalog);
tmpCatalog.models = tmpCatalog.models.filter(m => m.url !== '' && m.id !== modelId);
/**
* Remove a model from the user's catalog.
* @param modelId
*/
async removeUserModel(modelId: string): Promise<void> {
const userCatalogPath = this.getUserCatalogPath();
if (!fs.existsSync(userCatalogPath)) {
throw new Error('User catalog does not exist.');
}

const raw = await promises.readFile(userCatalogPath, 'utf-8');
const content = this.sanitize(JSON.parse(raw));

return promises.writeFile(
userCatalogPath,
JSON.stringify(
{
recipes: content.recipes,
models: content.models.filter(model => model.id !== modelId),
categories: content.categories,
},
undefined,
2,
),
'utf-8',
);
}

const customCatalog = path.resolve(this.appUserDirectory, 'catalog.json');
return promises.writeFile(customCatalog, JSON.stringify(tmpCatalog, undefined, 2), 'utf-8');
/**
* Return the path to the user catalog
*/
private getUserCatalogPath(): string {
return path.resolve(this.appUserDirectory, USER_CATALOG);
}
}
6 changes: 3 additions & 3 deletions packages/backend/src/managers/modelsManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@ describe('deleting models', () => {
test('delete local model should call catalogManager', async () => {
vi.mocked(env).isWindows = false;
const postMessageMock = vi.fn();
const removeLocalModelFromCatalogMock = vi.fn();
const removeUserModelMock = vi.fn();
const manager = new ModelsManager(
'appdir',
{
Expand All @@ -522,7 +522,7 @@ describe('deleting models', () => {
},
] as ModelInfo[];
},
removeLocalModelFromCatalog: removeLocalModelFromCatalogMock,
removeUserModel: removeUserModelMock,
} as unknown as CatalogManager,
telemetryLogger,
taskRegistry,
Expand All @@ -531,7 +531,7 @@ describe('deleting models', () => {
await manager.loadLocalModels();
await manager.deleteModel('model-id-1');

expect(removeLocalModelFromCatalogMock).toBeCalledWith('model-id-1');
expect(removeUserModelMock).toBeCalledWith('model-id-1');
});

test('deleting on windows should check if models is uploaded', async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/managers/modelsManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export class ModelsManager implements Disposable {
if (!model.url) {
modelPath = path.join(model.file.path, model.file.file);
// remove it from the catalog as it cannot be downloaded anymore
await this.catalogManager.removeLocalModelFromCatalog(modelId);
await this.catalogManager.removeUserModel(modelId);
} else {
modelPath = this.getLocalModelFolder(modelId);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/studio-api-impl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ test('openDialog should call podmanDesktopAPi showOpenDialog', async () => {

test('importModels should call catalogManager', async () => {
const addLocalModelsMock = vi
.spyOn(catalogManager, 'addLocalModelsToCatalog')
.spyOn(catalogManager, 'importUserModels')
.mockImplementation((_models: LocalModelImportInfo[]) => Promise.resolve());
const models: LocalModelImportInfo[] = [
{
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/studio-api-impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ export class StudioApiImpl implements StudioAPI {
}

async importModels(models: LocalModelImportInfo[]): Promise<void> {
return this.catalogManager.addLocalModelsToCatalog(models);
return this.catalogManager.importUserModels(models);
}

async checkInvalidModels(models: string[]): Promise<string[]> {
Expand Down