Refactor model directory structure and add new configuration files#14768
Refactor model directory structure and add new configuration files#14768Samith-NM wants to merge 2 commits into
Conversation
- Deleted obsolete model directories: controlnet, detection, diffusers, diffusion_models, embeddings, frame_interpolation, geometry_estimation, gligen, hypernetworks, latent_upscale_models, loras, model_patches, optical_flow, photomaker, style_models, text_encoders, unet, upscale_models, vae, vae_approx. - Created new model directories for audio_encoders, background_removal, checkpoints, clip, clip_vision, and updated configs for various models including anything_v3, v1-inference, v2-inference, and inpainting. - Added detailed configuration files for model training and inference, including parameters for learning rates, model architectures, and data handling.
Samith-NM
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
📝 WalkthroughWalkthroughThis change adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as folder_paths.py
participant FS as Filesystem
participant RedirectFile as JSON Redirect File
participant User
Startup->>FS: check if raw_path exists
alt path exists
FS-->>Startup: exists
else path missing
Startup->>RedirectFile: read redirect mapping
alt redirect found
RedirectFile-->>Startup: resolved path
else no redirect
Startup->>User: prompt for absolute path
User-->>Startup: provided path
Startup->>RedirectFile: write redirect mapping
end
end
Startup-->>Startup: return base_path / models_dir / output_directory
Related issues: None linked in the provided context. Suggested labels: enhancement, needs discussion Suggested reviewers: None identified from available context. Note: the change of Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@folder_paths.py`:
- Around line 21-48: `valid_path` currently prompts with `input()` during module
import, which can block or fail in non-interactive environments. Refactor
`valid_path` and the import-time assignments for `base_path`/`models_dir` to
avoid any stdin prompt at import; instead, return a fallback or raise a clear
non-interactive error, and move path resolution/creation behind an explicit
setup step or guard.
- Line 48: The models directory path in folder_paths.py was changed to the
singular form, which breaks the expected folder layout and downstream tests.
Update the models_dir assignment to use the existing plural directory name in
the path-building logic inside folder_paths.py, keeping it consistent with the
rest of the model subfolder handling and the expectations in folder path tests.
- Around line 25-42: Fix the redirect persistence logic in folder_paths.py: the
redirect loading/saving path handling is using the wrong variables and can fail
silently. In the redirect branch, use the redirect_path variable when opening
the JSON file instead of the string literal, and in the persistence block
replace the dict-vs-path mixups so os.path.exists and open operate on the
redirect file path, not the redirects dictionary. Keep the behavior centered
around the existing redirect_path, redirects, and new_path flow, and ensure the
redirect mapping is actually read and written successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3d614fa-82ef-432d-ba23-dea8759e93d6
📒 Files selected for processing (37)
folder_paths.pymodel/audio_encoders/put_audio_encoder_models_heremodel/background_removal/put_background_removal_models_heremodel/checkpoints/put_checkpoints_heremodel/clip/put_clip_or_text_encoder_models_heremodel/clip_vision/put_clip_vision_models_heremodel/configs/anything_v3.yamlmodel/configs/v1-inference.yamlmodel/configs/v1-inference_clip_skip_2.yamlmodel/configs/v1-inference_clip_skip_2_fp16.yamlmodel/configs/v1-inference_fp16.yamlmodel/configs/v1-inpainting-inference.yamlmodel/configs/v2-inference-v.yamlmodel/configs/v2-inference-v_fp32.yamlmodel/configs/v2-inference.yamlmodel/configs/v2-inference_fp32.yamlmodel/configs/v2-inpainting-inference.yamlmodel/controlnet/put_controlnets_and_t2i_heremodel/detection/put_detection_models_heremodel/diffusers/put_diffusers_models_heremodel/diffusion_models/put_diffusion_model_files_heremodel/embeddings/put_embeddings_or_textual_inversion_concepts_heremodel/frame_interpolation/put_frame_interpolation_models_heremodel/geometry_estimation/put_geometry_estimation_models_heremodel/gligen/put_gligen_models_heremodel/hypernetworks/put_hypernetworks_heremodel/latent_upscale_models/put_latent_upscale_models_heremodel/loras/put_loras_heremodel/model_patches/put_model_patches_heremodel/optical_flow/put_optical_flow_models_heremodel/photomaker/put_photomaker_models_heremodel/style_models/put_t2i_style_model_heremodel/text_encoders/put_text_encoder_files_heremodel/unet/put_unet_files_heremodel/upscale_models/put_esrgan_and_other_upscale_models_heremodel/vae/put_vae_heremodel/vae_approx/put_taesd_encoder_pth_and_taesd_decoder_pth_here
| def valid_path(saved_path,description = "Instance Folder"): | ||
| if os.path.exists(saved_path): | ||
| return saved_path | ||
|
|
||
| redirect_path = os.path.join(os.path.dirname(__file__),"path.redirects.json") | ||
| if os.path.exists(redirect_path): | ||
| with open("redirect_path","r") as f: | ||
| redirects = json.load(f) | ||
| if description in redirects and os.path.exists(redirects[description]): | ||
| return redirects[description] | ||
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | ||
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | ||
| try: | ||
| redirects = {} | ||
| if os.path.exists(redirects): | ||
| with open(redirects, "r") as f: redirects = json.load(f) | ||
| redirects[description] = new_path | ||
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return new_path | ||
|
|
||
| models_dir = os.path.join(base_path, "models") | ||
|
|
||
|
|
||
| base_path = valid_path(raw_path,description = "ComfyUI Core Directory") | ||
|
|
||
| models_dir =valid_path( os.path.join(base_path,"model"),description = "Model Directory") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
valid_path calls input() at module import — this will hang headless/server/CI startup.
valid_path runs at import time for base_path, models_dir, and output_directory. On a fresh install these directories usually don't exist yet, so the code drops into input(...) and blocks on stdin. In a non-interactive context (systemd service, Docker, CI, the unit tests above) there's no TTY, so this either hangs forever or raises EOFError, preventing folder_paths from importing at all. Directory creation/validation shouldn't gate module import behind an interactive prompt.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 35-35: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 37-37: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` around lines 21 - 48, `valid_path` currently prompts with
`input()` during module import, which can block or fail in non-interactive
environments. Refactor `valid_path` and the import-time assignments for
`base_path`/`models_dir` to avoid any stdin prompt at import; instead, return a
fallback or raise a clear non-interactive error, and move path
resolution/creation behind an explicit setup step or guard.
| redirect_path = os.path.join(os.path.dirname(__file__),"path.redirects.json") | ||
| if os.path.exists(redirect_path): | ||
| with open("redirect_path","r") as f: | ||
| redirects = json.load(f) | ||
| if description in redirects and os.path.exists(redirects[description]): | ||
| return redirects[description] | ||
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | ||
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | ||
| try: | ||
| redirects = {} | ||
| if os.path.exists(redirects): | ||
| with open(redirects, "r") as f: redirects = json.load(f) | ||
| redirects[description] = new_path | ||
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | ||
| except Exception: | ||
| pass | ||
|
|
||
| return new_path |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The redirect-file plumbing is riddled with wrong variables — it can crash and never persists.
Three concrete bugs in the redirect logic:
- Line 27 opens the string literal
"redirect_path"instead of theredirect_pathvariable, so whenever the real JSON file exists this raisesFileNotFoundError(and it's not inside thetry). - Lines 35, 36, 38 pass the
redirectsdict ({}) toos.path.exists/openwhere the path is expected.os.path.exists({})raisesTypeError, which the bareexcept Exception: passsilently swallows, so the mapping is never written.
Net effect: the redirect file is never read correctly and never saved.
🐛 Proposed fix
redirect_path = os.path.join(os.path.dirname(__file__),"path.redirects.json")
if os.path.exists(redirect_path):
- with open("redirect_path","r") as f:
+ with open(redirect_path,"r") as f:
redirects = json.load(f)
if description in redirects and os.path.exists(redirects[description]):
return redirects[description]
print(f"\n[!] ALERT: {description} not found at: {saved_path}")
new_path= input(f"Please paste the updated absolute path for '{description}': ").strip()
try:
redirects = {}
- if os.path.exists(redirects):
- with open(redirects, "r") as f: redirects = json.load(f)
+ if os.path.exists(redirect_path):
+ with open(redirect_path, "r") as f: redirects = json.load(f)
redirects[description] = new_path
- with open(redirects, "w") as f: json.dump(redirects, f, indent=4)
+ with open(redirect_path, "w") as f: json.dump(redirects, f, indent=4)
except Exception:
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redirect_path = os.path.join(os.path.dirname(__file__),"path.redirects.json") | |
| if os.path.exists(redirect_path): | |
| with open("redirect_path","r") as f: | |
| redirects = json.load(f) | |
| if description in redirects and os.path.exists(redirects[description]): | |
| return redirects[description] | |
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | |
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | |
| try: | |
| redirects = {} | |
| if os.path.exists(redirects): | |
| with open(redirects, "r") as f: redirects = json.load(f) | |
| redirects[description] = new_path | |
| with open(redirects, "w") as f: json.dump(redirects, f, indent=4) | |
| except Exception: | |
| pass | |
| return new_path | |
| redirect_path = os.path.join(os.path.dirname(__file__),"path.redirects.json") | |
| if os.path.exists(redirect_path): | |
| with open(redirect_path,"r") as f: | |
| redirects = json.load(f) | |
| if description in redirects and os.path.exists(redirects[description]): | |
| return redirects[description] | |
| print(f"\n[!] ALERT: {description} not found at: {saved_path}") | |
| new_path= input(f"Please paste the updated absolute path for '{description}': ").strip() | |
| try: | |
| redirects = {} | |
| if os.path.exists(redirect_path): | |
| with open(redirect_path, "r") as f: redirects = json.load(f) | |
| redirects[description] = new_path | |
| with open(redirect_path, "w") as f: json.dump(redirects, f, indent=4) | |
| except Exception: | |
| pass | |
| return new_path |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 35-35: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 37-37: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(redirects, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` around lines 25 - 42, Fix the redirect persistence logic in
folder_paths.py: the redirect loading/saving path handling is using the wrong
variables and can fail silently. In the redirect branch, use the redirect_path
variable when opening the JSON file instead of the string literal, and in the
persistence block replace the dict-vs-path mixups so os.path.exists and open
operate on the redirect file path, not the redirects dictionary. Keep the
behavior centered around the existing redirect_path, redirects, and new_path
flow, and ensure the redirect mapping is actually read and written successfully.
|
|
||
| base_path = valid_path(raw_path,description = "ComfyUI Core Directory") | ||
|
|
||
| models_dir =valid_path( os.path.join(base_path,"model"),description = "Model Directory") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
models_dir now points at base_path/model — this breaks the established layout.
The directory has always been models (plural), and every model subfolder plus the downstream test in tests-unit/comfy_test/folder_path_test.py expects os.path.join(base_path, "models"). Dropping the s will make ComfyUI look for models in a nonexistent directory (and, combined with valid_path, trigger the interactive prompt on startup).
🐛 Proposed fix
-models_dir =valid_path( os.path.join(base_path,"model"),description = "Model Directory")
+models_dir = valid_path(os.path.join(base_path, "models"), description="Model Directory")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| models_dir =valid_path( os.path.join(base_path,"model"),description = "Model Directory") | |
| models_dir = valid_path(os.path.join(base_path, "models"), description="Model Directory") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@folder_paths.py` at line 48, The models directory path in folder_paths.py was
changed to the singular form, which breaks the expected folder layout and
downstream tests. Update the models_dir assignment to use the existing plural
directory name in the path-building logic inside folder_paths.py, keeping it
consistent with the rest of the model subfolder handling and the expectations in
folder path tests.
1 participant
Closes How come we can't relocate instances? #14760