receyuki / stable-diffusion-prompt-reader

A simple standalone viewer for reading prompts from Stable Diffusion generated image outside the webui.
MIT License
1.07k stars 69 forks source link

[FEATURE REQUEST] - Support for Efficiency Loader #45

Closed Unstable-Reality closed 1 year ago

Unstable-Reality commented 1 year ago

Summary

It would be great to see the Efficiency Loader node, part of the Efficiency Nodes suite, supported in the next version of the app.

Basic Example

Hi. You can see my scenario below:

Screenshot 2023-09-11 at 19 27 19 Screenshot 2023-09-11 at 19 29 54

If it helps, the full workflow is downloadable here: https://perilli.com/ai/comfyui

I understand that the whole workflow might be too complex to support but, at least, having the Efficiency Loader and Efficiency KSampler nodes supported would help.

Thank you

Reference Issues.

No response

receyuki commented 1 year ago

Can you post an image generated using this workflow?

Unstable-Reality commented 1 year ago

Sure, @receyuki:

comfyui_base_00042

receyuki commented 1 year ago

I finally had some time to take a look at this workflow, but the bad news is that it's quite complex. Adding support for the Efficient loader is not difficult, but this workflow involves a large number of other custom nodes, and it's challenging for me to support such a large number of custom nodes.

I have just completed the development of the SD Prompt Reader ComfyUI node version, which includes a node similar to "Save Image w/Metadata", but fixes existing issues. I will be writing instructions within the next few days. https://github.com/receyuki/comfyui-prompt-reader-node

alessandroperilli commented 1 year ago

@receyuki I just published version 4.0 of my workflow above, with a series of Save Image w/Metadata nodes, as you recommended.

I'll keep an eye to see when your new node pops up in ComfyUI Manager and test if I can use that one, instead.

receyuki commented 1 year ago

My pull request was just merged into the ComfyUI Manager, so from now on, the ComfyUI Manager will handle the installation and update process.

If you encounter any issues or have any suggestions, please let me know.

alessandroperilli commented 1 year ago

Yes, I saw the new suite and installed it, thank you @receyuki.

My problem, which is really quite different from what I wrote in the title of this feature request, is that I cannot decouple the checkpoint name from everything else and carry it to your SD Prompt Saver node or to the Save Image w/Metadata node (which you previously suggested) via a series of reroute nodes.

This has nothing to do with your nodes, so I am sharing it just for the sake of documentation:

The only node I ever found that isolates the name of the checkpoint/modelname is the CheckPoint Selector node, in the Image Saver Tools suite.

This node connects to both your SD Prompt Saver node or to the Save Image w/Metadata node without issues.

But if I try to reach long distances with the help of reroute nodes (I mainly use rgthree Reroute nodes), the connection disconnects. Either immediately, or upon refresh, or randomly, as if there's a race condition that is not addressed.

Screenshot 2023-09-27 at 18 29 34

And this thing prevents me from keeping the workflow tidy.

Again, nothing to do with your nodes. I thank you for sharing them with the community.

I think this feature request can be closed.

receyuki commented 1 year ago

Firstly, thank you for your feedback.

In my nodes, there is a Parameter Generator node that can achieve the same thing as the CheckPoint Selector. The only difference is that I integrated all the selectors, generators, and literals from that suite into a single node.

Based on my guess, the reason for the disconnection might be due to different formats. My Prompt Saver node, like the official one, can only accept 'folder_paths.get_filename_list("checkpoints"),' which is a list containing all the checkpoint names. If you attempt to connect a STRING to 'model_name,' it will disconnect. If you don't mind, you can upload your workflow here, and I'll try to see if I can resolve the issue.

alessandroperilli commented 1 year ago

Thanks! The workflow is here. I left a red note for you.

receyuki commented 1 year ago

Damn it, everything is working fine on my end, and I can't reproduce this issue. Another possibility might be that a custom node's JS that you've installed is causing this bug.

alessandroperilli commented 1 year ago

