Moving models download settings to user settings.

[lerignoux]

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.
See linked API changes

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]

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]

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

[mcmonkey4eva]

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]

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]

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]

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]

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]

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

Hello

Sorry @huchenlei I was a bit lost, it seems adding this on front end and fetching the settings from Comfy API was ok.
Just in case I did a draft of fully front control of this.
Cf https://github.com/Comfy-Org/ComfyUI_frontend/pull/3638/files
Though security wise I think the current implementation is better.
I'll try to check how to trigger the PR on backend side. If you know someone who could help with the review please tell me.

Cheers