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

Use absolute path for normalized filepath #15035

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

catboxanon
Copy link
Collaborator

@catboxanon catboxanon commented Feb 26, 2024

Description

Small oversight I made as part of #14934. resolve was changing paths to use the symlink, causing there to be multiple representations of a path (it should use a single representation, that representation being the path as it appears to the user).

Fixes these issues (upscalers being listed twice due to multiple path representations, and symlinks/junctions breaking due to using the linked path and thus falling "outside" the Gradio directory):
lllyasviel/stable-diffusion-webui-forge#313
lllyasviel/stable-diffusion-webui-forge#282
#14942 (comment)

Checklist:

@catboxanon catboxanon changed the title Fix normalized filepath, resolve -> absolute Use absolute path for normalized filepath Feb 26, 2024
@light-and-ray
Copy link
Contributor

Maybe someone wants to change this symlink before reload ui. It's more logical to leave the original paths without symlink resolving

Why does this bug happen? Why does forge know about the original SD directory?

@light-and-ray
Copy link
Contributor

light-and-ray commented Feb 27, 2024

Hmm, dat upscales doesn't have this problem

Upd it doesn't have problem because it doesn't have cmd flag. Also should be added

I will make a new PR

@light-and-ray
Copy link
Contributor

light-and-ray commented Feb 27, 2024

Ooops, I misunderstood and did the same thing 🤣

@light-and-ray
Copy link
Contributor

Sorry for this misunderstanding. I left only add --dat-models-path cmd flag. #15039

@clayne
Copy link
Contributor

clayne commented Feb 27, 2024

Thanks for this, I'm definitely being bitten by the multiple upscaler entries issue (and also use a symlink for models -> <some-other-location>) in 1.8.0rc.

Related notes: https://bugs.python.org/issue39090

@AUTOMATIC1111 AUTOMATIC1111 merged commit 150b603 into dev Mar 2, 2024
6 checks passed
@AUTOMATIC1111 AUTOMATIC1111 deleted the fix/normalized-filepath-absolute branch March 2, 2024 03:36
AUTOMATIC1111 added a commit that referenced this pull request Mar 2, 2024
…absolute

Use `absolute` path for normalized filepath
@clayne
Copy link
Contributor

clayne commented Mar 24, 2024

@light-and-ray So this is afflicting me again after I rearranged some upscaler directories to be more compatible with a ComfyUI install using the same shared model directory.

Here's my layout:

Base model path is a symlink to a shared path which is shared by various UIs:

clayne@sv590:/f/stablediffusion/stable-diffusion-webui (dev=) $ ls -lad models
lrwxrwxrwx 1 clayne None 16 Mar 24 12:07 models -> ../shared/models

Within that shared directory, the A1111 specific upscaler directories are symlinked into a shared upscalers directory:

clayne@sv590:/f/stablediffusion/shared/models $ ls -la | grep upscalers
lrwxrwxrwx  1 clayne None  9 Mar 18 17:15 ESRGAN -> upscalers
lrwxrwxrwx  1 clayne None  9 Mar 18 17:15 GFPGAN -> upscalers
lrwxrwxrwx  1 clayne None 14 Mar 20 14:32 LDSR -> upscalers/ldsr
lrwxrwxrwx  1 clayne None  9 Mar 18 17:15 RealESRGAN -> upscalers
lrwxrwxrwx  1 clayne None  9 Mar 18 17:15 ScuNET -> upscalers
lrwxrwxrwx  1 clayne None  9 Mar 18 17:15 SwinIR -> upscalers
drwxr-xr-x+ 1 clayne None  0 Mar 23 22:50 upscalers

In A1111, this results in the following behavior:

image

vs Comfy:

image

Previously, this "worked" even with my stable-diffusion-webui/models -> ../shared/models symlink, but the second I converted the various upscaler directories to symlinks is when this bug resurfaced.

Edit: so it looks like absolute() should be fine here as it's simply resolving the directory without completely following the symlinks to their destination (as opposed to something like readlink -f):

>>> Path('models/RealESRGAN').absolute()
PosixPath('/f/stablediffusion/stable-diffusion-webui-1-8-0-rc/models/RealESRGAN')

vs

>>> Path('models/RealESRGAN').resolve()
PosixPath('/f/stablediffusion/shared/models/upscalers')

Not sure why I'm seeing dupe entries again though.

@catboxanon
Copy link
Collaborator Author

catboxanon commented Mar 25, 2024

Because you're symlinking an entire folder of upscalers and not separating them by type, they're going to appear several times since they'll be picked up for each architecture. That's why your list now contains 3 entries for each file. ComfyUI only provides a single upscale_models folder which is why it doesn't have this problem.

@clayne
Copy link
Contributor

clayne commented Mar 25, 2024

Because you're symlinking an entire folder of upscalers and not separating them by type, they're going to appear several times since they'll be picked up for each architecture. That's why your list now contains 3 entries for each file. ComfyUI only provides a single upscale_models folder which is why it doesn't have this problem.

I understand the likely mechanism of what's happening but this still seems like something trivial to fix on the code side by use of a simple hash table to prevent dupes. For now I guess I'll just symlink into upscaler specific directories and hope ComfyUI handles recursively finding them.

ruchej pushed a commit to ruchej/stable-diffusion-webui that referenced this pull request Sep 30, 2024
…zed-filepath-absolute

Use `absolute` path for normalized filepath
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