WASasquatch / was-node-suite-comfyui

An extensive node suite for ComfyUI with over 210 new nodes
MIT License
1.15k stars 170 forks source link

Issues when using WAS in a Docker image #47

Closed r7l closed 1 year ago

r7l commented 1 year ago

There are still some issues with WAS when using it in Docker even if it's possible now to install it. But the issue is not only with Docker but also with any new install of WAS.

You set up a config file but the code will never make use of it on the first run. That's because using it is packed behind an if / else which will only run the config file setup on the first run but won't load the file after it.

Due to the nature of Docker it will always be the first run when starting the container. For this reason, i'll never end up in the part of code behind else. This leads to this situation:

WAS Node Suite Running At: /stable-diffusion/custom_nodes/was_node_suite_comfyui/WAS_Node_Suite.py
WAS Node Suite Running From: /stable-diffusion/custom_nodes/was_node_suite_comfyui
WAS Node Suite: Created default conf file at `/stable-diffusion/custom_nodes/was_node_suite_comfyui/was_suite_config.json`.
Traceback (most recent call last):
  File "/stable-diffusion/nodes.py", line 1169, in load_custom_node
    module_spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/stable-diffusion/custom_nodes/was_node_suite_comfyui/__init__.py", line 1, in <module>
    from .WAS_Node_Suite import NODE_CLASS_MAPPINGS
  File "/stable-diffusion/custom_nodes/was_node_suite_comfyui/WAS_Node_Suite.py", line 193, in <module>
    if was_config.__contains__('use_legacy_ascii_text'):
NameError: name 'was_config' is not defined

Cannot import /stable-diffusion/custom_nodes/was_node_suite_comfyui module for custom nodes: name 'was_config' is not defined

The other question is: What does this config do? Should the file be stored permanently instead of being recreated on every start of ComfyUI? That would be the case unless i'll place the entire directory outside of Docker. I don't really intend to do that since it should stay inside to be easily replaced by any later update. The less files and directories you have to care about when building an image, the better.

In case the file is meant to be permanent, would it be possible to move the file to a different location? Even a config subdirectory would help already as it would allow to move this directory alone outside of the Docker image and mount it back in later on.

And while not really related to me i would suggest to add some drop out functionality in case of failure during config creation. Currently you only drop a message but your code is not doing anything about it. This will always just lead to errors.

WASasquatch commented 1 year ago

The system is able to be installed 3 ways

GitHub clone GitHub zip download Or just downloading the WAS_Node_Suite.py file alone.

There error would be you don't have file permissions if it can't be created, which is nothing that can be done if it can't be created as that would be user error with their install. And would effect more than just config. Models, repos like BLIP, etc will have issues.

r7l commented 1 year ago

Sorry for mixing multiple things together. But the config file was created just fine. I can see it with its content.

In line 137 the code is checking if the config file exists and if not, it creates the file. Then in line 143 it has the else situation for an existing config. If it goes into the if on line 137 you'll never create a variable called was_config. This only happens in else but it's still expected to exist in line 193.

The script only creates was_config variable if the file exists already and since you're not having some exit situation it will go through to 193 in any case.

WASasquatch commented 1 year ago

Yeah, I see the issue there. It's been patched. Additionally it'll use the template config if it can't load the config for some reason, but most may not encounter that as the common issue is them not having write access tot he custom nodes, etc. So I made the script exit if it can't get write accesss.

r7l commented 1 year ago

Thanks allot!

Is the config static otherwise? I see an update function but the location it has right now will keep it static within a Docker setup. Will that lead to issues later on with the file being reset to its defaults?

WASasquatch commented 1 year ago

It's basically just read unless it's missing elements from the template. If it's missing elements it'll add those defaults to the config so they exist and be customized.

There is probably a lot of logic that needs improvement. Unfortunately, the most time I get to work on this project is when the kids go to sleep, and I'm really tired. So I'm just full of typos and brain farts. Lol