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

In inner_sample, change "sigmas" to "sample_sigmas" #6360

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

Kosinkadink
Copy link
Collaborator

@Kosinkadink Kosinkadink commented Jan 5, 2025

In _calc_cond_batch, "sigmas" on transformer_options is written with the value of t, which is only the current step's sigma (and this PR does not change it). In inner_sample, there is another "sigmas" assigned to transformer_options earlier that is intended to have all the sigmas used for the current sampling run (i.e. all the steps that will be run).

When I added that inner_sample "sigmas", I did not notice the string was already used inside _calc_cond_batch, so third-party nodes that were intending to use the full sample sigmas do not work as intended since those values end up not being exposed anywhere.

So this PR just changes the "sigmas" assigned to transformer_options in inner_sample to "sample_sigmas" to make sure there is no overlap in the name. In the long term, we should have a class defined somewhere that keeps track of all the possible vanilla entries to transformer_options to avoid accidental clashes.

…ptions to not conflict with the "sigmas" that will overwrite "sigmas" in _calc_cond_batch
@blepping
Copy link
Contributor

blepping commented Jan 6, 2025

At first I was panicking because I thought you were talking about renaming the existing transformer_options["sigmas"]. If I understand correctly though, you previously added code to set it to the full sigmas list but that had no effect because it was (fortunately) overwritten by other code, so now you're adding it under a different name that doesn't conflict with existing keys. Is that right?

@catboxanon
Copy link
Contributor

catboxanon commented Jan 6, 2025

Commit message calls it sampler_sigmas but the actual code uses sample_sigmas. Which is the intended name?

@Kosinkadink
Copy link
Collaborator Author

yep, the purpose was to expose a new property on transformer_options that would be the list of all sigmas that will be seen by a call to CFGGuider's sampling. Unfortunately, most 'constants' in ComfyUI are just string literals instead of being centralized somewhere, so I missed that 't' was being assigned to "sigmas" later in _calc_cond_batch. What is referred to now as "sample_sigmas" was properly read by the hook keyframes code anyway, since "sigmas" was not set until later in the code, so nothing broke. But no third party code could actually make use of the intended "sample_sigmas".

In the near future, I plan to make things like properties for patches, transformer_options, and model_options have a class defining some consts that can be referenced to get those strings, like wrappers/callbacks have in WrappersMP/CallbacksMP in patcher_extension.py. This will eliminate the need to scour the entirety of the codebase to know for sure that there is no string clashing.

@Kosinkadink
Copy link
Collaborator Author

Commit message calls it sampler_sigmas but the actual code uses sample_sigmas. Which is the intended name?

The intended name is as it is actually in the code, 'sample_sigmas', i typo'd in the name of the commit.

@comfyanonymous comfyanonymous merged commit c496e53 into master Jan 6, 2025
6 checks passed
@comfyanonymous comfyanonymous deleted the add_sample_sigmas branch January 6, 2025 06:36
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