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

Update sd_samplers_cfg_denoiser.py #16814

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kittensx
Copy link

@Kittensx Kittensx commented Jan 28, 2025

This commit introduces a safeguard to ensure that schedulers and samplers strictly adhere to the requested step count during iterative processing. The change prevents the system from exceeding the total number of steps, avoiding potential errors, inconsistencies, or unexpected behavior caused by unintended step overflows.

A check is added to verify that the current step (self.step) does not exceed or equal the total allowed steps (self.total_steps). If the condition is met, an InterruptedException is raised, gracefully halting further processing.

Description

  • a simple description of what you're trying to accomplish
  • a summary of changes in code
  • which issues it fixes, if any

Screenshots/videos:

Checklist:

This commit introduces a safeguard to ensure that schedulers and samplers strictly adhere to the requested step count during iterative processing. The change prevents the system from exceeding the total number of steps, avoiding potential errors, inconsistencies, or unexpected behavior caused by unintended step overflows.

A check is added to verify that the current step (self.step) does not exceed or equal the total allowed steps (self.total_steps).
If the condition is met, an InterruptedException is raised, gracefully halting further processing.
@Kittensx
Copy link
Author

I found this error while working with an advanced scheduler - though I fixed the code in the scheduler, I thought this would be easily fixed with a safeguard in this file, which is the next file up from parse_prompter.py. During the loop, if a scheduler or sampler modifies the steps (accidentally or on purpose), instead of erroring out it simply drops any steps that exceed the requested user steps and finishes the image.

Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change directly breaks Sampling method: DPM adaptive
which is by design to have a non-deterministic amount of steps


lets suppose this doesn't break stuff like DPM adaptive, I still don't really see a reason to add the safeguard
as this really is not a common issue, it's only happens if you're working with some advanced things
and the other side of "safeguarding" is "limiting possibilities"
it is totally possible that some extension may wish to developerly modify the steps was not updating the total steps, sp this safeguard will break those extensions

in my opinion, fixing the thing that is actually causing issue is better than adding unnecessary safeguards that potentially cause more issues of its own

@w-e-w w-e-w marked this pull request as draft January 30, 2025 17:51
@Kittensx
Copy link
Author

Kittensx commented Feb 1, 2025

this change directly breaks Sampling method: DPM adaptive which is by design to have a non-deterministic amount of steps

lets suppose this doesn't break stuff like DPM adaptive, I still don't really see a reason to add the safeguard as this really is not a common issue, it's only happens if you're working with some advanced things and the other side of "safeguarding" is "limiting possibilities" it is totally possible that some extension may wish to developerly modify the steps was not updating the total steps, sp this safeguard will break those extensions

in my opinion, fixing the thing that is actually causing issue is better than adding unnecessary safeguards that potentially cause more issues of its own

It's true that advanced things like custom schedulers may modify steps without actually using that terminology "self.step" or self.steps" by manipulating the sigmas_karras in a loop without knowingly doing so. If DPM adaptive works by manipulating the sigmas instead of manipulating the steps, which would make the most sense, then it wouldn't effect it. For example, in my scheduler, I create a loop and inside it I manipulate things,

` sigmas_karras = get_sigmas_karras(n=n, sigma_min=sigma_min, sigma_max=sigma_max, device=device)
...

 target_length = min(len(sigmas_karras), len(sigmas_exponential))  
sigmas_karras = sigmas_karras[:target_length]
sigmas_exponential = sigmas_exponential[:target_length]

...

for i in range(len(sigmas_karras)): .....which in effect is equal to the number of steps the user wants....
nowhere in the code above (used in my scheduler) do i reference steps...the original code worked until I created a class and then it needed n, and then I needed to find how to reference n in your code, and I figured it out but at first setting n to an arbitrary number to make the code work when using the scheduler, it would force exactly those numbers of steps...versus what the user specified...

Hence....safety.

You don't want to ever introduce the ability for new code to contradict what the user specified in the gui. I would think.

You mentioned DPM adaptive not working when introducing this code. I would argue in that case, that DPM adaptive could be structured in a way which doesn't interfere with self.step or self.steps.

If code is used to introduce a new sampler or scheduler which forces a particular number of steps, let's say 30 steps, and the user specifies 50 steps, then you'll get a bad image, because the noise generated from 50 steps is different than 30 steps and the program thinks it has time to get there...but if the program says that n=30 insteaad of the 50 that the person requested then they could think the program is bad....or if they set the number of steps to 1 or basically anything less than 20 for most things.... then no matter what the person puts 35, 40, 45 etc. you'll only get 1 step completed because you let a program modify the steps directly in code.

A safeguard stops such programs in their tracks. You could even add a helpful message like:
if self.step >= self.total_steps: print(f"Warning: Step {self.steps} exceeds requested steps ({self.total_steps}). Halting further processing.") raise sd_samplers_common.InterruptedException

And when you say it breaks DPM Adaptive, are you referencing this code: https://github.com/CompVis/stable-diffusion/blob/main/ldm/models/diffusion/ddpm.py

Where they do modify N internally - which wouldn't effect this safeguard, since self.step and self.steps changes via code. This safeguard would protect in the instance given above as an example.

@w-e-w
Copy link
Collaborator

w-e-w commented Feb 2, 2025

If you actually test Sampling method: DPM adaptive you will quickly see the issue
left current (expeted), right your PR

image

"DPM adaptive ignores the user setp input"

if your PR is going to be merged, then the things it breaks needs to be fixed in your PR

As demonstrated you with DPM adaptive, yours change may have the potential to break existing things
so I can only assume that it may other things as well, things that we are not even aware of
such as extensions maintained by others developers, assuming that they are still maintaining it

when possible breaking change should be avoided
unless there's a very good reason to introduce to do so


You don't want to ever introduce the ability for new code to contradict what the user specified in the gui. I would think.

if you're saying this because your other PR was closed
I believe I've stated very clearly, your PR was closed because it is "dead code" https://en.wikipedia.org/wiki/Dead_code
adding instructions on how people to use it dose not make it a valid PR

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.

2 participants