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

Fix for API running unwanted alwayson scripts

Open Vespinian opened this issue 2 years ago • 10 comments

Describe what this pull request is trying to achieve.

To avoid having scripts that don't handle None argument throw errors and scripts running when the requester doesn't want them to, I changed how the ScriptRunner is handled for api requests. Now it makes a copy of the global ScriptRunner it wants to run and strips this copy of unrequested scripts. This should prevent unwanted scripts from running but it means you need to include all script that you want to run, even if they don't take any args.

For scripts that don't take args, an empty dict can be passed as their value instead of one with an args key like so:

"alwayson_scripts": {
		"Randomizer Keywords": {},
		"Additional networks for generating": {
			"args": [true, false, "LoRA", "<!!A lora model here!!>", 0.3, 0.3, "LoRA", "None", 1, 1, "LoRA", "None", 1, 1, "LoRA", "None", 1, 1, "LoRA", "None", 1, 1, "Refresh models"]
		}
	}

Additional notes and description of your changes

As a consequence of this change, if you look at the global ScriptRunners (*2img_scripts) while the processing is ongoing, they will not match what is in the processing class and so this should be avoided, instead you can look at the ScriptRunner that is passed to the processing class in scripts which will contain the ScriptRunner that has been copied and pruned.

closes #8666

Environment this was tested in

List the environment you have developed / tested this on. As per the contributing page, changes should be able to work on Windows out of the box.

  • OS: Windows
  • Browser: chrome
  • Graphics card: NVIDIA RTX 2080 8GB

Vespinian avatar Mar 16 '23 02:03 Vespinian

@ljleb This fix should not have any effect on controlnet but if you have a bit of time to spare could you please test it out? Just running the controlnet testcases with this PR would be great.

Vespinian avatar Mar 16 '23 03:03 Vespinian

One additional feature that needs to be implemented is to change the scripts args_from and args_to in case an extension takes a variable number of parameter. For example, in cn extension, API users could pass arbitrary numbers of controlnet units to the extension instead of having to pass exactly the max number of multi-cn configured in settings. Right now, as it is implemented, when cn is the only active script it will work, but if we ask for multiple alwayson scripts it will not work IIUC.

Let me know if this is out of scope for this PR, I'll open one in this case.

Edit: Controlnet tests pass:

----------------------------------------------------------------------
Ran 24 tests in 160.950s

OK

ljleb avatar Mar 16 '23 03:03 ljleb

Yes, it should be pretty simple to refit the scripts_args and indices, now that we are making a copy. Ideally, it would be another PR for this though, so we can get the fix for the current problem in and not accidentally introduce another one by removing limitations.

Vespinian avatar Mar 16 '23 04:03 Vespinian

So I've cooked up something that resizes the arg list and updates the indices. Not sure if I should just push it to this PR. Anyway, while doing so I noticed a side effect of copying the ScriptRunner. Code that relies on the global ScripRunners for information about other scripts can end up throwing errors, I just had that with Randomizer Keywords extension L252, simply changing the code to check the ScriptRunner in p.scripts fixed it, but that means there might be other things that break. Still scripts probably should check the runner that they are provided instead of the global, unless they have a really good reason to.

Vespinian avatar Mar 16 '23 06:03 Vespinian

Parked the mentioned future arg refit in this branch in case of misfortune. It's a 1 commit ahead branch from this repo but it should contain the fixes from this PR too. If you want to try it out, feel free.

Vespinian avatar Mar 17 '23 00:03 Vespinian

Wouldn't it be possible to obtain script's default values from its gradio components? This way there wouldn't be a need to change existing behavior and there wouldn't be a need to make copies.

AUTOMATIC1111 avatar Mar 25 '23 10:03 AUTOMATIC1111

After a bit of fiddling, I have some code for that, if that is what you would prefer @AUTOMATIC1111. I can revert these and commit that instead.

It just requires running all of the scripts' ui function to fill the None list in the init_script_args function and importing Gradio

        # None everywhere except position 0 to initialize script args
        script_args = [None]*last_arg_index

        # get default values
        with gr.Blocks(): # will throw errors calling ui function without this
            for script in script_runner.scripts:
                if script.ui(script.is_img2img):
                    ui_default_values = []
                    for elem in script.ui(script.is_img2img):
                        ui_default_values.append(elem.value)
                    script_args[script.args_from:script.args_to] = ui_default_values

Vespinian avatar Mar 25 '23 16:03 Vespinian

Yeah I think this would be best, but it only has to be ran once (per tab rather than once per request).

AUTOMATIC1111 avatar Mar 25 '23 17:03 AUTOMATIC1111

Then perhaps it would be better if this would be put in the initialize_scripts of the Scriptrunner class and have a self.default_script_args variable that I can just reuse for every request?

EDIT: Or in the Api class

Vespinian avatar Mar 25 '23 17:03 Vespinian

So I added 2 list default_script_arg_txt2img and default_script_arg_img2img in the API class. They get initialized on the first request made by their respective tabs. From then on the init_script_arg function just copies these defaults and applies the modification requested.

Vespinian avatar Mar 25 '23 18:03 Vespinian