This thing is driving me crazy. Since you can't reproduce, I recreated all reroute nodes (just in case) and started the process of migrating from the Save Image w/ Metadata node to your SD Prompt Saver node, as you can see below:

Screenshot 2023-09-27 at 21 11 52

However, as I queue a generation, an error informs me that the Efficient Loader is now missing the checkpoint name. This makes me discover that while the connections from the Checkpoint Loader to the SD Prompt Saver node now work, the wire connecting the Checkpoint Loader to the Efficient Loader is now severed:

Screenshot 2023-09-27 at 21 13 58

This is what must be happening: either I can go towards the Efficient Loader or towards the Save Image w/ Metadata // SD Prompt Saver node.

To test my theory, I try to reconnect the Checkpoint Loader to the Efficient Loader. It refuses to. The link appears possible but it refuses to maintain the connection. Which is exactly what was happening before towards the Save Image nodes.

receyuki commented 1 year ago

Oh my goodness! Maybe you need to find an alternative for 'reroute.' I just took a look at the source code for 'reroute', and it seems like the entire node is written in TypeScript, so I might not be able to help you.

But wait, are all the issues occurring with things like 'model_name'? In other words, STRING won't have similar problems? If that's the case, maybe I can try creating a node that converts this type into STRING and allows Saver to input STRING.

alessandroperilli commented 1 year ago

Yes, all the issues are because of this model_name output.

Perhaps it would be easier to create a replacement node that exclusively outputs the model/checkpoint name. Like your Parameter Generator, but just for the model name / checkpoint variable.

However, it would have to be

  1. compatible with the Efficient Loader
  2. compatible with your SD Prompt Saver
  3. compatible with rgthree Route

I'm not sure about 2 and 3, but it seems that the Efficient Loader doesn't accept a string input as ckpt_name.

But perhaps it's not worth your time.

Regardless, thank you for all the time you have invested in this so far.

alessandroperilli commented 1 year ago

BTW, it's not just rgthree reroute node. If I try to connect themodel_name output in your SD Parameters Generator node to the ckpt_name input in the Efficiency Loader node via a default ComfyUI Reroute node, I get the same rejection.

alessandroperilli commented 1 year ago

I found a hack:

If I put a rgthree Node Collector node just before the Efficient Loader node and the SD Parameters Generator / Save Image w/ Metadata nodes, nothing gets disconnected.

Screenshot 2023-09-27 at 23 38 18 Screenshot 2023-09-27 at 23 38 27

If I put the Node Collector at the top of the chain, before all reroute nodes, instead, the links continue to break.

It's a terrible hack, but it seems to be working.

receyuki commented 1 year ago

Managing such a complex workflow is undeniably challenging, I hope this hack runs reliably. model_name can be easily converted into a STRING, but it seems like comfyui doesn't support multiple input formats. Therefore, if we want to make Saver support STRING, we might need to add an additional model_name_str and explain in the readme that model_name_str is just an alternative input. What do you think?

alessandroperilli commented 1 year ago

That would help a lot!

receyuki commented 1 year ago

I've just added alternative inputs to Saver, please update and reload the nodes. Note that inputs of type STR have higher priority, so don't connect STR inputs unless necessary, as Saver will prioritize reading STR inputs over regular ones

截屏2023-09-28 下午8 12 47
alessandroperilli commented 1 year ago

This is great, thank you. I'll test it over the weekend and let you know.

alessandroperilli commented 1 year ago

This new SD Type Converter node is SO useful. I can now print checkpoint, sampler, and scheduler names for debug purposes. Thank you!

receyuki commented 1 year ago

That's great! After a few days of use, have you encountered any other issues or do you have any other suggestions? If everything is fine for now, I plan to release the stable version 1.0.0.

BTW, are "Unstable-Reality" and "alessandroperilli" both your accounts?

alessandroperilli commented 1 year ago

I encountered no issues with your nodes or your implementation. My new AP Workflow 4.0.2 implements both your SD Parameter Generator and your SD Type Converter node. Thank you!

The issue I still have is due to the way ComfyUI is implemented. I'll explain below, for transparency, but I have no expectation that you could fix this.

