Moving models download settings to user settings. #3216

lerignoux · 2025-03-24T05:05:52Z

I would like to be able to customize the download urls for comfy to whitelist new models servers.

I saw there was a todo in the front end part suggesting to do this change in particular so I decided to do it.

It should be completely transparent for user, just allow to customize the comfy.settings.json to have a custom option in case it needs to be changed.

I didn't find any relevant unit test to add, but please tell me if you think there should be one (both the modal and settings are already tested.)

┆Issue is synchronized with this Notion page by Unito

lerignoux · 2025-03-24T05:06:58Z

src/constants/coreSettings.ts

+    id: 'Comfy.ModelLibrary.AllowedSources',
+    category: ['Comfy', 'Model', 'Download'],
+    name: 'Model Download Allowed Sources',
+    type: 'hidden',


There does not seem to exists an array type so relying on hidden for now. I don't think allowing configuration in Interface brings a lot of value

lerignoux · 2025-03-24T05:07:25Z

src/constants/coreSettings.ts

+    defaultValue: [
+      'https://civitai.com/',
+      'https://huggingface.co/',
+      'http://localhost:'


should this be added only if current domain is localhost ? It would be safer no ?

src/components/dialog/content/MissingModelsWarning.vue

christian-byrne · 2025-03-24T06:57:35Z

src/components/dialog/content/MissingModelsWarning.vue

+const allowedSources = await settingsStore.get('Comfy.ModelLibrary.AllowedSources')
+const allowedSuffixes = await settingsStore.get('Comfy.ModelLibrary.AllowedSuffixes')


I think we can remove the await. Using await in top level of the script makes it so the setup function returns a Promise.

Edit: Sorry, didn't see it was still a draft PR.

No problem, I was still testing and doing changes.
Should be ok now, please tell me if you see any issue or suggestion

lerignoux · 2025-03-24T10:41:30Z

I am not sure what kind of tests could be added. Do you see any or is it ok without additional tests ?

huchenlei

Just for some context:

Originally I changed these hardcoded values to user settings, but they are reverted in https://github.com/Comfy-Org/ComfyUI_frontend/pull/890/files#r1772680306 with this TODO. The original idea for this TODO is to serve the allowed values from the backend server via server configuration rather than a value that the user can control.

@mcmonkey4eva for explaining what needs to be done here to resolve the TODO.

Ref: #890 (comment)

lerignoux · 2025-03-25T02:13:22Z

Just for some context:

Originally I changed these hardcoded values to user settings, but they are reverted in https://github.com/Comfy-Org/ComfyUI_frontend/pull/890/files#r1772680306 with this TODO. The original idea for this TODO is to serve the allowed values from the backend server via server configuration rather than a value that the user can control.

@mcmonkey4eva for explaining what needs to be done here to resolve the TODO.

Ref: #890 (comment)

Thank you for the feedback.

I tried to follow what I understood from the discussion intention.
I could not find a relevant API and config in Comfy so I tried adding a simple one.
Do you think it is acceptable ?
cf ComfyUI Merge Request

…lhost.

mcmonkey4eva · 2025-03-25T09:25:03Z

The download restrictions (last I looked at things some months ago) are defined in ComfyUI (the python code repo), the frontend is merely just prerendering what the python is going to say about it -- so the values should be sourced from the python repo (eg a consistent internal API). Frontend settings make no sense for that data. Frontend hardcoding is a lazy but functional placeholder.

huchenlei · 2025-03-25T15:59:32Z

The download restrictions (last I looked at things some months ago) are defined in ComfyUI (the python code repo), the frontend is merely just prerendering what the python is going to say about it -- so the values should be sourced from the python repo (eg a consistent internal API). Frontend settings make no sense for that data. Frontend hardcoding is a lazy but functional placeholder.

BTW, the model downloading logic has been removed from the comfyui backend, the download for standalone is just using browser's download to download the model file. Download to directory directly feature is only available on desktop version. I think it is fine now to have the validation in the frontend only.

Ref: comfyanonymous/ComfyUI#5432

lerignoux · 2025-03-26T03:10:29Z

The download restrictions (last I looked at things some months ago) are defined in ComfyUI (the python code repo), the frontend is merely just prerendering what the python is going to say about it -- so the values should be sourced from the python repo (eg a consistent internal API). Frontend settings make no sense for that data. Frontend hardcoding is a lazy but functional placeholder.

Thank you for the info.

I could not find a reference to this whitelist in the Comfy repo so I added a configuration and internal for it as you suggested.
Do you think it is ok ?
Cf MR

lerignoux · 2025-03-28T02:03:42Z

validation in the frontend only.

I see.
So that would mean keeping the current code and just removing the comment (In that case I will just suggest to move the list to a constant file).
@mcmonkey4eva It seems the comment to move this to API download was coming from you is it ok to keep that in the front end code ?

mcmonkey4eva · 2025-03-28T09:50:56Z

My comments in the code there are outdated to the modern system per huchenlei's explanation

huchenlei

LGTM on proceeding with orginal implementation of moving allow list to user settings.

lerignoux requested a review from a team as a code owner March 24, 2025 05:05

lerignoux marked this pull request as draft March 24, 2025 05:06

lerignoux commented Mar 24, 2025

View reviewed changes

src/components/dialog/content/MissingModelsWarning.vue Outdated Show resolved Hide resolved

lerignoux force-pushed the models_download_configuration branch 2 times, most recently from ff613ca to bf0df5c Compare March 24, 2025 05:15

christian-byrne requested changes Mar 24, 2025

View reviewed changes

lerignoux force-pushed the models_download_configuration branch from bf0df5c to a0888d9 Compare March 24, 2025 10:31

lerignoux force-pushed the models_download_configuration branch 2 times, most recently from d3d4e36 to edfbcf5 Compare March 24, 2025 13:52

lerignoux marked this pull request as ready for review March 24, 2025 14:03

lerignoux requested a review from christian-byrne March 24, 2025 14:03

huchenlei reviewed Mar 24, 2025

View reviewed changes

lerignoux mentioned this pull request Mar 25, 2025

Adding a server config file and api route to download models download configuration comfyanonymous/ComfyUI#7376

Open

lerignoux force-pushed the models_download_configuration branch 2 times, most recently from ea3e346 to bead8a6 Compare March 25, 2025 02:59

lerignoux added 3 commits March 25, 2025 14:51

Moving models download settings to user settings.

f64b822

Fixing linting errors with new configurable models download settings.

e98e457

Ensuring localhost models source is only added when hostname is loca…

fbd7c16

…lhost.

lerignoux force-pushed the models_download_configuration branch from bead8a6 to ec2731a Compare March 25, 2025 06:51

Using custom internal comfy API to fetch models download settings.

df1055c

lerignoux force-pushed the models_download_configuration branch from ec2731a to df1055c Compare March 25, 2025 07:08

lerignoux requested a review from huchenlei March 25, 2025 07:19

lerignoux marked this pull request as draft March 29, 2025 14:37

huchenlei approved these changes Mar 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving models download settings to user settings. #3216

Moving models download settings to user settings. #3216

lerignoux commented Mar 24, 2025 •

edited

Loading

lerignoux Mar 24, 2025

lerignoux Mar 24, 2025

christian-byrne Mar 24, 2025 •

edited

Loading

lerignoux Mar 24, 2025

lerignoux commented Mar 24, 2025

huchenlei left a comment •

edited

Loading

lerignoux commented Mar 25, 2025

mcmonkey4eva commented Mar 25, 2025

huchenlei commented Mar 25, 2025 •

edited

Loading

lerignoux commented Mar 26, 2025 •

edited

Loading

lerignoux commented Mar 28, 2025

mcmonkey4eva commented Mar 28, 2025

huchenlei left a comment

		const allowedSources = await settingsStore.get('Comfy.ModelLibrary.AllowedSources')
		const allowedSuffixes = await settingsStore.get('Comfy.ModelLibrary.AllowedSuffixes')

Moving models download settings to user settings. #3216

Are you sure you want to change the base?

Moving models download settings to user settings. #3216

Conversation

lerignoux commented Mar 24, 2025 • edited Loading

lerignoux Mar 24, 2025

Choose a reason for hiding this comment

lerignoux Mar 24, 2025

Choose a reason for hiding this comment

christian-byrne Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

lerignoux Mar 24, 2025

Choose a reason for hiding this comment

lerignoux commented Mar 24, 2025

huchenlei left a comment • edited Loading

Choose a reason for hiding this comment

lerignoux commented Mar 25, 2025

mcmonkey4eva commented Mar 25, 2025

huchenlei commented Mar 25, 2025 • edited Loading

lerignoux commented Mar 26, 2025 • edited Loading

lerignoux commented Mar 28, 2025

mcmonkey4eva commented Mar 28, 2025

huchenlei left a comment

Choose a reason for hiding this comment

lerignoux commented Mar 24, 2025 •

edited

Loading

christian-byrne Mar 24, 2025 •

edited

Loading

huchenlei left a comment •

edited

Loading

huchenlei commented Mar 25, 2025 •

edited

Loading

lerignoux commented Mar 26, 2025 •

edited

Loading