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

feat: optional restoring of ComfyUI snapshots to bake custom nodes into the Docker Image #30

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

kklemon
Copy link

@kklemon kklemon commented Apr 1, 2024

Motivation

Many users and workflows rely on custom nodes and extensions which correspondingly should also be available in a RunPod worker.

ComfyUI Manager, perhaps the most popular ComfyUI management exension, provides a convenient feature to export a snapshot of the ComfyUI instance with all installed extensions.

This PR implements optional restoring of a snapshot if provided as snapshot.json in the root directory. If such a file is found, ComfyUI Manager will be installed and restoring of the snapshot will be triggered by starting ComfyUI with the --cpu --quick-test-for-ci flags within the Docker image build process.

Issues closed

#6 #22 #18 #9 (partially)

Todo

  • Implement tests after this initial proposal is approved.

@triplecookedchips
Copy link

@kklemon Hi - is it possible to provide a little more guidance here? I added a snapshot.json in the root directory, but nothing changed when I built the image in Docker. Thank you

@kklemon
Copy link
Author

kklemon commented May 21, 2024

Adding a snapshot.json in the root directory should be sufficient. Also, make sure that you are building the image from scratch. Apart from these suggestions, I would need to take a look at the build logs to provide more help.

Edit: Since the Dockerfile uses glob-based file copying, you will need a relatively new Docker installation. I couldn't figure out the exact version, but it should work from Docker version 20 upwards.

@triplecookedchips
Copy link

Thanks @kklemon - I didn't build the image from scratch, so will try again with your advice

@TimPietrusky
Copy link
Member

@kklemon thanks for adding this, it sounds really nice. We will have to test this, as this is a feature we haven't used yet. But it sounds like this would solve a lot of problems regarding custom nodes and backing them into the actual Docker image.

Copy link
Member

@TimPietrusky TimPietrusky left a comment

Choose a reason for hiding this comment

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

@kklemon I have two questions, please take a look!

Dockerfile Outdated
@@ -39,15 +41,16 @@ RUN if [ -z "$SKIP_DEFAULT_MODELS" ]; then wget -O models/vae/sdxl_vae.safetenso
RUN if [ -z "$SKIP_DEFAULT_MODELS" ]; then wget -O models/vae/sdxl-vae-fp16-fix.safetensors https://huggingface.co/madebyollin/sdxl-vae-fp16-fix/resolve/main/sdxl_vae.safetensors; fi
RUN if [ -z "$SKIP_DEFAULT_MODELS" ]; then wget -O models/loras/xl_more_art-full_v1.safetensors https://civitai.com/api/download/models/152309; fi

# Support for the network volume
ADD src/extra_model_paths.yaml ./
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed? As this is also usable for adding models into ComfyUI via the network volume.

RUN chmod +x /start.sh /restore_snapshot.sh

# Optionally copy snapshot file
ADD snapshot.jso[n] /
Copy link
Member

Choose a reason for hiding this comment

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

Why would we have jso[n] here when we always expect a json-file?

@TimPietrusky TimPietrusky changed the title Implement optional restoring of ComfyUI snapshots feat: optional restoring of ComfyUI snapshots to bake custom nodes into the Docker Image May 29, 2024
@TimPietrusky TimPietrusky deleted the branch blib-la:main June 4, 2024 09:21
@TimPietrusky TimPietrusky reopened this Jun 4, 2024
@TimPietrusky TimPietrusky changed the base branch from dev to main June 4, 2024 09:23
@TimPietrusky
Copy link
Member

@kklemon I updated the base branch as we want to move away from having the dev branch.

@TimPietrusky
Copy link
Member

@kklemon ping 🙏

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