As I was explained by rgthree in this post (highly recommended reading), a checkpoint loader (either yours, inside the SD Parameter Generator node, or the one I was previously using), builds a list of available models which is used as output towards other nodes. A Save Image node does the exact same. If the total number of checkpoints in your computer changes, the workflow might generate a mismatch error between these two lists and the reroute nodes that connect them will disconnect the modelname input (like the one in the Save Image w/ Metadata node).

Apparently, there is no way to solve this problem, but to delete the existing nodes with the checkpoint lists and recreate them. Occasionally, I found that stopping ComfyUI and restarting it might fix it, too, but it's not clear to me why that would work.

It's very annoying because an image generation might complete successfully but you won't have your image saved because the modelname input is missing. Also, the sudden disconnection is very confusing for users who are not deeply familiar with how the workflow works.

Regarding your ask for suggestions, I have a few :) They are related to the SD Parameter Generator.

As you can see in this partial screenshot of AP Workflow 4.0.2, I cannot use your node in its entirety because it's missing five key features that I find in other nodes.

Screenshot 2023-10-04 at 12 47 15

Here's the five features:

  1. A button to quickly reuse the last seed. This is incredibly useful and I have to rely on rgthree Seed node for it.

  2. A way to automatically define the ratio between base and refiner steps. As you can see I created my own automated function for that by using 2 nodes. Some nodes allow you to define at what step to finish with the Base and at what step to start with the Refiner, but I preferred to go beyond that, giving the users the chance to input a percentage (e.g., do 75% of the steps with the Base). I then pass that variable to the rgthree Big Context node.

Screenshot 2023-10-04 at 12 51 10

This is a particularly delicate feature because fine-tuned SDXL models don't use the Refiner, and so the users must be able to specify the steps percentage AND be able to say if they want the refiner at all (they could set 0% to mean "no Refiner", but I don't think it's obvious).

  1. A way to automatically set height and width based on SDXL training ratios. For that, I use a CR SDXL Aspect Ratio node.

  2. A way to set the batch size. For that, I use the same CR SDXL Aspect Ratio node.

  3. A way to set independent positive and negative aesthetic scores. For that, I use simple Constant Number nodes.

I'm happy to keep using a mix of your nodes and others as is today, but I think it's a bit confusing for people to see some outputs connected and some others not.

IMO, if you'd add these capabilities, you'd have the ultimate SD Parameter Generation node.

A final suggestion:

Screenshot 2023-10-04 at 13 09 20

It would be easier if the drop-down config menu had an option called none or built-in selected by default, or if the config entry would disappear when the with_config switch is set to off.

Thank you!

(Yes, Unstable Reality is my AI R&D company. I used that account by mistake at the beginning of this thread. You can force close this feature request, or I can close it with that account.)

receyuki commented 1 year ago

I just read rgthree's explanation, and it's ridiculous. I also checked reroute's source code try to understand how they fixed this issue, but it seems I can't implement a similar fix on my node due to my limited JS skills.

I recently realised a new problem. Every time I update the node and add new input/output, all workflows that use this node break because comfyui seems to locate connections between output and input solely by index rather than by name. For example, in the old version of node A, there were four inputs (a, b, c, d). When I updated to the new version with inputs (a, b, b1, c, d) and reloaded the node, (b1, c, d) would break (because comfyui treats b1 as c and c as d), and I have to reconnect them. So, I think I should try to finalise all inputs and outputs before the stable release.

