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

Add 'Skip CFG during early sampling' to the XYZ_grid script #16281

Closed

Conversation

hoblin
Copy link
Contributor

@hoblin hoblin commented Jul 28, 2024

Description

Screenshots/videos:

image

Checklist:

@hoblin hoblin requested a review from AUTOMATIC1111 as a code owner July 28, 2024 14:08
@w-e-w w-e-w mentioned this pull request Jul 28, 2024
4 tasks
@w-e-w
Copy link
Collaborator

w-e-w commented Jul 28, 2024

thanks for the pr but sorry your implementation is way over complicated
all you need is literally one line of code see my PR

AxisOption("Skip Early CFG", float, apply_override('skip_early_cond')),

I added you as co-author for the commit

@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

all you need is literally one line of code see my PR

That's how i started. But if you'll take a look at the modules/sd_samplers_cfg_denoiser.py you'll find that it checks an option value rather than a passed parameter. So when you set a parameter in the script file it is not passed further until you actually read it's value in the modules/processing.py, pass it further to the denoiser, then use it there instead of opts.skip_early_cond

Just take a look at how s_min_uncond is implemented. It does pretty much the same as skip_early_cond

You could also add the print(f"Skip? {opts.skip_early_cond}") to the denoiser's forward function to check that.

@w-e-w
Copy link
Collaborator

w-e-w commented Jul 28, 2024

you'll find that it checks an option value

no
your using apply_field
I am using apply_override

apply_override overrides a shared.opt (aka setting)
it works as long as the value is used after

for k, v in p.override_settings.items():
opts.set(k, v, is_api=True, run_callbacks=False)
if k == 'sd_model_checkpoint':
sd_models.reload_model_weights()
if k == 'sd_vae':
sd_vae.reload_vae_weights()
sd_models.apply_token_merging(p.sd_model, p.get_token_merging_ratio())
# backwards compatibility, fix sampler and scheduler if invalid
sd_samplers.fix_p_invalid_sampler_and_scheduler(p)
with profiling.Profiler():
res = process_images_inner(p)

I even triple checked by verifying my PR your PR and manually adjusting the settings all produce the exact same output (pixel perfect)

AUTO also have a commit that made skip_early_cond work with override see 5429e4c

@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

your using apply_field
I am using apply_override

Aha, thanks, missed that detail. Then you're right, your way is much easier.
Could you please include a little fix into yours so I'll close this one? 5006b7f

@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

Closed in favor of #16282

@hoblin hoblin closed this Jul 28, 2024
@hoblin hoblin deleted the add-skip_early_cond-to-xyg_grid-script branch July 28, 2024 17:01
@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

@w-e-w BTW i would prefer an integer value to set precise step to activate CFG rather than a progress float. It might be much easier than dividing step number i want it to be activated by the total steps. Does it makes sense to propose such a change?

For example 1.5, 1.6, 1.7 will all activate CFG on step 4 so it might be quite confusing for users. It will be much easier to use the step number instead as we use for Prompt Editing feature

@w-e-w
Copy link
Collaborator

w-e-w commented Jul 28, 2024

Could you please include a little fix into yours so I'll close this one? 5006b7f

the current scale from [0, 1) is is not broken
if we did what you said and +1
all that dose is just shift the steps by 1 to (0, 1]
it doesn't improve functionality or fix and issues
the only thing it does is maybe make the calculation which is not the light important easier
but it also change the meaning of the value which is a breaking change

if you really think it's important make a PR but I won't support it
and I don't think others will


BTW i would prefer an integer value to set precise step to activate CFG rather than a progress float. It might be much easier than dividing step number i want it to be activated by the total steps. Does it makes sense to propose such a change?

if you think about it an absolute step doesn't make sense if you change the step size
the absolute step does not have real meaning, it only has meaning if you combined it with multiple steps along with the schedule you're using
using a float value representing the relative position in the schedule makes more sense

20240729-024330-750298 1144432688 98c6

depending on your sampler and scheduler your results may vary but you can see that the same Skip Early CFG generate similar outputs regardless of step size

@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

the only thing it does is maybe make the calculation which is not the light important easier

With the current state any non-zero value will make a first step skip (current_step 0 / total_steps is always 0). It's not logical but that's indeed a breaking change so probably not worth it.

the same Skip Early CFG generate similar outputs regardless of step size

Yup, makes sense. I'll make more tests and think it through. Thanks for feedback!

@w-e-w
Copy link
Collaborator

w-e-w commented Jul 28, 2024

With the current state any non-zero value will make a first step skip (current_step 0 / total_steps is always 0). It's not logical but that's indeed a breaking change so probably not worth it.

if you don't want any steps to be skipped set it to 0 it's disabled
if you want to use it I think it's safe to say you want the first step to be skipped so you were set it to a non-zero value

I don't see it really impact the usage unless you are constantly switching between LCM (4~ step) and other normal ones (20~ step) and want Skip Early CFG to be "automatically disabled" for LCM by setting it to a value <=~0.25

@w-e-w
Copy link
Collaborator

w-e-w commented Jul 28, 2024

tip
if you found yourself constantly adjust a setting value such as Skip Early CFG then you can add it to Quick Settings or Additional Options

@hoblin
Copy link
Contributor Author

hoblin commented Jul 28, 2024

I'm quite an experienced dude in SD but thanks anyway =)

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