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

Fix postprocessing_enable_in_main_ui ScriptPostprocessing elem_id #16373

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

w-e-w
Copy link
Collaborator

@w-e-w w-e-w commented Aug 12, 2024

Description

Fix postprocessing_enable_in_main_ui ScriptPostprocessing duplicate elem_id

ScriptPostprocessing (extras tab) modules such as Upscale can be use as a processing add on in txt2img and img2img pipeline with the use of ScriptPostprocessingForMainUI / opts.postprocessing_enable_in_main_ui

this causes an issue, due to the way ScriptPostprocessingForMainUI creates the UI for the ScriptPostprocessing UI is the same as Extra tab, if an element has a fixed elem_id configured the element IDs will be duplicated

this has always been an issue since the beginning but this has become a serious issue due to the switch to InputAccordion in ScriptPostprocessing
as InputAccordion relies on the id being unique (if manually set)

the issue can be reproduced by

  1. Setting > Postprocessing > Enable postprocessing operations in txt2img and img2img tabs add Upscale
  2. try to use Upscale on extras tab, you should find that the InputAccordion is not responsive
  3. the InputAccordion on txt2img tab actually controls the InputAccordion on extras tab

assuming that txt2img is created before extras tab


Proposed solution

similer to scripts.Script elem_id should be generated with the elem_id() helper function
tab_name will be append to the end of the elem_id
on extras tab tab_name is blank
but before ScriptPostprocessingForMainUI creates the UI, tab_name of the script should be set to _txt2img or _img2img

issue with this approach any extension that wishes to make their ScriptPostprocessing should also be update to use elem_id() to create there elem_id
unfortunately I don't think there is a better solution that can be applied in automatic efficiently issue globally without change to extensions
reason being elem_id are mostly reference inside JavaScript, and there's really no unified method of modifying what function do they target within python

also additional inconvenience extension that wishes to use this function if they wish to maintain compatibility with old versions of web UI they need to test if this function exists


there are two variant of the create elem_id helper function elem_id() and func elem_id_suffix()
elem_id() is for new extentions, they can use this to creat there elem_id, this create the ID with extras_ and script name prefix followed by the input string and tab_name suffix

func elem_id_suffix() on the other hand only adds the suffix without modifying the rest of the ID
the reason is that it is possible that some other things extensions are already utilizing set ID to Target the specific elements and it would be bad to multiply the ID

Note invalid characters are still removed in both functions

Checklist:

@light-and-ray
Copy link
Contributor

Maybe there is a universal way to allow coping of Input According, i.e. allow existence of InputAccordions with the same id. I remember I copied soft inpaint ui ik my extension, and a was needed to recreate InputAccordion element. Even callbacks on after/before component couldn't help with this, because it copies elem_id in constructor moment somewhere

I also tried to add on_before_component in init of input accordion, but it was a bad idea because it's not possible to pass arguments by reference to edit elem_id

Maybe it's possible to avoid coping elem_id to somewhere, and use lambdas, but I don't remember global input accordions ids is in python or JavaScript

Btw if do something, it goes into a new pr, because it helps with on_after_component, not with postprocessing in ui. But also if this is implemented, we can avoid using elem_id_suffix() and add this suffix in ScriptPostprocessingForMainUI using on_after_component callback

@w-e-w
Copy link
Collaborator Author

w-e-w commented Aug 13, 2024

Maybe there is a universal way to allow coping of Input According, i.e. allow existence of InputAccordions with the same id

yes I consider that path, and yes if we only want to fix the issue for only InputAccordion then yes there are ways to get around it
but the issue is we don't know what extensions are doing
we cannot make assumption that input accordion is the only thing that relies on the elem ID

if we want to fix it for InputAccordion only then the simplest method modify the initialization of InputAccordion to guarantee that the ID is unique, the downside is that this would have side effect of changing the accordion ID for any duplicate elements

the whole reason behind a fixed elem_id is that it porvides a unambiguous way to "targeting" the element using JavaScrip / CSS and sometimes in python as well

but that code the use the elem id can be anywhere,

  • it could be in Python a part of the extension, like someone made a custom element similar to InputAccordion
  • it could be JavaScript some additional function that is being implemented
  • it could be in CSS used to apply certain styles to an element
  • it could be a custom browser userscript that someone has written themselves

there's no way we can know what code would be using the elem id for their own reason, and so there's no way we can retroactively modify that code to the correct element

and so the only way we can truly fix this is at the source, "the ID should be unique in the first place"
and any code that wishes to reference this element via it's ID, needs to know that the element can have different variants with different suffix

and this is why elem_id() and func elem_id_suffix() is realistically the only method even though it's tedious


