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

force Euler method for the "img2img alternative test" script #1149

Closed
wants to merge 7 commits into from

Conversation

48design
Copy link
Collaborator

When the "img2img alternative test" script is selected, the UI will be locked to the Euler method, and return to the previous state when changing the script to something else. This should prevent confusion among users and hopefully reduce issues raised regarding the feature.

@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

There's an option here for scripts to advertise limits and locking of the samplers, And have this be useful for all scripts not just this specific one.

@48design
Copy link
Collaborator Author

There's an option here for scripts to advertise limits and locking of the samplers

Can you point me to the right file/line? I also didn't see anything about disabling elements in the Gradio docs.

And have this be useful for all scripts not just this specific one.

Is there an overview about which scripts are bound to which methods? Then I could implement it for all of them in one go.

@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

Sorry nothing official, I mean to say we have the option here, something like script.py looking at a new method on the script superclass that returns a list of compatible samplers (or an empty list for universal compatibility), that scripts.py sets on one of the components it creates that drives this.

@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

Also, surely the auto-numbered ids can change?

@Connum
Copy link
Contributor

Connum commented Sep 27, 2022

Also, surely the auto-numbered ids can change?

Absolutely, forgot to add elem_ids and use those, intended to do so before committing.

@Connum
Copy link
Contributor

Connum commented Sep 27, 2022

Sorry nothing official, I mean to say we have the option here, something like script.py looking at a new method on the script superclass that returns a list of compatible samplers (or an empty list for universal compatibility), that scripts.py sets on one of the components it creates that drives this.

Oh, right, got you! I'm not that fit in python, so I mainly focus on doing things on the frontend side. But I agree that this is a good idea!

@48design 48design marked this pull request as draft September 27, 2022 09:41
@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

We can output an empty element at the end of the script with a class "customScriptAttrs" data attribute with a comma delimited list of the compatible samplers?

image

If that attribute is empty take it as universal compatibility, and on the custom script it just needs:

    def compatible_samplers(self):
        return ['Euler']

Added to any that want to start filtering.

@d8ahazard
Copy link
Collaborator

d8ahazard commented Sep 27, 2022

If you're doing this, then you should have the sampling steps match the script sampling steps as well, because it's pointless otherwise. (Trying with mis-matched steps, I mean).

@d8ahazard
Copy link
Collaborator

Oh, also, when changing the "enabled" RealESRGAN models in settings, it would be ideal if the UI could update the list of scalers on the fly in the UI so a restart isn't required.

@Connum
Copy link
Contributor

Connum commented Sep 27, 2022

Oh, also, when changing the "enabled" RealESRGAN models in settings, it would be ideal if the UI could update the list of scalers on the fly in the UI so a restart isn't required.

I think that's something for a separate PR.

But the linking of CFG Scale and Decode CFG Scale definitely makes sense and I will include it.

@48design
Copy link
Collaborator Author

Thanks @dfaker for the proposal! Instead of compatible_samplers, I named it ui_restraints, and it returns an object with a methods property that holds an array of allowed methods. That way, we are more flexible for other future restraints (like linking UI elements as @d8ahazard mentioned).

Regarding linking the CFG Scale and Decode CFG Scale: Why does the script offer a range slider for the Decode Scale in the first place? Couldn't it just use the CFG Scale then and we wouldn't need to link the UI elements?

@48design
Copy link
Collaborator Author

Couldn't it just use the CFG Scale then and we wouldn't need to link the UI elements?

Or extending on that question: Can we really be sure that no one will ever want to use different values for CFG scale and Decode CFG Scale?

@48design 48design marked this pull request as ready for review September 27, 2022 17:32
@48design
Copy link
Collaborator Author

Due to said concerns, I decided against implementing the linking between default and script UI elements. So this includes the sampling method restriction only, but it's usable in other scripts as well and the functionality is now better extensible for future additional restraints.

@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

@48design okay the back-end section is committed into your branch now, we can always extend it with additional data attributes if required.

@48design
Copy link
Collaborator Author

Wait - now we have unnecessary code in there. I didn't know you had already prepared anything that you wanted to merge into this PR. So my code from 19 minutes ago was already working as intended.

@dfaker
Copy link
Collaborator

dfaker commented Sep 27, 2022

Yes sorry! reverted now!

@AUTOMATIC1111
Copy link
Owner

Shouldn't it say ui_restriction rather than ui_restraint?

My problem with all this is it is a lot of code and a change in public interface (scripts) to add a tiny tiny feature. You can just change the sampler in the script itself if it's the wrong one, and possibly produce a warning, either to console, or to the text output HTML.

@48design
Copy link
Collaborator Author

If a script doesn't provide the new def, nothing will change. I had it implemented frontend-only at first, but I thought it was a good idea to have it available to all scripts.
It happens to me all the time that I forget to change the method and end up wondering why it's not working until I notice. I wanted to make this more end-user friendly.

