BennyKok / comfyui-deploy

An open source `vercel` like deployment platform for Comfy UI
https://comfydeploy.ing
GNU Affero General Public License v3.0
769 stars 84 forks source link

Support ComfyUIDeployExternalNumber and ComfyUIDeployExternalNumberInt #25

Open bnlucas opened 5 months ago

bnlucas commented 5 months ago

When using number nodes, ComfyUIDeployExternalNumber and ComfyUIDeployExternalNumberInt, the values are not replaced when running a workflow.

createRun.ts:106 only accounts for text nodes.

    // Replace the inputs
    if (inputs && workflow_api) {
      for (const key in inputs) {
        Object.entries(workflow_api).forEach(([_, node]) => {
          if (node.inputs["input_id"] === key) {
            node.inputs["input_id"] = inputs[key];
            // Fix for external text default value
            if (node.class_type == "ComfyUIDeployExternalText") {
              node.inputs["default_value"] = inputs[key];
            }
        });
      }
    }

There's a few issues here.

  1. While inputs[key] has the type string | number, it will always be a string here. If you just pass node.inputs["default_value"] = inputs[key]; then ComfyUI will fail when running the workflow.
  2. If you provide a number value in one of these inputs in a run, then clear it in the next the value is set to "" instead of the original undefined, so if you fix for the first issue you then have to validate empty strings.

This is a quick and dirty fix I implemented to get around this. I also found the node.inputs["input_id"] = inputs[key]; line does absolutely nothing.

    // Replace the inputs
    if (inputs && workflow_api) {
      for (const key in inputs) {
        Object.entries(workflow_api).forEach(([_, node]) => {
          if (node.inputs["input_id"] === key) {
            if (inputs[key] === "") return;

            let value: string | number = inputs[key].toString();

            if (node.class_type === "ComfyUIDeployExternalNumber") {
              value = parseFloat(value);
            } else if (node.class_type === "ComfyUIDeployExternalNumberInt") {
              value = parseInt(value);
            }

            node.inputs["default_value"] = value;
          }
        });
      }
    }
BennyKok commented 5 months ago

Thanks for the issue, curious what you mean by node.inputs["input_id"] = inputs[key]; doesnt work in which context?

For context we implemented recently the default_value for the text input node, so havent really checked for other input type at the time being.

In your suggested code, it will be replacing all node's default value right if the input_id match?

bnlucas commented 5 months ago

node.inputs["input_id"] is just an identifier for the web app to display inputs. Say I add a ComfyUIDeployExternalNumber with input_id: cfg, default_value: 3.0 the way the code is structured currently, assuming number nodes worked, you are just updating the workflow_api JSON to be input_id: {provided value on run}, default_value: 3.0

When the graph runs, the default_value is the output to the connected node, not the input id.

The node.inputs["input_id"] === key conditional already identifies these input nodes, now you just need to replace default_value with whatever the user provides for it during a run, that updates the workflow_api JSON and things execute correctly. Using my cfg example, the current state is that it will always execute using 3.0 regardless of what I've provided.

BennyKok commented 5 months ago

Oh, originally the system depended on input_id for the actual value in the node and not fully migrate default_value since it will introduce breaking changes to all the existing user with any older version of comfy deploy, unless they updated comfy deploy version.

Meaning it will actually be

before replace input_id: "my_image_input" after input_id: "https://xxxx.png"

then in the custom node python code download the image from input_id

It's a little bit confusing, but hope this will clarify it. And also looking into optimizing the input system down the line.

We could do a fix for the ComfyUIDeployExternalNumber and ComfyUIDeployExternalNumberInt first

But ideally, we could introduce a new input nodes type with all these new behaviour and probably input validation

ruucm commented 4 months ago

yes this is not working properly.