feat: support random seed before generation #93
Conversation
|
It works. Does it make sense for this kind of seed behaviour to be a setting that can be adjusted per node instead of a global setting? |
|
I think this should be on a per-node basis. I would definitely use it that way, at least. |
|
Would it make sense to let the user decide weither it's a global setting or not by making a node "RandomNumberGenerator", and adding an integer (that has a different type than INT) as an input to KSamplerAdvanced for setting the seed? -> like that, if I want to share a seed, I can simply connect multiple KAdvancedSampler to the same RandomNumberGenerator. If I want independant seeds, I can create 1 RandomNumberGenerator per KAdvancedSampler. I think this would give a lot more control for handling seeds. |
I often use the same seed for the first and second pass (upscaled), and also other values, so that would be very useful. It would be better to add the ability to connect nodes for primitive types (int, float, string ...) and create nodes for values (ValueString, ValueInt, ValueFloat, ...) and add a randomize option in the numeric nodes. |
|
This is definitely a must, and should have been this way from the get go, cause when you generate, it immediately switches out your seed, so you're left looking to PNG info. It just kinda seems backwards. You should know your seeding going into the gen. |
|
I implemented an hijacked sampler node that reuses the original one to use 2 latent inputs (1 for the image, the other for the noise) and that reuses the original code as much as possible. I also implemented a latent noise generator, and a random number generator to go with it. Let me know if you want them, I can share them here later. One cool thing you can do with them is generate offset noise towards a certain color to make the sampler more likely to generate that color, for example. Though I think these nodes should be in ComfyUI from the get go instead of having to make them myself, and that we shouldn't have to rely on tricks like this to make it work. Here's what it looks like: |
This is actually smart, and more conducive with what is actually going on, on the diffusion side as apposed to the random seed int which is just a input for the noise. When we get optional inputs the official ksampler definitely needs a seed input and noise input. I love this. I called the |
|
Because every input can potentially be "random" because of: #243 I think the random behaviour should be adjusted as a global option especially if there are more options added than just "random" like "increment" or "decrement". What do you think? |
Shouldn't it just be a mode then with the node? global settings for something you may want on for one node, and off for another is silly business. |
|
Are you really going to mix "random before every gen" and "random after every gen" on the same workflow? |
Why not? You're making a node network, these are things you leave as options, as it's for custom workflows, whatever they me, especially what you might not envision. |
|
I'm just asking because I want to get it as correct as possible the first time because if it needs to be changed later it will be a pain. |
Yeah that's true. I am just thinking it may be considered an annoying switch to have to go into settings? A place that's otherwise hidden. Also a workflow description/tutorial/post may unintentionally lead a person to think things that aren't true for them cause of user preferences. While this does kinda seem like a preference thing, on a logic side there could be a reason for both. I honestly can't think of a reason, but there could be. Maybe a memory node that retains seeds or something and compares seeds (why? I don't know, but people have been doing ridiculous stuff with seeds, let alone with SD back-end in general) |
|
It comes down to different ways of seeing things. I see the UI as a queue where what you see is what is sent directly to the queue when the "queue prompt" button is pressed. I understand why many people see it the other way though. |
|
maybe it's just me, but wouldn't it be possible to add both? Both a global setting, and settings on individual nodes. The global setting could have priority over every seed if it's set to "override node behavior" or something. By default, it would be set to "set seed manually for every node". If it's set to "override node behavior", then the "Random seed after every gen" text would be greyed out. Would that be possible? |
|
this is something i was thinking about so i'll say it here since there's discussion about global settings. how about handling global setting as a node? it wouldn't connect to anything (maybe as the first thing that gets executed?) and would basically be always active (if placed, if not placed then default settings could apply instead). that way you can transfer workflows to other people and still be sure they'd execute the exact same way. an implementation like this would also fit in much better in the node based nature of this project imo |
Would be a great way to share a whole workspace and workflow. Especially considering any additions that may turn up in global setting that may impact workflow usage.
Wouldn't that cause confliction? User reports bug their seed isn't working right to find they set a global setting that conflicts with node settings? Or wonders why settings are locked not taking note of a global setting? |
|
While I encourage discussion, I don't think this is the right place for it unless you're talking about this PR. Maybe we can move this tangential conversation elsewhere? Are there changes you want me to make to this PR, or can it be merged? |
It's directly related to the PR and whether it should be merged, or not, based on global setting. |
|
This PR is only adding the possibility to randomize the seed before the generation. Weither it is a global setting or node-based is another matter that should be addressed on its own. I've created an issue to continue this discussion #278. |
And the discussion is whether or not that should be the case. I don't know what the confusion is or why you are being combative. Do you feel threatened that your PR won't be merged or something? But the conversation is literally about if it should, or if it shouldn't and just be a global setting. It shouldn't be a anywhere else but this PR to prevent confusion and cross posting |
|
Yes lets move this discussion to #278 |
|
I don't need spam notifications for two topics regarding the same thing. That's ridiculous |
|
It's his pull request and he politely asked use to move the discussion elsewhere so lets do that. You also won't get spam notifications from this topic because discussion will happen in the other one. |
It's a PR to a project, it isn't "his" PR request. And we should be following PR and Github etiquette. Not making special cases cause someone is annoyed they're getting notifications. Pull Requests are the fundamental unit of how we progress change, and track how we got there. We shouldn't have discussion split out places, especially when it's regarding a PR. This is literally what this discussion is for. I'll continue to follow PR and Github etiquette. |
|
@WASasquatch You're putting a lot of word in my mouth that I never said. I'm not being combative. I'm not annoyed by the notifications. And I'm not worried this PR won't be merged. It's simply that the changes you are discussing are changes that I'm not going to make because they are out of the scope of my original implementation and goal for this PR. If someone wants to fork this and add global settings, that is fine by me, but I'm not going to be the one to do it. So I don't see the point in discussing it here. Either review this PR as it was originally intended or I will close it. I will not be implementing global settings in this PR. |
|
Let's say I set it to before, queue the prompt and I liked the result, but I want to change things after the sampler but I want to keep the same seed, if I set it to off will it make the sampler run again? Because I don't want to run the sampler again, if it doesn't run the sampler again then it is exactly what I need for my workflow. |
|
@jn-jairo It will not run the sampler again. |
Great, does it work with the new widget to input? So I can share the seed. |
Not yet. Let me rebase. |
But the point of that conversation is literally pertaining to whether the PR was appropriate to begin with, or should be handled another way. It directly pertains to the nature of this PR and a track record of it's discussion of desired implementation. As like we have also mentioned, having both, is ill-advised and would create end-user confliction and possible issue reports for nothing. |
|
@WASasquatch The project maintainer asked you to move the conversation elsewhere. @comfyanonymous Please mark these comments as off topic. |
|
@jn-jairo Okay, it's been rebased and it works with the new Primitive and Input/Widget conversion settings. |
I just tried, and it works, that is what I need for my workflow. |
|
I'd like to clear the air and come to an agreement with @WASasquatch on this subject. I don't want to sound draconian, combative, petty, or even like I'm going against GitHub etiquette. First, I don't want to stifle any discussion. I'm glad to see so many of us interested in this project and discussing how it can be improved. That's why I suggested that the global settings conversation continues—just not here. The issue is about scope. The topic of this PR is "supporting random values before generation". It's a relatively small scope that adds the option to generate random values before image generation rather than just after or never. (As we now have a Primitive node that can represent integers other than seeds, this goes beyond just seed generation.) Whether or not we have global seed generation options is outside the scope of this issue since it is a separate topic, one that this PR is not addressing. If the consensus is to wait to merge this PR until deliberation has finished on whether we should have global seed generation settings, that is fine with me. We can add a comment that this PR is on hold and link to the global seed discussion. But this is not the place for that discussion. In fact, the only reason that discussion is here is because comfy asked that intriguing, yet tangential, question in their first comment. But it is outside of the scope of this PR because it does not answer the question "should we have an option to generate random values before image generation?" Now, let me address the comment I made about not implementing global seed generation settings in this PR, as I don't want anyone to get the wrong impression. What I mean by that is, this PR is only concerned with adding the option to allow random value generation before image generation. If code is committed to the main branch that adds global seed generation options, I'd be happy to rebase and update this PR to accommodate the updated codebase. What I will not do is implement global seed generation settings in this PR because that is not within its scope. What I will do is add the before generation feature to any existing global settings should they exist before this PR is merged. @WASasquatch I get what you're saying about not wanting to have discussions split up. But having a discussion about global settings in a PR that doesn't address that issue is not following GitHub etiquette since it's inflating the scope of the PR. We should follow the pattern of separation of concerns so that we can focus our conversations. This thread should be for reviewing the code in the PR and its affect on the codebase. The discussion about whether we should have global seed settings should separate. I hope that clears things up. I don't want to promote any negativity in this community, and I'm sorry if anything I said came off that way. |
|
@comfyanonymous There are still two outstanding issues with this PR. I've updated the description with checkboxes for them, but I'll list them here too.
|
|
You can patch workflows before and right after they are loaded with |
This is how these discussions work everywhere when the argument is whether the PR is relavent because of something, like comfy originally raised, which is a solid point which pertains to the whole existence of this PR. It's directly related to whether this PR should be merged. Is it a good idea to merge it because there could be a global setting. Cross referencing posts on this debate is negligence imo. Currently most of the debate and information is here. |
|
To be clear @jordanbtucker it was rude to put words in your mouth, but no offense, it seems clear you want to maintain a positive veiw and conversation about your PR and not detract from it's appeal to merge. Which I think is off-top. PRs come with rebuttal on whether they're relevant or needed VS other proposed ideas of stuff on the roadmap. And to further clarify, I am on board with your idea, just do not like cross referencing ideas to keep things, clean, positive, or whatever it may be. The points raised are directly made against the PR itself, not something else. It's whether the PR would be needed over a global setting, but I think that using both for different tasks is important. Your idea to split things is odd, cause what if it was a established idea by someone else, like me, for example. Do I just put in a challenging PR that directly works against your PR? No, we should discuss in your PR, cause the ideas would conflict. |
Thanks for the tip. This PR is now backward compatible with workflows created before the time this gets merged. If you're okay with calling |
|
Okay, I think I finally understand what everyone is talking about when they say "global setting" and the confusion is due to the fact that I'm being dense. 😳 When comfy suggested the setting to be global instead of per-node, I took that to mean that all seed generation settings would be moved out of nodes and into a global settings panel. And that never made sense to me. What comfy probably meant is that nodes would still have a toggle to turn randomness on or off, but there would be another global setting that would be a toggle for whether the random value would be set before or after image generation. With this viewpoint, the conversation makes a lot more sense and I can see why it's directly relevant to this PR. I think I completely missed the point and that's why it didn't make sense to me to be talking about global settings when all I wanted was to change this one little quirk. 😁 @WASasquatch and everyone, please forgive my stupidity. I'm open for discussion on whether this before/after setting should be global, now that I think understand it, and I think it makes sense to continue that conversion here. Please correct me if I'm missing any other crucial points of discussion. 🙏 |
Yes that's exactly what I meant, sorry for the confusion.
Instead of calling graphToPrompt you can iterate on the nodes like this: https://github.com/comfyanonymous/ComfyUI/blob/master/web/scripts/app.js#L660 |
|
is there anything preventing this from being merged? I find the "after generation" random number generation weird or maybe I'm doing it wrong and there's a better workflow. |
|
This is covered by a setting now |
Update image_nodes.py
* Add Ideogram generate node. * Add staging api. * Add API_NODE and common error for missing auth token (#5) * Add Minimax Video Generation + Async Task queue polling example (#6) * [Minimax] Show video preview and embed workflow in ouput (#7) * Remove uv.lock * Remove polling operations. * Revert "Remove polling operations." * Update stubs. * Added Ideogram and Minimax back in. * Added initial BFL Flux 1.1 [pro] Ultra node (#11) * Add --comfy-api-base launch arg (#13) * Add instructions for staging development. (#14) * remove validation to make it easier to run against LAN copies of the API * Manually add BFL polling status response schema (#15) * Add function for uploading files. (#18) * Add Luma nodes (#16) * Refactor util functions (#20) * Add VIDEO type (#21) * Add rest of Luma node functionality (#19) * Fix image_luma_ref not working (#28) * [Bug] Remove duplicated option T2V-01 in MinimaxTextToVideoNode (#31) * Add utils to map from pydantic model fields to comfy node inputs (#30) * add veo2, bump av req (#32) * Add Recraft nodes (#29) * Add Kling Nodes (#12) * Add Camera Concepts (luma_concepts) to Luma Video nodes (#33) * Add Runway nodes (#17) * Convert Minimax node to use VIDEO output type (#34) * Standard `CATEGORY` system for api nodes (#35) * Set `Content-Type` header when uploading files (#36) * add better error propagation to veo2 (#37) * Add Realistic Image and Logo Raster styles for Recraft v3 (#38) * Fix runway image upload and progress polling (#39) * Fix image upload for Luma: only include `Content-Type` header field if it's set explicitly (#40) * Moved Luma nodes to nodes_luma.py (#47) * Moved Recraft nodes to nodes_recraft.py (#48) * Add Pixverse nodes (#46) * Move and fix BFL nodes to node_bfl.py (#49) * Move and edit Minimax node to nodes_minimax.py (#50) * Add Minimax Image to Video node + Cleanup (#51) * Add Recraft Text to Vector node, add Save SVG node to handle its output (#53) * Added pixverse_template support to Pixverse Text to Video node (#54) * Added Recraft Controls + Recraft Color RGB nodes (#57) * split remaining nodes out of nodes_api, make utility lib, refactor ideogram (#61) * Add types and doctstrings to utils file (#64) * Fix: `PollingOperation` progress bar update progress by absolute value (#65) * Use common download function in kling nodes module (#67) * Fix: Luma video nodes in `api nodes/image` category (#68) * Set request type explicitly (#66) * Add `control_after_generate` to all seed inputs (#69) * Fix bug: deleting `Content-Type` when property does not exist (#73) * Add preview to Save SVG node (#74) * change default poll interval (#76), rework veo2 * Add Pixverse and updated Kling types (#75) * Added Pixverse Image to VIdeo node (#77) * Add Pixverse Transition Video node (#79) * Proper ray-1-6 support as fix has been applied in backend (#80) * Added Recraft Style - Infinite Style Library node (#82) * add ideogram v3 (#83) * [Kling] Split Camera Control config to its own node (#81) * Add Pika i2v and t2v nodes (#52) * Temporary Fix for Runway (#87) * Added Stability Stable Image Ultra node (#86) * Remove Runway nodes (#88) * Fix: Prompt text can't be validated in Kling nodes when using primitive nodes (#90) * Fix: typo in node name "Stabiliy" => "Stability" (#91) * Add String (Multiline) node (#93) * Update Pika Duration and Resolution options (#94) * Change base branch to master. Not main. (#95) * Fix UploadRequest file_name param (#98) * Removed Infinite Style Library until later (#99) * fix ideogram style types (#100) * fix multi image return (#101) * add metadata saving to SVG (#102) * Bump templates version to include API node template workflows (#104) * Fix: `download_url_to_video_output` return type (#103) * fix 4o generation bug (#106) * Serve SVG files directly (#107) * Add a bunch of nodes, 3 ready to use, the rest waiting for endpoint support (#108) * Revert "Serve SVG files directly" (#111) * Expose 4 remaining Recraft nodes (#112) * [Kling] Add `Duration` and `Video ID` outputs (#105) * Fix: datamodel-codegen sets string#binary type to non-existent `bytes_aliased` variable (#114) * Fix: Dall-e 2 not setting request content-type dynamically (#113) * Default request timeout: one hour. (#116) * Add Kling nodes: camera control, start-end frame, lip-sync, video extend (#115) * Add 8 nodes - 4 BFL, 4 Stability (#117) * Fix error for Recraft ImageToImage error for nonexistent random_seed param (#118) * Add remaining Pika nodes (#119) * Make controls input work for Recraft Image to Image node (#120) * Use upstream PR: Support saving Comfy VIDEO type to buffer (#123) * Use Upstream PR: "Fix: Error creating video when sliced audio tensor chunks are non-c-contiguous" (#127) * Improve audio upload utils (#128) * Fix: Nested `AnyUrl` in request model cannot be serialized (Kling, Runway) (#129) * Show errors and API output URLs to the user (change log levels) (#131) * Fix: Luma I2I fails when weight is <=0.01 (#132) * Change category of `LumaConcepts` node from image to video (#133) * Fix: `image.shape` accessed before `image` is null-checked (#134) * Apply small fixes and most prompt validation (if needed to avoid API error) (#135) * Node name/category modifications (#140) * Add back Recraft Style - Infinite Style Library node (#141) * Fixed Kling: Check attributes of pydantic types. (#144) * Bump `comfyui-workflow-templates` version (#142) * [Kling] Print response data when error validating response (#146) * Fix: error validating Kling image response, trying to use `"key" in` on Pydantic class instance (#147) * [Kling] Fix: Correct/verify supported subset of input combos in Kling nodes (#149) * [Kling] Fix typo in node description (#150) * [Kling] Fix: CFG min/max not being enforced (#151) * Rebase launch-rebase (private) on prep-branch (public copy of master) (#153) * Bump templates version (#154) * Fix: Kling image gen nodes don't return entire batch when `n` > 1 (#152) * Remove pixverse_template from PixVerse Transition Video node (#155) * Invert image_weight value on Luma Image to Image node (#156) * Invert and resize mask for Ideogram V3 node to match masking conventions (#158) * [Kling] Fix: image generation nodes not returning Tuple (#159) * [Bug] [Kling] Fix Kling camera control (#161) * Kling Image Gen v2 + improve node descriptions for Flux/OpenAI (#160) * [Kling] Don't return video_id from dual effect video (#162) * Bump frontend to 1.18.8 (#163) * Use 3.9 compat syntax (#164) * Use Python 3.10 * add example env var * Update templates to 0.1.11 * Bump frontend to 1.18.9 --------- Co-authored-by: Robin Huang <robin.j.huang@gmail.com> Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: thot experiment <94414189+thot-experiment@users.noreply.github.com>
8 participants
This PR adds the ability to generate a random seed before the workflow is queued.
after generationfor backward compatibility.beforeQueuedcallback.after generation.trueandfalsefor backward compatibility with existing workflows.after generationwhen it seestrueandoffwhen it seesfalse, but I haven't figured that out yet.graphToPrompttwice. It feels hacky, so maybe there's a better way to implement that part.TODO
truevalues should be changed toafter generation, andfalsevalues should be changed tooff.graphToPromptreally needs to be called twice or if there is a better way to inspect and update the random widgets before generation.