@48design
Copy link
Collaborator Author

Shouldn't it say ui_restriction rather than ui_restraint?

I'm not a native speaker, so if there's a difference I didn't intend anything by it and would be totally fine with renaming it! 😉

@JustMaier
Copy link
Contributor

JustMaier commented Sep 29, 2022

I agree with @48design, giving scripts the ability to restrict or lock settings in the UI is going to be increasingly important as the community around scripting continues to grow and scripts get more complex. I totally get your concern @AUTOMATIC1111 about changing the public interface, so this probably deserves a larger conversation about the future of scripts.

Maybe another potential option is to extend/modify the return parameters of the Script.ui function to include additional options like the restrictions dict in this code and others in the future. For example, it could return a dictionary with predefined props such as components (the old array that came out of this function), restrictions (the dict proposed here), and other UI centric options/props in the future. And to prevent breaking, this chunk could be updated to create the dictionary if it got an array back from the Script.ui function

@48design
Copy link
Collaborator Author

48design commented Sep 29, 2022

Thanks for having my back @JustMaier, but wouldn't your proposal be more likely to break anything or at least be more complex to prevent breaking anything instead of the additional def?

Really most changes in this PR are mostly about getting the config to the frontend, nothing too complex happening here. The rest is taken care of by the frontend Javascript, as the dictionary can easily be extended in the future by each script without touching script handling in python, just by adding more functionality to the JS.

@48design 48design closed this Sep 29, 2022
@48design 48design reopened this Sep 29, 2022
@JustMaier
Copy link
Contributor

JustMaier commented Sep 30, 2022

My proposal would handle the breaking change gracefully while also future proofing the ui function to some degree by switching it to an output type (dict or class) that is more easily extended without modifying the interface. Here's what that would look like:

for script in self.scripts:
    script.args_from = len(inputs)
    script.args_to = len(inputs)

    ui_options = wrap_call(script.ui, script.filename, "ui", is_img2img)

    if type(ui_options) is list:
        ui_options = {"controls": ui_options}

    if "controls" in ui_options and ui_options["controls"] is not None:
        for control in ui_options["controls"]:
            control.custom_script_source = os.path.basename(script.filename)
            control.visible = False

        inputs += controls
        script.args_to = len(inputs)

    if "restrictions" in ui_options and ui_options["restrictions"] is not None:
        # TODO: restrictions handling
        pass

    if "some_future_thing" in ui_options and ui_options["some_future_thing"] is not None:
        # TODO: some_future_thing handling
        pass

I'm not suggesting that we move the restrictions from the front-end scripts, that all should stay there. I just thought that since you're the first I've seen propose an addition to the scripts spec and @AUTOMATIC1111 was questioning the change to the spec that it made sense to suggest approaching the change in a potentially more future friendly way by keeping all UI related things contained in a single function.

Either way, I'd love to see the ui_restrictions functionality added. I've done some work on the seed travel script and it could use the ui_restrictions to restrict the batch size and count to 1.

ps. forgive any inefficient python code, most of my dev time has been in C# and JS.

@48design
Copy link
Collaborator Author

Yeah, having it all under ui would probably make sense! My second paragraph regarding the volume of code changes and JS was targeted towards @AUTOMATIC1111

@48design
Copy link
Collaborator Author

Thinking about it, if @AUTOMATIC1111 absolutely doesn't want it in the python side, but there's obviously demand for it, I could cook up a frontend-only solution, a JS script in core that makes it possible to hook into and define the UI restrictions for a script. So a script could distribute a js file alongside its python file that will go into the javascript directory. Let's say we make up a making convention to name those [whatever]. script.js or [whatever].user.js and add that pattern to .gitignore. Those scripts could then add their config to a global object, either like in the dictionary in this PR, or optionally even as a custom function.

@AUTOMATIC1111
Copy link
Owner

I let this PR cook a bit for people to discuss how they want this to work. If going the javascript route, you already can have a plugin put a javascript file into the javascript directory. If we want to add an interface for python scripts to to disable parts of UI, I want the interface to be general, so that the programmer can disable anything he wants (possiblty using the paths that go into querySelectorAll).

@cmp-nct
Copy link

cmp-nct commented Oct 3, 2022

Just as a general thought: More power to the scripts

Imho scripts should be able to take quite full control of settings and SD behavior, the scripts are kind of the heart of your webui.
From outpainting to noise search to various prompt and batch modes.
As a script is so much easier to get into than the whole code I am quite sure this will continue to expand, the more possibilities the scripts have the more likely we'll see some advanced ideas no one is thinking about right now.

In this example a javascript trick to switch to euler & disable sampling options then re-enable them in case of deselection of the script would do it too.

@Ehplodor
Copy link

I believe PR #2375, and the associated commit e548fc4 should close the present one.

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.

8 participants