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

ExtraNetworks: Performance Updates and Improvements #15530

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

Conversation

Sj-Si
Copy link
Contributor

@Sj-Si Sj-Si commented Apr 15, 2024

⚠️ THE FOLLOWING COMMENT IS THE MOST UP-TO-DATE STATUS OF THIS PR. START THERE: ⚠️

#15530 (comment)

⚠️ DESCRIPTION BELOW THIS POINT IS OUT OF DATE ⚠️

Description

This PR is a complete overhaul of the code that I previously wrote for the ExtraNetworksTreeView PR (#14588) in response to the negative feedback such as in #15164 and various issues that have been posted.

The goal of this PR is to improve performance and reduce resource use across ExtraNetworks tabs.

I have tested these changes with up to 100,000 models at a time. With this many models, the initial load is slow but as soon as the server fully generates the dataset it performs very well. Before, my browser would just crash if I even tried to load the page with more than 50k models. The real limiting factor here is the server (though it takes quite a bit to reach the limit). Right now, ui_extra_networks.py is generating the HTML for all of the files it discovers in any of the models directories. It then waits for requests from the client and sends only the requested items HTML back to the client. This way the client doesn't have massive amounts of DOM elements constantly loaded. So as long as the device running the server can load the generated HTML for every single item in the models folders then it should be fine. I don't see this being an issue until some psycho comes along with 10 million models but I don't really think we should be pandering to such a niche audience. Even then, I think other things will crash before this new code does.

Sorry ahead of time... this is a big one.

The following has changed:

  • Implemented a modified version of Clusterize.js to allow for an asynchronous data loader.
    • This allows us to offload generating and storing HTML strings to the server instead of the client.
    • HTML strings for tree/card elements are generated in the python server and only fetched by the client via JavaScript when they are needed. Fetching is done in smaller chunks to reduce overhead.
    • Set up a pseudo paging system to use as a cache for fetched HTML elements. This way we can reduce the number of times we fetch data from the server.
  • Reduced DOM complexity by minimizing the amount of nested elements in the tree/cards views.
  • Created all new SVG icons for the ExtraNetworks tab controls.
    • Accidentally went on a tangent and ended up learning how SVGs work.
    • I designed all of these SVGs so we don't have to worry about licensing crap.
      • I'm not a graphics artist in any way so feel free to change these designs as needed or draw up a sample and I'll try to update it.
  • Created a utils.js file and added a lot of helpful utility functions that could probably be used elsewhere if needed.
  • Tab now allows use of both the tree and directory views at same time.
    • Selecting a filter button in one view selects the corresponding button in the other view.
  • Fixed various bugs that went undiscovered in my previous implementation.
  • Added a custom event that fires whenever a resizeHandle.js handle is double clicked.
    • This allowed me to implement an auto-resize function on the tree view whenever the resize handle is double clicked. This automatically sets the width of the tree view to the width of the widest row in the list.
  • Added an option to only show directories in the tree view per some user's request.
  • Updated CSS to use variables instead of hardcoded values.
    • This way these changes should be way more compatible with custom themes.

UI Changes

Below is an example of what the new format looks like:

image

Controls

image

The controls have been grouped with a header to hopefully make their intent more obvious.

Directory View

image

Directory view buttons have been styled a bit better and now the selected directory will be highlighted.

Tree View

image

Slightly modified the appearance of the tree view and simplified the code that generates it. Also directories in the tree view are no longer expanded when the item itself is clicked. Now the user must click the chevron on a directory in order to expand it.

Checklist:

  • I have read contributing wiki page
  • I have performed a self-review of my own code
  • My code follows the style guidelines
  • My code passes tests

    ⚠️ Can't run tests with my environment however the tests in CI did pass.

Additional Notes

Someone suggested that we use emojis for the controls in the extra networks tabs. Personally I hate emojis and they look disgusting. On top of that, I couldn't find any that really fit the purpose of each of the controls. If someone has a theme (like lobo or whatever) then they can handle replacing these SVGs if they want. The SVGs are all easy to query so they can handle a find/replace if they want.

One thing that I couldn't figure out is how to manually trigger the gradio loading icon within the extra networks pane (see below):

image

This loading page appears whenever the page is reloaded or refreshed but I would have liked to have been able to manually trigger it to show up and stay there until the data is fully loaded but I couldn't figure out where this loading pane was even coming from. I'd really like to be able to put one in the Tree View and one in the Cards View. Then I would trigger each manually whenever I need to load more data.

Also at some point someone requested that clicking one of the directory view buttons to filter cards should only show direct children of that directory and not any of the nested directory content. What was the consensus on this? Should that be the behavior or do we prefer to show all nested items? The current behavior is the same as it always has been and it is a pretty simple implementation where clicking a button simply adds that path to the search box and updates the filter based on the search box text. Changing the behavior of clicking directory buttons would definitely require a bit of redesign and I think this change has already had enough scope creep as is.

Needs Testing

I would like to have this feature undergo a bit more testing than the last time in order to avoid having so many people get upset at the same time. I'd really appreciate feedback for this PR so that we can catch as many problems as possible before merging.

I have really only tested in the following environment:

  • Server running on WSL Ubuntu
  • Client connecting via Chrome on Windows 10 (localhost:7860)

Sj-Si added 30 commits March 13, 2024 17:11
…tml. maybe should only update the data json. also maybe look into sharing data json between img2img and txt2img to reduce size of dom.
@w-e-w w-e-w force-pushed the extra-networks-performance-updates branch from 153a22f to 8bf4548 Compare July 23, 2024 17:46
@Sj-Si
Copy link
Contributor Author

Sj-Si commented Aug 7, 2024

@w-e-w Hey I'm sorry for goin AWOL for a bit. By all means go ahead and make improvements to this PR.

I'm a bit tied up at the moment so I can't really dedicate any time to this project until I get things sorted irl first.

Also your understanding of how the model cards are loaded (loaded in chunks) is correct.

@w-e-w
Copy link
Collaborator

w-e-w commented Aug 8, 2024

sorry for goin AWOL for a bit
everything is fine and you're totally cool, no one's getting paid no one's getting horses everything is voluntary including you and me, anyone can be "AWOL" (LOL I had to search up what that means)


By all means go ahead and make improvements to this PR

Thanks


I don't know if AUTO has reviewed this or not
I think AUTO he is also currently busy with other things recently so I'm guessing that's why he's not able to review this yet

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

seems to have found another issue
Default order field for Extra Networks cards
Default order for Extra Networks card dose not seems to work
the icon in the ui does point down by default when set to descending
but the order of the cards is still in ascending order

same for the field as well, the first time that it displays the cars it is not sorted only until I update the icons does it get sorted
but if it's already selected it cannot be selected again
so if I default is Switched by name then I need to switch away then switch back to names in order to have a sort by names

if I wanted to show in descending order I actually have to click it twice
once for the icon to change to ascending order
a second time for it to actually apply descending order


the following is more to the realm of future improvements as opposed to issues


some thoughts about sorting categories

is it useful to have both creat time and modification time?
unless I'm mistaken this is using the time of the actual model file, the thing is for majority of the users they will never modify the model itself
in a while the user made copy or move the files and depends on the method use the times may get reseted or changed
maybe instead of two creat time and modification it should just be time
Maybe some use cases modification I will better While others creation time will be betterbut I think it one categories and in settings will be more useful
or maybe combine both times and choose between the newer or older of the time


on the other hand I think the actual useful time related sorting method would be some sort of usage time
it is shoub be possible for us to create a database on how many times a model is used or how many times a model is clicked

and it might be possible to also create a favorites list type system in order for user to choose the specific models that they use most often and have it listed in front

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

personally I found this a bit odd
is there a particular reason for the design choice of making the controls floating in the middle as opposed to on right side as before in previous version?
image

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

I'm working on the assumption that this is not intentional
as the cause of this is due to ther beeing two div of txt2img_extra_network_controls_div but one is empty for some reason
image

this also has the added effect of moving the controls the right side
as it was previously blocked by a invisible duplicate
 element
@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

the create_ui() is is actually called more than once to create txt2img and img2img to create tab separately
since you've implemented it so that extraNetworksSetup() will call boath extraNetworksSetupTab('txt2img') and extraNetworksSetupTab('img2img')
you inadvertently made it so that it is's called twice and creating a second control that has nothing in it
image
image

I fix it so that now extraNetworksSetup() takes in a extra arg tabname -> extraNetworksSetup(tabname)
and made it so python / gradio create_ui() register the js with the tabname

result screenshot
image

since the invisible div is removed the object is now pushed to the right

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

the order of the card icons got swapped somehow

Original this PR before commit after new commit
image image image

move template from local var to self.button_row_tpl
@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

found a potential issue but maybe not that serious and can be fixed later if it's really a problem

basically it takes time for the UI to scan for available models
and it is possible for the web page to request for the cards before the model has finished scanning
if this happens the user will be presented with an incomplete list of the models

this can happens if model scanning is taking unusually long

in short basically on an issue takes around~ 50 seconds for me to load the model without #16556
and enabled browser auto launch so the page opens before the models have fully been scanned
the result is I've been presented with only four out of all my TI's
I will have to reaload the model (waiting another 50 seconds) for it to show all the cards


they also seems to be a timeout for the page to load the cards, before it shows "Nothing here"
in the event that someone has a ridiculously large amount of cards would it be possible for
or has poor internet connection, could it be possible for the cards to always failed to load?



I use Chrome's built-in throttling feature to simulate bad connection (3G profile), the pages loades but the images seems to have failed
that cards have loaded successfully but it seems not some image are dropped, not too sure if there's much we can do about this
one thing I can think of is maybe we can Implement some sort of downscaling feature on server side so theres less data that needs to be sent

but I think that is out of scope for this PR as the pr is already too big

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 1, 2024

  • fix issue with cards and trees not working special characters within prompts

special characters such as " \ \n

  • fix embeddings not able to paste into negative prompts

the behavior that user can decide but it will not embedding is pasted into prompts or negative pump was broken

  • fix removal of negative prompts

() was always left behind when prompts is removed form negative prompts

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 1, 2024

found a strange issue regarding clicking on extra networks cards

from what I can tell when I clicking on the left 1/3 of the a card
the pasting of the network to the prompt box does not work

2024-11-01.15_41_43_689.chrome.mp4

this issue seems to be only on chromium based browsers but not Firefox

tested on Chrome 130.0.6723.70, Brave 130.06723.91, Edge 130.0.2849.56, Firefox 132.0,
also did a quick test on Android on Chrome same behavior, and again Firefox on android seems to be fine

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.

Current known issues

  • Clicking on cards seems to be 1/3 broken on chromium based browsers
    see comment

  • Default order field and Default order after page loads
    user will have to manually click on the category to refresh it
    see comment

@w-e-w
Copy link
Collaborator

w-e-w commented Nov 1, 2024

the extra networks will controls will disappear
if you switch to another tab and select an extra networks tab

  1. on txt2img select a extra networks tab (controls will show)
  2. switch to another tab and switch back the controls will still be shown
  3. switch img2img and select any extra networks tab
  4. switch back to txt2img tab and you controls are gone until switching to another extra networks tab

video

2024-11-02.01_38_06_551.chrome.mp4

there seems to be some logic that hides all controls when a extra networks tab is selected even those that are not of the current tab (txt2img / img2img tab)

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.