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 ComfyUI #451

Closed
wants to merge 7 commits into from
Closed

Update ComfyUI #451

wants to merge 7 commits into from

Conversation

PassiveLemon
Copy link
Contributor

Just updated ComfyUI to Pytorch 2, Xformers 0.0.20, and the latest Comfy commit. I updated the Docker-compose accordingly.
This also fixes an issue where the mount for the output is not loaded so the container will not save images to a host mounted directory.

I found zero change with and without Triton so I just removed it.

The volume mount for output needs to be present or else ComfyUI will not save images to a host mounted directory.
Fixes error about not having permission to execute entrypoint.sh
@AbdBarho
Copy link
Owner

AbdBarho commented May 7, 2023

This also fixes an issue where the mount for the output is not loaded so the container will not save images to a host mounted directory.

hmmm, weird, I did not have this problem.

I found zero change with and without Triton so I just removed it.

triton now is now automatically installed with pytorch 2.

@PassiveLemon
Copy link
Contributor Author

PassiveLemon commented May 7, 2023

This also fixes an issue where the mount for the output is not loaded so the container will not save images to a host mounted directory.

hmmm, weird, I did not have this problem.

If you use a "save image" node, it automatically saves to the output folder? It does not for me. It will only work if I specifically add that docker mount.

@PassiveLemon
Copy link
Contributor Author

PassiveLemon commented May 7, 2023

This is mostly temporary but I pushed it so you can get the idea. I am now currently working on making custom nodes much easier to install.
If there is any directory in the custom_nodes path, it will get searched for a requirements.txt and then be installed. Sometimes they require distro specific packages like cmake so those might also need to get installed in this stage but I don't currently have a way to automatically fetch those which is the main issue here I also don't like doing this in the entrypoint since it will try to install the packages every single rebuild.
Currently, I just put Debian's build-essential package there because it contains a lot of the things nodes might need but extra stuff can always just be added to that line. Still thinking of different solutions so maybe you might have an idea that you like. A separate file for only distro packages could be added for easy user interaction but I'd like to avoid mostly empty stray files. They could also be loaded and installed in the Dockerfile using a similar method but again, its another stray file.
The method at this point is functional, though not ideal in my opinion.

Wont error from a file that doesn't exist
@AbdBarho
Copy link
Owner

AbdBarho commented May 8, 2023

Yeah extensions are weird, same situation is for auto. I had to specifically create a startup.sh script so users can put all of their stuff there, usually installing dependencies.

That being said, pip is smart enough to not re-install everything, only missing stuff.

Do you think a startup.sh script is a good solution?

@@ -44,4 +44,16 @@ for to_path in "${!MOUNTS[@]}"; do
echo Mounted $(basename "${from_path}")
done

if [ "$(ls -A /stable-diffusion/custom_nodes)" ]; then
chmod 777 -R "/stable-diffusion/custom_nodes/"
apt-get install build-essential -y
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could move this into the dockerfile, where we install aria2 and others

@@ -44,4 +44,16 @@ for to_path in "${!MOUNTS[@]}"; do
echo Mounted $(basename "${from_path}")
done

if [ "$(ls -A /stable-diffusion/custom_nodes)" ]; then
chmod 777 -R "/stable-diffusion/custom_nodes/"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing reasons. I wasn't going to bother fixing everything until there was a proposed solution

Comment on lines 50 to 56
for dir in "/stable-diffusion/custom_nodes/*"; do
if [ -e "$dir/requirements.txt" ]; then
echo $dir
cd $dir
pip install -r requirements.txt
fi
done
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could simplify this loop

shopt -s nullglob
list=(${ROOT}/custom_nodes/*/requirements.txt)
for req in "${list[@]}"; do
  pip install -r "$req"
done

Comment on lines +79 to +80
volumes:
- *v2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already mounted in the x-base-service, so there is no need to change the mounts here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the issue though. I know it's present in the base, but the save image node won't save the images to the hosts output directory unless it is present in the actual profile.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the output folder of the save image node? and does the output/comfy exist on your machine?

@PassiveLemon
Copy link
Contributor Author

PassiveLemon commented May 8, 2023

Yeah extensions are weird, same situation is for auto. I had to specifically create a startup.sh script so users can put all of their stuff there, usually installing dependencies.

That being said, pip is smart enough to not re-install everything, only missing stuff.

Do you think a startup.sh script is a good solution?

Maybe. It would definitely work, but it would really only be used for the Debian packages since the node pip dependencies would be included in their requirements.txt files. I'll quickly whip up a design and see how you like it

Comment on lines +79 to +80
volumes:
- *v2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the output folder of the save image node? and does the output/comfy exist on your machine?

Comment on lines +53 to +58
shopt -s nullglob
list=(${ROOT}/custom_nodes/*/requirements.txt)
for req in "${list[@]}"; do
pip install -r "$req"
done

Copy link
Owner

@AbdBarho AbdBarho May 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do still think this should be separate, otherwise it could introduce so many bugs we cannot account for.
i.e. we build the container with certain dependencies, yet at startup they get overwritten by random ones from the extensions.

An example which is very frustrating that comes to mind is opencv-headless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The save image node doesn't have a path, just a name I set it to. It should save the image to the output/ folder in the comfyui directory. /output/comfy does exist on my system.

Would it be possible to create a whole separate pip environment for the extensions alone? I'm not familiar with it or python so I don't know if it's possible and how it would be done anyways.

@AbdBarho
Copy link
Owner

AbdBarho commented Jul 22, 2023

hey I am going to take the liberty and change some stuff do we can get this merged.

The auto install of custom node dependencies will be removed, but can still be added in a new PR.

AbdBarho added a commit that referenced this pull request Jul 22, 2023
Closes #451

---------

Co-authored-by: PassiveLemon <lemonl3mn@protonmail.com>
cloudaxes pushed a commit to cloudaxes/stable-diffusion-webui-docker that referenced this pull request Sep 6, 2023
Closes AbdBarho#451

---------

Co-authored-by: PassiveLemon <lemonl3mn@protonmail.com>
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.

None yet

2 participants