About the feature suggestions:

  1. It sounds very reasonable. I checked the source code of Efficiency Loader and rgthree, and they both implemented this feature in JS. I may need to spend some time learning how they did it, and if possible, I will add this feature.

  2. Since I initially created the Parameter Generator just to make Prompt Saver work correctly, I didn't consider anything beyond what Prompt Saver needed. However, considering that KSampler require this parameter and it's just a few lines of code, I will add this feature. (As you mentioned: add an input named base_refiner_step_ratio and an output named STEPS_REFINER, where STEPS = steps * ratio, STEPS_REFINER = steps - STEPS, and the default value of ratio is 1.0, meaning the refiner is turned off. How does that sound?)

  3. Thank you for letting me know about SDXL's optimal resolution and ratio. I will add the resolutions mentioned on https://platform.stability.ai/docs/features/api-parameters as optional choices.

  4. This also makes sense and is easy to implement.

  5. I completely forgot about aesthetic scores. Since the models I use daily are SD1.5, I have never used aesthetic scores. However, considering that CLIPTextEncodeSDXLRefiner has this parameter, I will add this input.

  6. About final suggestion, I have already made changes in the dev branch. When I was writing it, I simply copied the original code over and didn't think of this (clearly better) method.

I really appreciate your suggestions, they have been very helpful to me. BTW, it's His her SD Parameter Generator node :)

alessandroperilli commented 1 year ago

Ridiculous or not, every time I add or remove a checkpoint file to my directory, the workflow breaks. It used to disconnect the wire going into the modelname input of the Save Image node. Now, instead, it raises an error and tells me that the hash of the checkpoint list has changed. Either way, if my directory of checkpoints changes, I need to go through acrobatics to make the workflow work again. And while I am capable of doing that, it makes really hard to distribute the workflow to other members of the community.

Re 2: IMO, the easiest-to-understand implementation would be that the node asks you to input the Refiner steps as a percentage and, as you do, it automatically calculates the number of steps and displays it in parenthesis.

For example, the label could say "Refiner Steps %", you put in 20 (as in 20%), and automatically, in parenthesis, the node shows you that 20% equals 12 (as in 12 steps).

I think the ratio is less intuitive to understand, but whichever implementation you will choose, it will be welcomed. Thank you.

Re all the other points discussed: great! I look forward to using the updated node a near future.

Thank you for implementing all these features.

receyuki commented 1 year ago

Finally, everything mentioned above has been completed. How do you feel about this? I've thought about the "refiner_start" naming for a while, and personally, I think "refiner_start" or "refiner_start_at" would be a better choice.

截屏2023-10-13 下午11 22 40
alessandroperilli commented 1 year ago

I'm very excited about this. I stand by what I said before and I think you have built the definitive Parameter Generator. Can't wait to try it.

The only thing that I would recommend is rearranging the vertical order of certain elements, if possible. Not just in terms of selectors, but also in terms of output order*.

I could give you the full order I think would work best, if you'd consider the possibility.

*I wish the underlying engine had a way to arbitrarily rearrange output and selector elements without messing with Python code. It would be transformative for the cleanness of the workflows.

receyuki commented 1 year ago

About rearranging elements, I think it can be done by modifying some of the comfyUI's code using JS. Perhaps someone proficient in JS could do it. I can only adjust the input and output order by editing the Python code.

Currently, the input and output order I've designed is mostly based on KSampler, as shown in the image below. Additionally, I've placed common parameters at the beginning and SDXL parameters at the end. About the bottom section of the node (textboxes and buttons), I haven't found a way to rearranging it, so I've left it at the end.

If you have any better ideas, please don't hesitate to let me know.

截屏2023-10-15 上午3 03 51
alessandroperilli commented 1 year ago

It's unfortunate that the textboxes and buttons cannot be placed elsewhere, in between the output dots. And, what I'll suggest below might become useless in a few weeks if Stability AI (hypothetically) releases SDXL 3.0 that doesn't require the refiner component anymore.

That said, this is the order that I think would be most logical:

OUTPUTS

  1. Model_name
  2. Model
  3. CLIP
  4. VAE
  5. Seed
  6. Steps
  7. Basesteps << consider renaming this Base_end_step to maintain coherence with various KSamplers_
  8. Refinersteps << consider renaming this Refiner_start_step to maintain coherence with various KSamplers_
  9. CFG
  10. Sampler << remove suffix name to maintain consistency with the Scheduler output
  11. Scheduler
  12. Positive_ascore
  13. Negative_ascore
  14. Width
  15. Height
  16. Batch

