stable-diffusion-webui icon indicating copy to clipboard operation
stable-diffusion-webui copied to clipboard

Hotfix: API cache cleanup

Open ramyma opened this issue 1 year ago • 9 comments

Description

After processing image generation via API p.close() wasn't being called, and accordingly the cache wasn't being cleared as discussed on https://github.com/Mikubill/sd-webui-controlnet/issues/1719

This PR calls p.close() after img2img and txt2img generations to do the necessary cleanup for API calls.

I hope we can merge this as a hot fix to master as well since it affects API integrations with Controlnet at the moment.

Checklist:

ramyma avatar Jul 02 '23 02:07 ramyma

please clean the code first then you have to start fixing

Aashray24092000 avatar Jul 03 '23 07:07 Aashray24092000

Are you using experimental_persistent_cond_cache? I'm not sure how this would be an issue otherwise..?

akx avatar Jul 03 '23 09:07 akx

Are you using experimental_persistent_cond_cache? I'm not sure how this would be an issue otherwise..?

I'm not using experimental_persistent_cond_cache, however, if you look at __init__: https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/master/modules/processing.py#L182

self.cached_uc = StableDiffusionProcessing.cached_uc
self.cached_c = StableDiffusionProcessing.cached_c

It's self referencing, so, without running close() after processing, the values won't be reset at: https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/master/modules/processing.py#L295C9-L297C63

if not opts.experimental_persistent_cond_cache:
            StableDiffusionProcessing.cached_c = [None, None]
            StableDiffusionProcessing.cached_uc = [None, None]

Before https://github.com/AUTOMATIC1111/stable-diffusion-webui/commit/7f2214aa2b9a88bc1631f3f1d164a75a3583c851#diff-1fe5b15d29310b37af0c4560da365990f4e6eee7b99bf3f041cfa18ebb11610fL179, both values where reset in the __init__ function explicitly:

self.cached_uc = [None, None]
self.cached_c = [None, None]

In short, without calling p.close() after processing an API call, the cleanup won't happen; even when experimental_persistent_cond_cache is not set.

ramyma avatar Jul 03 '23 11:07 ramyma

Ping @w-e-w, can you take a look since you authored the conds cache stuff? ☝️

akx avatar Jul 03 '23 12:07 akx

yes I think this is a bug in API apparently conds cacheis not cleare wheh using API because it's missing p.close in the flow effectively it's like having experimental_persistent_cond_cache always enabled so this PR will fix the issue

the question is now how do we make experimental_persistent_cond_cache compatibleis with controlnet

irrc before the addition of experimental_persistent_cond_cache p.close was redundant, so there was no issue with api not haveing p.close

when I make the change I didn't check if API had p.close, I assume that it had that like in the gradio pipeline

w-e-w avatar Jul 03 '23 12:07 w-e-w

apparently I found a bug /design flaw in GitHub projects If I add this PR to project (even to my personal) and change the title of the project entry, it will also change the title here no confirmation

w-e-w avatar Jul 03 '23 13:07 w-e-w

For a future refactor, we should probably use ps with contextlib.closing() for automagical cleanup :)

akx avatar Jul 03 '23 13:07 akx

At least I'll suggest

            p = StableDiffusionProcessingTxt2Img(sd_model=shared.sd_model, **args)
            try:
                p.scripts = script_runner
                p.outpath_grids = opts.outdir_txt2img_grids
                p.outpath_samples = opts.outdir_txt2img_samples

                shared.state.begin()
                if selectable_scripts is not None:
                    p.script_args = script_args
                    processed = scripts.scripts_txt2img.run(p, *p.script_args) # Need to pass args as list here
                else:
                    p.script_args = tuple(script_args) # Need to pass args as tuple here
                    processed = process_images(p)
                    shared.state.end()
            finally:
                p.close()

Instead of just p.close(), it should expect some failure can happen during operation (mostly OOM /CUDAError), then still try to close it at last.

Contextlib is more clean version for this.

aria1th avatar Jul 03 '23 16:07 aria1th

At least I'll suggest

            p = StableDiffusionProcessingTxt2Img(sd_model=shared.sd_model, **args)
            try:
                p.scripts = script_runner
                p.outpath_grids = opts.outdir_txt2img_grids
                p.outpath_samples = opts.outdir_txt2img_samples

                shared.state.begin()
                if selectable_scripts is not None:
                    p.script_args = script_args
                    processed = scripts.scripts_txt2img.run(p, *p.script_args) # Need to pass args as list here
                else:
                    p.script_args = tuple(script_args) # Need to pass args as tuple here
                    processed = process_images(p)
                    shared.state.end()
            finally:
                p.close()

Instead of just p.close(), it should expect some failure can happen during operation (mostly OOM /CUDAError), then still try to close it at last.

Contextlib is more clean version for this.

Good call, I pushed the changes.

ramyma avatar Jul 03 '23 17:07 ramyma

Can we merge this PR?

ramyma avatar Jul 07 '23 14:07 ramyma

Can we merge this PR?

AUTO has gone into hibernation

w-e-w avatar Jul 07 '23 14:07 w-e-w