Model downloader dialog

[mcmonkey4eva]

Status

Functional MVP - lot of fundamental work and improvements still-to-do, but all the basic functionality is there.

Could be merged as-is and have followup PRs for further work, or can treat as draft and fix some of the UI-level limitations listed below before merging, let me know what you prefer @huchenlei

About

Adds a "Model Downloader" dialog when you import a workflow json that has models listed, to autodownload models from trusted sources.

Here's a test workflow json with 4 models URLs: valid sd 1.5 safetensors, and 3 unique error cases:
test-models.json

Overview of how it works

• Tracks the lists of models from server using API added @ add a get models list api route comfyanonymous/ComfyUI#4519
• keeps the list in a temporary local cache (cleared on refresh button hit) (cache is just to prevent slowdown from API spamcalls)
• checks any models in the GraphData of newly imported workflows (atm only added manually, no automatic storage of these values)
• when models are missing, opens the new MissingModelsWarning (based on LoadWorkflowWarning)
• has interface that lists all missing models, with download buttons
• Downloads the model using API added @ Add model downloading endpoint. comfyanonymous/ComfyUI#4248
  • handles the new download_progress ws event from that PR, and the new status data type

UI-level limitations

• Does not yet handle the case of both models and nodes are missing properly
  • handling this well might need coordinating with Comfy Manager and/or merging Manager in? Since I imagine manager overrides this whole dialog and all anyway.
• The dialog is kinda ugly. I just copypasted the missing nodes dialog for now. It's perfectly sufficient, just not beautiful yknow.
• has error handling, but only for expected issues - no particular handling of what if the comfy server dies or anything else particularly unusual
• beforeConfigureGraph should probably have missingModels added in - I'm hesitant to do this offhand without looking into how that code works, as idk if that'll break anything to just shove a new var there
• theoretically multiple users/tabs could trigger overlapping model download requests, and idk what the upstream server will do about that, nor what the UI will necessarily do. Needs testing at least

API-level limitations

• There's no easy way to cancel a download if you change your mind. Upstream API adjustment needed to do that.
• There's no easy way in the API to know what model is being downloaded. I am currently using a hackaround for this by parsing the message string. Upstream API improvement would be preferred.
• There's no way to check for "same model, different path" (ie hash comparison) yet. Needs significant upstream upgrades.

[huchenlei]

We can probably merge this now without fixing those API level limitations such as interrupt the download, as currently we only plan to add model info for example workflows.

BTW currently loading the default workflow does give me a warning for the default SD1.5 checkpoint download which can be annoying. We need to find a good way to let people bypass that.

Also it would be nice to add a setting for users to disable this warning dialog. We got a similar request for disabling missing nodes dialog in #446

[huchenlei]

Based on currently dialog implementation, if both model and nodes are missing, the missing model dialog content will replace the missing node dialog content.

I think we should only fire a single show warning dialog call here and make the dialog content show both missing node warnings and missing model warnings.

[mcmonkey4eva]

In the main PR description I wrote:
Does not yet handle the case of both models and nodes are missing properly
handling this well might need coordinating with Comfy Manager and/or merging Manager in? Since I imagine manager overrides this whole dialog and all anyway.
I'm hesitant to merge the dialogs right away due to the increased potential conflict, unless you know confidently that the existing dialog can be mucked up without it being an issue

[huchenlei]

Maybe the hint URL can be a tooltip instead?

[mcmonkey4eva]

applied as a basic title, not sure if there's a better tooltip component to use

[huchenlei]

I feel like this model download action button can be its own component.

[mcmonkey4eva]

I'm not entirely clear on what you mean by that

[mcmonkey4eva]

I didn't realize the default workflow had a models value slipped into it already. That explains the tests failing.

... We should really swap that at the very least to a safetensors file btw. It is very silly that it still has a ckpt there

[huchenlei]

I didn't realize the default workflow had a models value slipped into it already. That explains the tests failing.

... We should really swap that at the very least to a safetensors file btw. It is very silly that it still has a ckpt there

Just a reminder. All playwright test screenshot expectations will need update if we update the default checkpoint file name 🤣

[mcmonkey4eva]

Added setting by copypasta of your code for the other setting - I note it doesn't seem to be enabled until i toggled it off and back on? something might be wrong there

also removed the models entry from the default workflow both (A) to fix that issue for now and (B) because a ckpt file listed there won't be valid at any point in the foreseeable future anyway

[huchenlei]

Added setting by copypasta of your code for the other setting - I note it doesn't seem to be enabled until i toggled it off and back on? something might be wrong there

Thanks for pointing that out. I think that is due to how default is handled in the app.ui.settings. I think from now on, accessing the new settings from all use settingStore as it correctly handles default value of settings if the setting entry is not present.