SELECTORS

  1. model_version
  2. modelname << I would call it this way rather than ckptname, to maintain consistency with the output label_
  3. model_config
  4. seed
  5. steps
  6. refiner_start
  7. CFG
  8. sampler
  9. scheduler
  10. positive_ascore
  11. negative_ascore
  12. aspect_ratio
  13. width
  14. height
  15. batch _size

INFO WINDOWS

  1. Step calculator
  2. Image dimensions calculator

I'm suggesting this inverted order to maintain consistency with both the Output section and the Selectors section, where I suggested keeping the image settings towards the bottom.

I understand that this re-arrangement wouldn't align perfectly with the KSampler inputs, but I believe that those inputs were not designed for a world where the Refiner exists.

Whatever you choose, IMO, it's a massive improvement over the current version, and I'll 100% incorporate it in the AP Workflow.

A final suggestion

If you could add a final output dot, at the very end, that outputs ALL of this as a list of strings label: value, that would be monumental.

At that point, we could use 3rd party nodes to print the whole list in the terminal and/or save the whole list in a file, for debug and logging purposes. Today, I have to do this via an expansive Debug section with a dozen nodes, and it could be almost completely avoided:

4 0 2 Debug

I can't think of anything else that could improve this node. Great job!

receyuki commented 1 year ago

I've implemented some suggested changes (but not all of them), and I'll explain why.

About order: After careful consideration, I find all your suggestions to be quite reasonable. As a result, I've rearranged inputs and outputs according to your recommendations. The only exception is ckpt_name, which I believe should be placed at the beginning.

About renaming:

BTW, parameters is the debug output you're looking for.

I'll merge dev into main ASAP after I finish modifying the readme. Just merged

截屏2023-10-17 下午11 31 48
alessandroperilli commented 1 year ago

I'm very grateful for this work. It looks great.

I'm testing it now and I have a few minor things to report:

  1. The two windows describing what's happening and the buttons at the end don't appear in my version:
Screenshot 2023-10-17 at 18 48 10

I updated the extension and restarted ComfyUI, as usual. Not sure what's the issue.

  1. Would it be possible to be more detailed in the aspect_ratio options? For example, instead of just writing 16:9, could we see the exact dimensions chosen, like in the other implementation I was using?
Screenshot 2023-10-17 at 18 37 58
  1. I am not sure this is possible, but as the user selects a certain aspect ratio, is it possible to automatically change the values of width and height to reflect the choice?

For example, if one chooses 16:9, could width automatically become 1344 and height automatically become 768?

I always found it extremely confusing, even in the other node I was using, that one selects a certain aspect ratio but those values don't reflect the choice.

  1. Would it be possible to have more precision in the definition of the Refiner start? Right now I can only choose between 0.8 and 0.9, but I usually like to set it at 0.75 for a 75% ratio.

  2. Would it be possible to print additional information in the parameters output? It's particularly important to print the base ends at step X and refiner starts at step Y.

Finally: I understand the logic for not wanting to rename the base_end and refiner_start outputs, but I would politely flag the following situation I have:

Screenshot 2023-10-17 at 18 56 30

Here, to make it work as expected, I have to connect the Base_Steps output of the SD Prompt Generator node to the step_refiner input of rgthree's Context Big node. I know what's going on and I can deal with it, but it might be very confusing for other people.

A person who is less familiar with ComfyUI and the intricacies of the SDXL architecture might be naturally inclined to connect your refiner_steps output with his step_refiner input, which causes the SDXL Base model to run the few steps meant for the Refiner.

It's not the end of the world, but maybe this might be worth some more design thoughts.

Sorry for the extra requests. This node is fantastic.

receyuki commented 1 year ago
  1. Just fixed.
  2. Different model versions have different optimal resolutions for the same aspect ratio. To avoid an excessively long list, I may need to split the node into multiple versions, similar to what other custom nodes do. This is why I decided to display this information in the info window.
  3. Great idea, just made the change.
  4. Just fixed
  5. Since I can't detect whether refiner_steps is connected to anything, I'm concerned that many users, especially those using SD 1.5, might simply ignore it rather than set refiner_start to 1. In such cases, output this information might seem odd. (model_version is unreliable)
  6. Actually, the refiner_steps in the node is useless, it wont's be used in any nodes. Since base_end_step is equal to refiner_start_step, it might be better to change base_steps to refiner_start_step and remove refiner_steps.
