-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support for ComfyUI #384
Support for ComfyUI #384
Conversation
Map to existing auto and invoke folder structures
Thank you, I unfortunately don't have time now to test this thoroughly, I will try to have it tested by the end of the week. We would need to think about the folder hierarchy and try to not create new subfolders for every ui, otherwise we cannot re-use anything. I still have to check the license stuff as mentioned in #367 it would be great if the changes were constrained to the comfy UI and only whats necessary in docker-compose. The readme formatting could be reverted, it was like this because of a vscode extension I use. |
Understood, I reverted all unnecessary changes to the code that doesn't affect anything. About the folder hierarchy, aren't the build scripts for each UI completely different anyways? I feel like it would be more of a hassle than its worth to try to make things reusable. |
Unless you mean something like a master Dockerfile that can manage the similarities between the containers like the Pip packages but then also pass control to the individuals that manage the more UI specific configuration. |
Oh, by "re-using" I meant the actual models, since these are usually very large, it is redundant to re-download or copy them to every UI folder. Even though it would be the easiest solution. |
I'm still slightly confused. Is there something that I need to change with this PR or is it suggesting something that could be worked on to provide compatibility between all UIs? This profile will use the same file structure as the others and symlink them to their appropriate locations in the container |
I am kind of referring to this:
but don't worry, this is not your problem, this is a more general architecture question regarding this repo. |
Oh I believe those are functions specific to ComfyUI so they are in their own directory to avoid confusion with the other UIs. As for this repo, let me know if you would like any help. I'm pretty much free for the next week or so. |
services/comfy/Dockerfile
Outdated
|
||
RUN --mount=type=cache,target=/root/.cache/pip pip install torch==1.13.0 torchvision torchaudio --extra-index-url https://download.pytorch.org/whl/cu117 | ||
|
||
RUN apt update && apt install -y git && apt clean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to use/keep apt-get instead of apt. apt is an user-facing UI for apt-get and it's siblings, that isn't mean for scripts. From apt's man page (man apt
):
SCRIPT USAGE AND DIFFERENCES FROM OTHER APT TOOLS
The apt(8) commandline is designed as an end-user tool and it may
change behavior between versions. While it tries not to break backward
compatibility this is not guaranteed either if a change seems
beneficial for interactive use.
All features of apt(8) are available in dedicated APT tools like apt-
get(8) and apt-cache(8) as well. apt(8) just changes the default value
of some options (see apt.conf(5) and specifically the Binary scope). So
you should prefer using these commands (potentially with some
additional options enabled) in your scripts as they keep backward
compatibility as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know there was a difference in them. Since apt-get seems more suited for this, I switch back over to that. Thanks for that
This reverts commit 9cfb7e8.
I have updated this to match the latest commits OCI compliance. I based most of it off of the Sygil service since it is the most similar in terms of structure. |
You can add |
I've tried the Dockerfile and it blends in perfectly into this project. As a suggestion: Quote the The downside with comfy is that there isn't a plugin manager yet and lots of custom nodes would require additional |
I'll let AbdBarho decide if and how this gets added as there are currently plans to organize stuff a little better.
All of the other profiles have the same echo command and it doesn't appear to cause any problems so I'll leave it as is. As for the custom nodes, I couldn't find any mention of git or pip so I am unsure if something will need to be done. I will have to think of a way to get that sorted if they do require external commands though. |
Hey! sorry for not being active, I am currently changing jobs so time is a bit tight, I will try to get to this in the evening today to do some initial testing.
That is really great, maybe we need some testers since I don't have an AMD GPU.
Maybe we can consider a |
How would you want this implemented? I'm thinking profiles like for auto-cpu but I don't want to crowd the compose file with 4 profiles for 1 UI. |
Test 1 variables: - 3060 Ti - 20 steps - 7.5 cfg - euler_a - 4x 512x512 Test 1: Without: 16.43, 15.98, 15.59 - About 49 seconds total for 3 runs. With Xformers: 10.39, 10.22, 10.64 - About 30 seconds total for 3 runs. About 1.6x faster with Xformers -------------------- Test 2 variables: - 3060 Ti - 20 steps - 7.5 cfg - euler_a - 4x 1024x512 Test 1: Without Xformers: 53.25, 54.23, 53.53 - About 2.6 minutes total for 3 runs. With Xformers: 20.06, 20.42, 20.47 - About 1 minute total for 3 runs. About 2.6x faster with Xformers!!
Added support for Xformers (I ripped it from auto). Very good improvements in performance depending on image size. Details in the 'Xformers' commit. |
@PassiveLemon can you please allow to push into your branch? I want to do some final changes and merge this. |
I invited you as a collaborator |
@PassiveLemon thank you for this PR Would be great if you have any other improvements, we can always add them in other PRs! |
I will try to help maintain this profile as Comfy is still very active and updated multiple times a day |
As discussed in Discussion [AbdBarho#367](AbdBarho#367), this adds support for the newer ComfyUI. I forked the fork that would already add this but the maintainer of that fork hasn't implemented the changes needed to properly get the output function working, which I did. I believe everything is functional though I have not tested every single node. I changed the table format for the README and a few other minor things for aesthetic reasons but if you want me to revert those, I will. --------- Co-authored-by: Jonathan Kovacs <[email protected]> Co-authored-by: AbdBarho <[email protected]>
As discussed in Discussion [AbdBarho#367](AbdBarho#367), this adds support for the newer ComfyUI. I forked the fork that would already add this but the maintainer of that fork hasn't implemented the changes needed to properly get the output function working, which I did. I believe everything is functional though I have not tested every single node. I changed the table format for the README and a few other minor things for aesthetic reasons but if you want me to revert those, I will. --------- Co-authored-by: Jonathan Kovacs <[email protected]> Co-authored-by: AbdBarho <[email protected]>
As discussed in Discussion #367, this adds support for the newer ComfyUI. I forked the fork that would already add this but the maintainer of that fork hasn't implemented the changes needed to properly get the output function working, which I did. I believe everything is functional though I have not tested every single node.
I changed the table format for the README and a few other minor things for aesthetic reasons but if you want me to revert those, I will.