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

Update extra_config.py #6441

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update extra_config.py #6441

wants to merge 2 commits into from

Conversation

dchatel
Copy link

@dchatel dchatel commented Jan 12, 2025

added path normalization to full_path.

needed to ensure that line 204 in folder_paths.py: if full_folder_path in paths: works properly on all OS.

added path normalization to full_path.

needed to ensure that line 204 in folder_paths.py: `if full_folder_path in paths:` works properly on all OS.
change folders_path.models_dir to the base_path specified in extra_models_path.yaml if set.

This ensure that models get correctly uploaded to the specified path across all custom_nodes.
@bigcat88
Copy link
Contributor

needed to ensure that line 204 in folder_paths.py: if full_folder_path in paths: works properly on all OS.

In my opinion, a good start would be to modify existing tests or add a new test that explains why these changes are necessary.

@dchatel
Copy link
Author

dchatel commented Jan 13, 2025

An example of why this is at least desirable is that without this, if a user sets a custom base_path and is_default to true, then the model downloader in comfyui-manager does not work as intended.

You can try it yourself: set the base_path somewhere external to comfyui, set is_default to true, then try to download a model using comfyui-manager, or let a custom node download its own model (like comfyui-florence2). You'll see that the models get download inside the comfyui/model folder instead of the one you've set in base_path.

@bigcat88
Copy link
Contributor

Then this is not quite the correct implementation for this.

There can be many be several records with is_default for different folder paths, it is impossible to determine which of them should ultimately overwrite the root models_dir.

Currently software that performs download should look under the specific keys in the global folder_names_and_paths variable and get path to store models from it.

@comfyanonymous would you like to extend the functionality of the yaml file and allow it to set the models_dir variable?

@iwr-redmond
Copy link

In #5864, @vvuk suggested that base paths be processed in the order they were defined in extra_models_paths.yaml, with the first to be defined taking precedence.

@vvuk
Copy link

vvuk commented Jan 17, 2025

(I'd appreciate some feedback/guidance in #5864 -- I'm not quite sure what the dev process is here, let me know if I should be doing something else for PR submissions!)

@iwr-redmond
Copy link

There appears to be a backlog of YAML file PRs:

#2720 by @soof-golan
#3744 by @iwanders
#4365 by @ltdrdata

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.

4 participants