alessandroperilli commented 1 year ago

Trying it now. It works splendidly.

I agree on 6.

Very minor final comment: you might want to remove the comma after every debug line:

Screenshot 2023-10-17 at 23 07 50
receyuki commented 1 year ago

I added the commas in the parameters for consistency with the format of settings, considering that "parameters" might be used for other purposes.

Just updated 6

alessandroperilli commented 1 year ago

Everything works perfectly, and this was a nice touch:

Screenshot 2023-10-18 at 00 06 24

Time to close this epic feature request. Thank you!

receyuki commented 1 year ago

If you have any new ideas, please feel free to contact me anytime. :)

alessandroperilli commented 1 year ago

The only thing I can think about to improve the Parameters Generator node is to generate the output of the two windows at the bottom in real time rather than after you hit Queue as it happens now. The reason is that it might be rather confusing for people to see those values when they just set up a very different thing with the sliders. But I don't know if it's technically possible.

Other than that, I have very many other ideas for completely different nodes. Nothing related to this specific one.

receyuki commented 1 year ago

This is something I've been looking for as well. I have checked all the available documentation and the source code of commonly used custom nodes, but it seems there is no API that can do this. The current API only supports triggering during workflow runtime.

receyuki commented 1 year ago

BTW, there are a few issues with the Save Image w/Metadata node you’ve been using in your workflow.

Therefore, I recommend using my Prompt Saver as a replacement, which not only solves all the above problems and has all the features of Save Image w/Metadata, but also adds some new parameters.

alessandroperilli commented 1 year ago

Yes, that node is VERY slow and I was hoping to fix the problem. But I simply followed your original recommendation and didn't have the time to review alternatives (including your Prompt Saver).

Now I did some tests and the next version of AP Workflow will have your Prompt Saver in place of the Save Image w/Metadata node. Nothing makes me happier than reducing the number of dependencies on 3rd party custom nodes, especially if they are not maintained anymore.

Plus, the extra info input is particularly useful for something I was thinking about earlier today.

The only thing that I don't fully understand is the function of these three inputs:

Screenshot 2023-10-18 at 19 51 39

I thought I could use them to create custom labels for model, sampler, and scheduler values, but they don't appear in the metadata when I read the image with your SD Prompt Reader (I use the macOS version), so I simply leave them blank.

Other than that, another great node. And yes, I have a couple of suggestions about it (but it's much better if I open a separate issue for it).

receyuki commented 1 year ago

I've just added alternative inputs to Saver, please update and reload the nodes. Note that inputs of type STR have higher priority, so don't connect STR inputs unless necessary, as Saver will prioritize reading STR inputs over regular ones 截屏2023-09-28 下午8 12 47

As I mentioned earlier, they are just some backup inputs because you said model_name, sampler_name, and scheduler would sometimes get disconnected.

I have just tested these three inputs, everything is working fine on my end.

截屏2023-10-19 上午3 23 36 截屏2023-10-19 上午3 26 14

But now, I feel they are not needed. If these three inputs are disconnected, the other connections might have already been a mess. So perhaps I can simply remove them.

Other than that, another great node. And yes, I have a couple of suggestions about it (but it's much better if I open a separate issue for it).

You can open a new issue in https://github.com/receyuki/comfyui-prompt-reader-node/issues

alessandroperilli commented 1 year ago

Ah, I see. Only the model name gets disconnected, and it happens when the list of checkpoint in the file system changes.

Those three backup inputs I cannot use anyway because they don't allow an inbound connection from rgthree Context Big node. So I'd say it's safe to comment them out until I can get to a point where I can reproduce the bug at will. I'm amazed so few people have noticed this issue among the ones that build complex workflows.

receyuki commented 1 year ago

Alright, see you in another repo