Btw if do something, it goes into a new pr, because it helps with on_after_component, not with postprocessing in ui. But also if this is implemented, we can avoid using elem_id_suffix() and add this suffix in ScriptPostprocessingForMainUI using on_after_component callback

I don't quite follow you

@light-and-ray
Copy link
Contributor

the whole reason behind a fixed elem_id

I don't mean dynamic element ids, I suggest relay on ids which gradio components have after ui created. Not on what they have in constructor. E.g. allow

x = InputAccordion()
x.elem_id = "my_elem_id"

I don't quite follow you

For example this in my replacer's code. I don't suggest exactly the same, I mean similar logic

needWatchControlNetUI = False
controlNetAccordion = None

def watchControlNetUI(component, **kwargs):
    global needWatchControlNetUI, controlNetAccordion
    if not needWatchControlNetUI:
        return

    elem_id = kwargs.get('elem_id', None)
    if elem_id is None:
        return

    if elem_id == 'controlnet':
        controlNetAccordion = component
        return

    if 'img2img' in elem_id:
        component.elem_id = elem_id.replace('img2img', 'replacer')

...

script_callbacks.on_after_component(replacer_extensions.controlnet.watchControlNetUI)

I can enable this code by setting up replacer_extensions.controlnet.needWatchControlNetUI = True. But the better way is to turn it into class with def __enter__(self): and def __exit__(self, *args):.

@light-and-ray
Copy link
Contributor

light-and-ray commented Aug 13, 2024

Not on what they have in constructor. E.g. allow

I mean delay this code inside InputAccordion.__init__:

self.accordion_id = kwargs.get('elem_id')
if self.accordion_id is None:
    self.accordion_id = f"input-accordion-{InputAccordion.global_index}"
    InputAccordion.global_index += 1

kwargs_checkbox = {
    **kwargs,
    "elem_id": f"{self.accordion_id}-checkbox",
    "visible": False,
}
super().__init__(value, **kwargs_checkbox)

self.change(fn=None, _js='function(checked){ inputAccordionChecked("' + self.accordion_id + '", checked); }', inputs=[self])

kwargs_accordion = {
    **kwargs,
    "elem_id": self.accordion_id,
    "label": kwargs.get('label', 'Accordion'),
    "elem_classes": ['input-accordion'],
    "open": value,
}
self.accordion = gr.Accordion(**kwargs_accordion)

On the moment after ui created, but the app was not started

@w-e-w
Copy link
Collaborator Author

w-e-w commented Aug 13, 2024

yes I have also considered delaying the init of InputAccordion , in face that was one of my initial solutions, but as I have mentioned solving the issue for InputAccordion it's basically just postponing the root issue

also if you want to avoid this issue you can just not set elem_id and have it all to generate and it will work
this issue exists only for elements that have been specified their elem_id people want a specific ids so that they can use it for something unknown

this issue has existed ever since postprocessing_enable_in_main_ui is introduced but it wasn't causing major issues until switch to InputAccordion

maybe having additional code for fixing it for InputAccordion, as it can improve compatibility of some extensions without change, but since it's not fixing the root issue, I belive the InputAccordion should not introduce extra complexity to the cloud base


For example this in my replacer's code. I don't suggest exactly the same, I mean similar logic
......

if an element ID is set replace it with something unique after it's been created
yes that I have also considered and that is actually my first thing I implemented
in order for this to actually work anything that relies on elem_id will have to be delayed until a later stage so that the replacement can be taken to affect, basically the exact same thing that you just mentioned about delaying init of InputAccordion
this basically reduce this into a InputAccordion specific fix, and not a once for all fix

so the next time you run into duplicate ID issues, something similar to my current PR would need to be applied


if you want additional fixes that makes duplicate elem_id InputAccordion compatible without modification on extension side, good I'm also considering that
but if a "duplicate elem_id InputAccordion compatible" solution is implemented that should be in different PR as an additional supplemented fix,
and is not a reason not to fix the root cause, because eventually you will run into issues that can only be fixed by fixing the root cause

@light-and-ray
Copy link
Contributor

So you think elem_id_suffix() is still important because we need to avoid duplicating of ids for js, css etc? And my suggested solution is not applicable for this problem in a proper way. Do I understand correctly?

@w-e-w
Copy link
Collaborator Author

w-e-w commented Aug 13, 2024

So you think elem_id_suffix() is still important because we need to avoid duplicating of ids for js, css etc? And my suggested solution is not applicable for this problem in a proper way. Do I understand correctly?

that's basically it that's the reason why people want elem ids is to serve as an unique identifier for the element from "anywhere", they may have different use case we simply cannot account

there's nothing wrong with makeing InputAccordion more compatible, it is just it's not solving the root issue, which is what does PR is trying to address
InputAccordion leads to the discovery of this entire issue, and it is arguably the most breaking issue related to this, but it is only a subset of the problem not the whole

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