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

reduce script extensions loading time using threadpoolexecutor #16545

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

wkpark
Copy link
Contributor

@wkpark wkpark commented Oct 12, 2024

Description

  • reduce script extensions loading time using ThreadPoolExecutor
    • at loading extension modules step, it could be parallelized.
  • add script_loading_max_thread "System" option. (set max thread =1 for normal loading)

Notes: Due to the fact that sys.path is required with Lock(), the effect of max_thread>2 is not quite large.

Results:

test condition:

  • with all builtin extensions
  • +controlnet + dynamic_prompt + supermerge + loractl + ...

without ThreadPoolExecutor() : script_loading_max_thread=1
~16.5 sec load scripts time.

image

with ThreadPoolExecutor(): script_loading_max_thread=4
~10 sec load scripts time.
image

Checklist:

Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for most script it's fine but similer to your other #16548, this will call scripts be loaded in undetermined order

some scripts are designed in a way that they are loaded in a specific order
otherwise it breaks

sometimes the order within an extension itself (IMO this means that the extension has multiple coded poorly) bout sometimes require other extensions to be loaded first

while this mainly affects the extensions that integrates with or dependent onother extensions, but those extensions do exist

also so even there is no conflicts within extensions
also
it is totally possible during loading of extension module
the extension decides to modify / patch parts of web UI
which is not thread safe

basically if the PR is meaged is that it will requires all extension loading to be rewritten with thread safe manner
and since this is not the case in the past, an extension has to declare that they are thread safe for us to load it in parallel

it is totally possible for something to be broken to the point prevents the UI from even launching
furthermore the behavior is undeterministic which basically makes debugging impossible
this feature is also enabled by default which is asking for more trouble

@w-e-w w-e-w marked this pull request as draft October 20, 2024 00:23
@wkpark
Copy link
Contributor Author

wkpark commented Oct 20, 2024

for most script it's fine but similer to your other #16548, this will call scripts be loaded in undetermined order

some scripts are designed in a way that they are loaded in a specific order otherwise it breaks

sometimes the order within an extension itself (IMO this means that the extension has multiple coded poorly) bout sometimes require other extensions to be loaded first

you mean, some extensions have execution code while loading time and that could cause issue?

yes. you are right, some extensions have execution code like as install.py do,
but webui already have hook mechanism and a well-designed extension should minimise the execution code on loading time, and use appropriate hooks as intended.

while this mainly affects the extensions that integrates with or dependent onother extensions, but those extensions do exist

also so even there is no conflicts within extensions also it is totally possible during loading of extension module the extension decides to modify / patch parts of web UI which is not thread safe

any possible monkey patches should use proper hooks If he want to guarantee the intended behaviour.

basically if the PR is meaged is that it will requires all extension loading to be rewritten with thread safe manner and since this is not the case in the past, an extension has to declare that they are thread safe for us to load it in parallel

it is totally possible for something to be broken to the point prevents the UI from even launching furthermore the behavior is undeterministic which basically makes debugging impossible this feature is also enabled by default which is asking for more trouble

we don't have to make perfect extensions, simply we can switch this feature on and off by setting script_loading_max_thread more than 1, and slowly migrate/fix/propose some badly designed extensions.

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 20, 2024

but webui already have hook mechanism and a well-designed extension should minimise the execution code on loading time, and use appropriate hooks as intended.
any possible monkey patches should use proper hooks If he want to guarantee the intended behaviour.

there are far more things that doesn't have a hook or callback then things that has
the controlnet extension uses a bunch of patches called "hijacks"

webui is by designed expecting extensions will apply their own patches when necessary
in case you don't know there's a on_script_unloaded callback, the documented use of this callback is to undo patches

we don't have to make perfect extensions, simply we can switch this feature on and off by setting script_loading_max_thread more than 1, and slowly migrate/fix/propose some badly designed extensions.

I just can't see including something that has potential of breaking webui in non-deterministic ways as a feature as a good idea

while this does indeed has potential of decreasing low time
but it has a bigger potential of generating some weird issue that is practically impossible to debug
leading to some unpaid developer spending 100x time that the future saves in a fruitless effort of trying to solve the issue

I see the potential troubles that this feature may cause outweigh its benefits


I forgot to mention some scripts are explicitly designed to be loaded after some other Scripts
you can't even specify you wish for extension to be loaded after another extension
and in general there are certain internal scripts that are must be loaded in a specific order
this is already built into the current system

your PR that added thread pool basically throws the order away
if you use a low worker account, you might be lucky to avoid the issues of what occur if the orders are messed up

but if you are work account is higher, you could easily experience issues like this
as there's certain callbacks that the order is based on the load order this includes the ui callbackl
image

you can see the extension order in the front end UI is randomized on every re-load
the test is done by setting worker to 8
with 7 extentions that it has a random time sleep 0~5 sec in the path of loading of the module to simulate a worst case scenario

if I recall correctly the load the order of an extension also affects the internal parameter order that is sent to the back end when processing a generation job, means that the parameter index changes

why does this internal order matter?

currently
let's say you have a web UI web page open, if you're done for some reason decide to restart the back end server
assuming that they are not major changes such as enabling or disabling extensions across restart
once the server is up and running the existing webpage will continue to work as if nothing has happened
this is because the argument that is sent to and from the client and server has not change

but if you parallelize script module load order, this is no longer the case
it is totally possible that on each restart the parameter order is different
and the web page will no longer function properly

it forces you to reload a web page if the server ever reboot
this is perfectly bad for multi-person usage as probably only the admin would know that the server is restarted

if you wish to simulate an issue similar to this, you could two webui webpage use one webpage to enable / disable an extention then restart the server, after the server has restarted you would use the old web page that's still thinks that the extension is disable / enable to sent a request the te server

if you're "lucky" things will break, if you're unlucky things might seem to work, in the worst case it has the potential of corrupt your settings as wrong parameters are essentially wrong values


I still stand my point
unless an extension is explicitly written to be and declared itself as thread safe, we should not run it in parallel
and I really don't see this happening

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 20, 2024

conclusion
the current implementation is objectively broken
has it has the potential of breaking even vanilla web UI and the situation issue only gets worse if extensions are mixed
and this is before considering extensions that patches functions

it requires much much more work to allow the modules to be loaded in parallel

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