initial model store #674

mcmonkey4eva · 2024-08-28T23:32:04Z

Initial model list + metadata store handler, moving the model list out of app.ts into it's own handler. This is a prerequisite to full model management interface. Also adds view_metadata API route + some handling attached to it in modelStore.

PR'd this before anything else as @huchenlei I imagine you'll have some comment on the structure here? So better to get that reviewed before I go too deep into using it.
The other stores/ files all use defineStore which to my eye is some indecipherable Enterprise™ overcomplication of defining a class and a singleton? I don't know what the heck is going on there. For modelStore I gave it a direct impl instead of trying to match that pattern.
(Technically more like a service pattern but it's a datastore.)

ps, the metadata code looked so nice before npm run format ruined it :(

huchenlei · 2024-08-29T00:09:24Z

src/stores/modelStore.ts

+
+/** Defines and holds metadata for a model */
+export class ComfyModelDef {
+  /** Proper filename of the model */


We should define the schema in zod for easier parsing and validation.

this isn't a schema or api type, it's just temporary local data storage. model metadata doesn't come in a reliable format, at least not currently.

unless you think we should build a whole "proper metadata" api on the ComfyUI serverside and then just read it for frontend? would make sense to do too tbh

Transformed zod code:

import { z } from 'zod' const zModelMetadata = z.object({ 'modelspec.title': z.string().optional(), title: z.string().optional(), display_name: z.string().optional(), name: z.string().optional(), 'modelspec.architecture': z.string().optional(), architecture: z.string().optional(), 'modelspec.author': z.string().optional(), author: z.string().optional(), 'modelspec.description': z.string().optional(), description: z.string().optional(), 'modelspec.resolution': z.string().optional(), resolution: z.string().optional(), 'modelspec.usage_hint': z.string().optional(), usage_hint: z.string().optional(), 'modelspec.trigger_phrase': z.string().optional(), trigger_phrase: z.string().optional(), 'modelspec.tags': z.string().optional(), tags: z.string().optional(), 'modelspec.thumbnail': z.string().optional(), thumbnail: z.string().optional(), image: z.string().optional(), icon: z.string().optional() }).passthrough() export const zComfyModelDef = z.object({ name: z.string(), directory: z.string(), title: z.string(), architecture_id: z.string(), author: z.string(), resolution: z.string(), description: z.string(), usage_hint: z.string(), trigger_phrase: z.string(), tags: z.array(z.string()), image: z.string(), has_loaded_metadata: z.boolean() }) export type ComfyModelDef = z.infer<typeof zComfyModelDef> export function parseComfyModelDef(name: string, directory: string, metadata: z.infer<typeof zModelMetadata>): ComfyModelDef { const parsedMetadata = zModelMetadata.parse(metadata) const title = parsedMetadata['modelspec.title'] || parsedMetadata.title || parsedMetadata.display_name || parsedMetadata.name || name const tagsString = parsedMetadata['modelspec.tags'] || parsedMetadata.tags || '' const tags = tagsString.split(',').map(tag => tag.trim()) const image = parsedMetadata['modelspec.thumbnail'] || parsedMetadata.thumbnail || parsedMetadata.image || parsedMetadata.icon || '' return zComfyModelDef.parse({ name, directory, title, architecture_id: parsedMetadata['modelspec.architecture'] || parsedMetadata.architecture || '', author: parsedMetadata['modelspec.author'] || parsedMetadata.author || '', resolution: parsedMetadata['modelspec.resolution'] || parsedMetadata.resolution || '', description: parsedMetadata['modelspec.description'] || parsedMetadata.description || '', usage_hint: parsedMetadata['modelspec.usage_hint'] || parsedMetadata.usage_hint || '', trigger_phrase: parsedMetadata['modelspec.trigger_phrase'] || parsedMetadata.trigger_phrase || '', tags, image, has_loaded_metadata: true }) }

But why tho

this isn't a schema or api type, it's just temporary local data storage. model metadata doesn't come in a reliable format, at least not currently.

splitting object type would introduce significant complexity to practical usage, and not make it any easier to tell. It's not defined by "intuition" it's easily checkable with the boolean. More practically in most cases the difference does entirely matter. The times it matters are cases where you actively want the metadata so you'll check the bool and/or await o.load() to ensure that.

Separate interface is to avoid this boolean flag check everywhere when you need to use the value. Imagine every time you access any field in the structure you have to check a boolean flag and optionally call the load function. The ensure o.load() will also make tracking when/where the data is fetched very hard as there will be a ton of callsites to load.

BTW, I'm also fine if you make this explicitly lazy load, i.e. fetch data on first attempt to access fields.

this is an explicit lazy load design, yes.

Ok, we can continue with this, and refactor later if necessary. Can you add some tests as requested by the previous comment?

src/stores/modelStore.ts

the inner ModelStore (per-folder) can't be pinia because its made of temporary instances, but it can be reactive

mcmonkey4eva · 2024-08-29T20:57:38Z

the main store is now Pinia, the other values are all now reactive (as far as I can tell, pinia is limited to global scoped stores, so the sub-stores can't be pinia directly, but they can easily be made reactive)

huchenlei · 2024-08-29T21:48:53Z

the main store is now Pinia, the other values are all now reactive (as far as I can tell, pinia is limited to global scoped stores, so the sub-stores can't be pinia directly, but they can easily be made reactive)

For sub store that has smaller scope (A subtree of the component tree), I would suggest using Vue's provide/inject mechanism for dependency injection. https://vuejs.org/guide/components/provide-inject

mcmonkey4eva · 2024-08-29T22:37:59Z

This... leaves me very confused about how that would even work, and also looks like significant added complexity.

Note there's not vue components here, this is just simple data.

huchenlei · 2024-08-29T23:26:19Z

This... leaves me very confused about how that would even work, and also looks like significant added complexity.

Note there's not vue components here, this is just simple data.

I thought you are talking about context that are specific to a subtree, which are the opposite to global store. An example of subtree context is if we have multiple workflows hosted in multiple containers, each component used in the container should be able to know which workflow the container is holding, but this local store is not very necessary if the nested level if not very deep. A global store with all the workflows will also work in this example.

mcmonkey4eva · 2024-08-30T00:32:05Z

Workflows? huh?

This is root -> model folders -> models in the folder

it has to be in this structure due to the way upstream comfy handles models

huchenlei

I think we should also have some tests on the schame parsing and pinia store.

You can reference existing tests

Note

Code in review comments are generated by claude 3.5

huchenlei · 2024-09-01T22:41:24Z

src/stores/modelStore.ts

+
+/** Defines and holds metadata for a model */
+export class ComfyModelDef {
+  /** Proper filename of the model */


Transformed zod code:

import { z } from 'zod' const zModelMetadata = z.object({ 'modelspec.title': z.string().optional(), title: z.string().optional(), display_name: z.string().optional(), name: z.string().optional(), 'modelspec.architecture': z.string().optional(), architecture: z.string().optional(), 'modelspec.author': z.string().optional(), author: z.string().optional(), 'modelspec.description': z.string().optional(), description: z.string().optional(), 'modelspec.resolution': z.string().optional(), resolution: z.string().optional(), 'modelspec.usage_hint': z.string().optional(), usage_hint: z.string().optional(), 'modelspec.trigger_phrase': z.string().optional(), trigger_phrase: z.string().optional(), 'modelspec.tags': z.string().optional(), tags: z.string().optional(), 'modelspec.thumbnail': z.string().optional(), thumbnail: z.string().optional(), image: z.string().optional(), icon: z.string().optional() }).passthrough() export const zComfyModelDef = z.object({ name: z.string(), directory: z.string(), title: z.string(), architecture_id: z.string(), author: z.string(), resolution: z.string(), description: z.string(), usage_hint: z.string(), trigger_phrase: z.string(), tags: z.array(z.string()), image: z.string(), has_loaded_metadata: z.boolean() }) export type ComfyModelDef = z.infer<typeof zComfyModelDef> export function parseComfyModelDef(name: string, directory: string, metadata: z.infer<typeof zModelMetadata>): ComfyModelDef { const parsedMetadata = zModelMetadata.parse(metadata) const title = parsedMetadata['modelspec.title'] || parsedMetadata.title || parsedMetadata.display_name || parsedMetadata.name || name const tagsString = parsedMetadata['modelspec.tags'] || parsedMetadata.tags || '' const tags = tagsString.split(',').map(tag => tag.trim()) const image = parsedMetadata['modelspec.thumbnail'] || parsedMetadata.thumbnail || parsedMetadata.image || parsedMetadata.icon || '' return zComfyModelDef.parse({ name, directory, title, architecture_id: parsedMetadata['modelspec.architecture'] || parsedMetadata.architecture || '', author: parsedMetadata['modelspec.author'] || parsedMetadata.author || '', resolution: parsedMetadata['modelspec.resolution'] || parsedMetadata.resolution || '', description: parsedMetadata['modelspec.description'] || parsedMetadata.description || '', usage_hint: parsedMetadata['modelspec.usage_hint'] || parsedMetadata.usage_hint || '', trigger_phrase: parsedMetadata['modelspec.trigger_phrase'] || parsedMetadata.trigger_phrase || '', tags, image, has_loaded_metadata: true }) }

huchenlei · 2024-09-01T22:45:12Z

src/stores/modelStore.ts

+export class ModelStore {
+  models: Record<string, ComfyModelDef> = reactive({})
+
+  constructor(directory: string, models: string[]) {


Instead of using a class for ModelStore, it would be better to incorporate this logic directly into the Pinia store. This would make the store more self-contained and easier to manage.

The reactive wrapper is not necessary when defining state in a Pinia store, as Pinia automatically makes the state reactive.

Here's how you could refactor this to better align with Pinia best practices:

import { defineStore } from 'pinia' import { ComfyModelDef } from './ComfyModelDef' // Assume this is moved to a separate file export const useModelStore = defineStore('modelStore', { state: () => ({ modelStoreMap: {} as Record<string, Record<string, ComfyModelDef>> }), actions: { async getModelsInFolderCached(folder: string) { if (folder in this.modelStoreMap) { return this.modelStoreMap[folder] } // TODO: needs a lock to avoid overlapping calls const models = await api.getModels(folder) if (!models) { return null } this.modelStoreMap[folder] = {} for (const model of models) { this.modelStoreMap[folder][model] = new ComfyModelDef(model, folder) } return this.modelStoreMap[folder] }, async loadModelMetadata(folder: string, modelName: string) { if (this.modelStoreMap[folder]?.[modelName]) { await this.modelStoreMap[folder][modelName].load() } }, clearCache() { this.modelStoreMap = {} } } })

Pinia best practices increasingly seem to be directly contradictory to general programming best practices. "Don't use a subclass because pinia can't do that, just shove it all into one" seems more like an argument against pinia than anything else

I would still recommend you follow the direct storage type Record<string, Record<string, ComfyModelDef>>. The store is essentially storing a collection of the ModelDef, and the aux ModelStore is just provide a way to access ModelDef by name. You can define getter (computed state) later to add other ways to access data Record<modelName, Record<modelDir, ComfyModelDef>>. Following your approach, we would need a ModelStoreByDir to manage that ownership.

The first principle is that Pinia store should own all objects it manages and the method to access them.

huchenlei · 2024-09-02T23:06:21Z

src/stores/modelStore.ts

+
+/** Defines and holds metadata for a model */
+export class ComfyModelDef {
+  /** Proper filename of the model */


If you don't want to define the interface with zod, you can define it in plain TS interface. The current way to initialize the object is not very flexible. Let's say if later we accept the parsed data from backend directly, you would need to adjust the constructor signature.

So my suggestion here is an interface definition ComfyModel, and a static factory function to create ComfyModel from the data source. This also elimiates the middle state where the class is constructed but before the load function is called. (Is there any reason for the middle state to exist?)

huchenlei · 2024-09-02T23:07:40Z

src/stores/modelStore.ts

+  /** Metadata: resolution of the model, eg '1024x1024' */
+  resolution = ref('')
+  /** Metadata: description of the model */
+  description = ref('')


Drop all these ref, as Vue's ref is deep, i.e. as long as we have Pinia manage a collection of ComfyModel, the nested object trees all become reactive.

huchenlei · 2024-09-03T21:03:20Z

src/stores/modelStore.ts

+
+/** Defines and holds metadata for a model */
+export class ComfyModelDef {
+  /** Proper filename of the model */


Ok, we can continue with this, and refactor later if necessary. Can you add some tests as requested by the previous comment?

huchenlei · 2024-09-03T21:04:35Z

src/stores/modelStore.ts

+
+/** Model store for a folder */
+export class ModelStore {
+  models: Record<string, ComfyModelDef> = reactive({})


reactive can be dropped here, as anything stored in Pinia is marked as reactive deeply (The whole recursive object tree are marked as reactive automatically)

huchenlei · 2024-09-03T21:04:48Z

src/stores/modelStore.ts

+      if (!models) {
+        return null
+      }
+      const store = reactive(new ModelStore(folder, models))


Same here. reactive can be dropped, as anything stored in Pinia is marked as reactive deeply (The whole recursive object tree are marked as reactive automatically)

mcmonkey4eva · 2024-09-03T23:06:26Z

reactive/ref removed (not confident that's actually correct to do tbh but if it's an issue can readd later). Added simple unit tests for the core expectations of how it should work, included some potential ways it might go wrong to validate against

huchenlei

LGTM with minor nits.

huchenlei · 2024-09-04T14:25:29Z

tests-ui/tests/store/modelStore.test.ts

+    ;(api.viewMetadata as jest.Mock).mockImplementation((_, model) => {
+      if (model === 'noinfo.safetensors') {
+        return Promise.resolve({})
+      }
+      return Promise.resolve({
+        'modelspec.title': `Title of ${model}`,
+        display_name: 'Should not show',
+        'modelspec.architecture': 'stable-diffusion-xl-base-v1',
+        'modelspec.author': `Author of ${model}`,
+        'modelspec.description': `Description of ${model}`,
+        'modelspec.resolution': '1024x1024',
+        trigger_phrase: `Trigger phrase of ${model}`,
+        usage_hint: `Usage hint of ${model}`,
+        tags: `tags,for,${model}`
+      })
+    })


This mock should live outside test body as other tests also depend on this mock.

I tried putting it in the initial jest.mock call at the top, it was ignored. I tried putting it in the describe block outside the it block, it was ignored. I tried putting it in the beforeEach, it was ignored. I don't know how to make it apply from outside of an it block. They do however run linearly so being in the first block applies it to all of them.

I think you can wrap it as a function and call it in test case that depends on it. Doing it in a test case and let other subsequent test case depends on it should be avoided as this makes us unable to disable individual test.

fair enough, done

initial model store

b46e314

huchenlei requested changes Aug 29, 2024

View reviewed changes

mcmonkey4eva added 3 commits August 29, 2024 13:23

refactor the 'modelstoreserviceimpl' to pinia

6609de5

pepper in some reactive

26eeb59

the inner ModelStore (per-folder) can't be pinia because its made of temporary instances, but it can be reactive

use refs in metadata class

1a6ca77

huchenlei requested changes Sep 1, 2024

View reviewed changes

huchenlei requested changes Sep 2, 2024

View reviewed changes

huchenlei reviewed Sep 3, 2024

View reviewed changes

mcmonkey4eva added 4 commits September 3, 2024 15:09

remove 'reactive'

a951df3

remove ref too

69cf159

add simple unit tests for modelStore

178be30

make things worse via autoformatting

2c04180

huchenlei approved these changes Sep 4, 2024

View reviewed changes

move mock impls to a function

6b91080

huchenlei merged commit 060e61f into main Sep 4, 2024
6 checks passed

huchenlei deleted the model-store branch September 4, 2024 23:59

huchenlei mentioned this pull request Sep 5, 2024

Update frontend to v1.2.47 comfyanonymous/ComfyUI#4798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial model store #674

initial model store #674

mcmonkey4eva commented Aug 28, 2024

huchenlei Aug 29, 2024

mcmonkey4eva Aug 29, 2024 •

edited

Loading

mcmonkey4eva Aug 29, 2024

huchenlei Sep 1, 2024

mcmonkey4eva Sep 2, 2024

mcmonkey4eva Sep 3, 2024

huchenlei Sep 3, 2024 •

edited

Loading

huchenlei Sep 3, 2024

mcmonkey4eva Sep 3, 2024

huchenlei Sep 3, 2024

mcmonkey4eva commented Aug 29, 2024

huchenlei commented Aug 29, 2024

mcmonkey4eva commented Aug 29, 2024

huchenlei commented Aug 29, 2024 •

edited

Loading

mcmonkey4eva commented Aug 30, 2024

huchenlei left a comment

huchenlei Sep 1, 2024

huchenlei Sep 1, 2024

mcmonkey4eva Sep 2, 2024

huchenlei Sep 2, 2024

huchenlei Sep 2, 2024

huchenlei Sep 2, 2024

huchenlei Sep 3, 2024

huchenlei Sep 3, 2024

huchenlei Sep 3, 2024

mcmonkey4eva commented Sep 3, 2024

huchenlei left a comment

huchenlei Sep 4, 2024

mcmonkey4eva Sep 4, 2024

huchenlei Sep 4, 2024

mcmonkey4eva Sep 4, 2024

initial model store #674

initial model store #674

Conversation

mcmonkey4eva commented Aug 28, 2024

Choose a reason for hiding this comment

mcmonkey4eva Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huchenlei Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmonkey4eva commented Aug 29, 2024

huchenlei commented Aug 29, 2024

mcmonkey4eva commented Aug 29, 2024

huchenlei commented Aug 29, 2024 • edited Loading

mcmonkey4eva commented Aug 30, 2024

huchenlei left a comment

Choose a reason for hiding this comment

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmonkey4eva commented Sep 3, 2024

huchenlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcmonkey4eva Aug 29, 2024 •

edited

Loading

huchenlei Sep 3, 2024 •

edited

Loading

huchenlei commented Aug 29, 2024 •

edited

Loading