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

Load user catalog if file exists or use default one #85

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jan 17, 2024

A default catalog is provided with the extension. The user can provide its own catalog (in ~/podman-desktop/ai-studio/catalog.json). If the user's file is valid, it will be used. Otherwise, the default catalog will be used.

For now, changes on the user's file are not detected.

Fixes partially #32

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

looks like there are quotes/single quote issues

Comment on lines 30 to 31
const catalogPath = path.resolve(__dirname, '..', 'catalog.json');
const catalogStr = fs.readFileSync(catalogPath, 'utf-8');
Copy link
Collaborator

@benoitf benoitf Jan 17, 2024

Choose a reason for hiding this comment

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

I'm wondering here if you need to use a read of the file as it seems anyway you embed it into the extension

so could it be like

import catalog from '../catalog.json'

or with newer TypeScript spec

import catalog from './catalog.json' with { type: 'json' };

so you don't need to add it in Containerfile, do not need to handle synchronous method/io like readFileSync instead of promises readFile, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, but I'll want in the next iteration to run a watch on this file, to detect changes, and I prefer to go one step in that direction

Copy link
Collaborator

Choose a reason for hiding this comment

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

note that if it's imported, vite will automatically reload the file as well and update the extension's code

Copy link
Collaborator

Choose a reason for hiding this comment

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

(so no need to setup a custom watcher for development mode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what about the production code? I would like the extension to detect changes even when the app is running in production mode

Copy link
Collaborator

Choose a reason for hiding this comment

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

in the current code the file is embedded and read from the extension so it doesn't change the behaviour.

if you need to read from a custom folder, you'll still need to include a default file in case it's not yet provided by the user

and in production, anyway you should not write to the source directory and monitor the source directory. You'll need to copy/read the file from a different folder (extension storage folder) b/c here if you update the extension it'll override the file anyway.

so as you need to have an 'included' version anyway it's easier to import/include it as a resource of the extension rather than a separate file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, you are right, I didn't think about having an 'included' version.
I'll proceed differently then.
Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

but it's not a requested change on my side, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the scenario to use the imported json file as default catalog if the user does not provide any catalog.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

probably will need some formatting/linting changes

@feloy feloy marked this pull request as draft January 18, 2024 08:28
@feloy feloy force-pushed the feat/deploy-catalog-json branch from 7d7d411 to f674fae Compare January 18, 2024 09:29
@feloy feloy force-pushed the feat/deploy-catalog-json branch from f674fae to 6d66dcb Compare January 18, 2024 09:33
@benoitf benoitf self-requested a review January 18, 2024 09:40
@feloy feloy force-pushed the feat/deploy-catalog-json branch from 941123f to 03e01b8 Compare January 18, 2024 10:48
@feloy feloy marked this pull request as ready for review January 18, 2024 10:48
Comment on lines 42 to 59
await fs.promises
.readFile(catalogPath, 'utf-8')
.then((data: string) => {
try {
const cat = JSON.parse(data) as Catalog;
// TODO(feloy): check version and format
console.log('using user catalog');
this.setNewCatalog(cat);
} catch (err: unknown) {
console.error('unable to parse catalog file, reverting to default catalog', err);
this.setNewCatalog(defaultCatalog);
}
})
.catch((err: unknown) => {
console.error('got err', err);
console.error('unable to read catalog file, reverting to default catalog', err);
this.setNewCatalog(defaultCatalog);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure about chaining promises with the await

try {
  const data = await fs.promises.readFile(catalogPath, 'utf-8')
  const cat = JSON.parse(data) as Catalog;
  this.setCatalog(cat);
} catch (
  console.error(...)
  this.setCatalog(defaultCatalog);
}

I would check if the file exists before trying to read it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's much simpler, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, readFile correctly and efficiently checks the existence of the file, we don't have to check it before (or we would have to handle another error case)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's mainly b/c usually the absence of the file is not an error so you want to read it if you know user provided a custom file

and getting an exception b/c the file is not there or getting an exception b/c we're unable to read the file is not the same while here we handle it

it looks like we'll get a log trace

unable to read catalog file, reverting to default catalog

even if we don't have customized the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree about the error log trace. Let's add the check and be silent if the file doesn't exist

}
}

setNewCatalog(newCatalog: Catalog) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setNewCatalog(newCatalog: Catalog) {
setCatalog(newCatalog: Catalog) {

Comment on lines 87 to 89
async getCategories(): Promise<Category[]> {
return content.categories;
return this.catalog.categories;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why it's a Promise ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(same remark for almost all methods)

as catalog is a model, I'm not sure about the usage of async/promises for reading from it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they are async to respect the interface (class) defined in shared/src/StudioAPI.ts, where they are returning a Promise, and from the client side, they are really returning a Promise, due to the wrapper sending a message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok got it.

I thought that client was getting the model object asynchronously but then it was just fields

client:

const catalog = await rpc.get(CatalogAPI.class).getCatalog();
const categories = catalog.categories;
const something = catalog.something;
const else = catalog.else;

and not a call each time we need something like

const categories = await rpc....getCategories();
const something = await rpc....getSomething();
const else = await rpc....getElse();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I expect to do with the next PR is to have the backend (this class) post a message to the frontend with the new content of the catalog every time it changes (in newCatalog()). On the frontend, I wonder what would be the best approach: having a single catalog store, or 3 (or more) stores: recipes, models, categories.

Or, even, having 3 different messages (newRecipes, newModels, newCategories)? But inihs case we would have to check which ones have been modified, to push only the modified ones.

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with all options (as the traffic/latency is less a problem on a local client)

I think what would matter here, is that webview is recreated each time we click on the webview icon (and webview can read/store a state)

in that case, instead of asking again the catalog, it could start from the cache/state to display faster some items

so it would be interesting to have frontend being able to refresh/restore its UI as fast as possible (so avoiding lot of computes on backend if nothing has changed)

@benoitf benoitf self-requested a review January 18, 2024 12:49
@feloy feloy requested a review from lstocchi January 18, 2024 13:08
@feloy feloy changed the title Install catalog file with extension and read it from there Load user catalog if file exists or use default one Jan 18, 2024
@feloy feloy merged commit a0fda99 into main Jan 18, 2024
3 checks passed
@feloy feloy deleted the feat/deploy-catalog-json branch February 16, 2024 12:49
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants