fix: Missing data dirs should be handled gra 8000 cefully #8115
Conversation
| parser.add_argument("--output-directory", type=str, default=None, help="Set the ComfyUI output directory. Overrides --base-directory.") | ||
| parser.add_argument("--temp-directory", type=str, default=None, help="Set the ComfyUI temp directory (default is in the ComfyUI directory). Overrides --base-directory.") | ||
| parser.add_argument("--input-directory", type=str, default=None, help="Set the ComfyUI input directory. Overrides --base-directory.") | ||
| parser.add_argument("--user-directory", type=is_valid_directory, default=None, help="Set the ComfyUI user directory with an absolute path. Overrides --base-directory.") |
There was a problem hiding this comment.
For consistency, could set this to str and it'll be created if it's missing, otherwise the other related options could also use type=is_valid_directory to enforce a valid dir is provided (but that may break some expectations?)
| parser.add_argument("--user-directory", type=is_valid_directory, default=None, help="Set the ComfyUI user directory with an absolute path. Overrides --base-directory.") | |
| parser.add_argument("--user-directory", type=str, default=None, help="Set the ComfyUI user directory with an absolute path. Overrides --base-directory.") |
| if args.temp_directory: | ||
| temp_dir = os.path.join(os.path.abspath(args.temp_directory), "temp") | ||
| logging.info(f"Setting temp directory to: {temp_dir}") | ||
| folder_paths.set_temp_directory(temp_dir) |
There was a problem hiding this comment.
Breaking change, but would be consistent with expectations of related args:
| if args.temp_directory: | |
| temp_dir = os.path.join(os.path.abspath(args.temp_directory), "temp") | |
| logging.info(f"Setting temp directory to: {temp_dir}") | |
| folder_paths.set_temp_directory(temp_dir) | |
| if args.temp_directory: | |
| temp_dir = os.path.abspath(args.temp_directory) | |
| logging.info(f"Setting temp directory to: {temp_dir}") | |
| folder_paths.set_temp_directory(temp_dir) |
| data_dirs = [ | ||
| folder_paths.get_input_directory(), | ||
| folder_paths.get_temp_directory() | ||
| ] |
There was a problem hiding this comment.
Optionally create the user/ directory here (UserManager already does so at startup?), and restore the output/ directory creation (presumably not necessary and is instead handled on-demand).
| data_dirs = [ | |
| folder_paths.get_input_directory(), | |
| folder_paths.get_temp_directory() | |
| ] | |
| data_dirs = [ | |
| folder_paths.get_input_directory(), | |
| folder_paths.get_output_directory(), | |
| folder_paths.get_temp_directory(), | |
| folder_paths.get_user_directory() | |
| ] |
| if not os.path.exists(custom_node_path): | ||
| logging.warning(f"Directory {custom_node_path} for custom nodes does not exist, skipping") | ||
| continue |
There was a problem hiding this comment.
This is the primary fix I'm interested in, if the directory does not need to exist but is checked by default, it seems better to skip instead of crash?
I am not sure if that aligns with your expectations, so if it makes sense to log a warning I've added one here, otherwise the warning could be removed?
| logging.warning(f"Directory {custom_node_path} for custom nodes does not exist, skipping") | ||
| continue | ||
|
|
||
| possible_modules = os.listdir(os.path.realpath(custom_node_path)) |
There was a problem hiding this comment.
os.path.realpath was added here but could be reverted.
A user reported the lack of resolving symlinks as problematic for similar custom_node_path logic at nodes.py and resolved it by adding os.path.realpath: #2035
I added it here for consistency.
Fixes: #8110
Overview:
input/+temp/directories moved to a common function call run afterapply_custom_paths().input/dir was created before it could be updated by the CLI arg. Now it will not create another directory before the input dir path is changed.temp/dir seems like it might have been implemented a bit wrong? Rather than set a specific directory like the others, it creates atemp/folder at that path (so--temp-directory /tmpwould create/tmp/temp)... fixing that would technically be a breaking change though.custom_nodes/is another default directory that caused ComfyUI to crash if it did not exist when it's contents was checked. Instead prefer to skip empty custom node directories at startup.References:
--temp-directoryintroduced--user-directoryintroduced--output-directoryintroduced (this moved theinput/dir creation fromnodes.pytofolder_paths.py, and removedoutput/dir creation, might have been a mistake?)input/dir handling, similar to what this PR does.Slight inconsistency observed with the
user/directory.--user-directoryis a newer option added to the CLI (8 months ago) after the related ones (2 years ago). For some reason it was not co-located with the existing options (affects list order in help output) and unlike the other related options it enforces a check on the given path to exist, rather than create when missing.ComfyUI/comfy/cli_args.py
Line 194 in f3ff5c4
I could likewise shift that user dir logic into the common setup function and co-locate the CLI arg? (normalizing on your preference of enforcing the path given to exist or not)