-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Rollback extra networks tree view #15164
Rollback extra networks tree view #15164
Conversation
All changes are current up to 58f7410 • extensions-builtin/Lora/ui_extra_networks_lora.py Rollback to d4945f4 • html/extra-networks-card.html Rollback to 699108b • javascript/extraNetworks.js Rollback to a2f23f9 Apply 801461e..ecffe85 • modules/shared_options.py Rollback to 1ddb886 Apply 569dc19..e3fa46f Remove unnecessary options: - "extra_networks_tree_view_default_enabled" - "extra_networks_tree_view_default_width" - "extra_networks_card_description_is_html" • modules/ui_extra_networks.py Rollback to 320a217 Apply 321b2db • modules/ui_extra_networks_checkpoints.py Rollback to 337bc4a • modules/ui_extra_networks_hypernets.py Rollback to 74b80e7 • modules/ui_extra_networks_textual_inversion.py Rollback to 74b80e7 • modules/ui_extra_networks_user_metadata.py Rollback to 5d7d182 • style.css Rollback to 2f98a35 Apply 9f3ba38..1466dae
Sorry guys I guess I dropped the ball on this one. I am working on changes that will hopefully please everyone. But for now I think rolling it back will at least quell the storm a bit and give me time to work. I'll also have to figure out how the whole built-in extensions thing works so it may be a while. |
Maybe instead of deleting everything you should have added option to bring back old view?
It's impossible. You saw the code |
Many will be grateful to you if you implement this solution, while correcting all the shortcomings listed above and not creating new ones. |
You want this - you make a pr. I don't want. And I'm against of this kind of vandalistic pr |
possible related |
|
I had the same before the update |
May be related too |
But I still think vandalism is not a solution |
Neither do I however it was because of this change that some users are practically unable to function at this point. So either we do this or these people have to wait for an entire development cycle before they can use the app as they did before. When I originally tested my code, I was testing with a couple hundred safetensors files not realizing that people out there have thousands of these things for some reason. I'm still working on the laundry list of bugs that people brought to my attention so I don't know how long it will be before I can even put in a PR with the revised code. I wish more people used the |
How is this vandalism? The PR just looks like a revert - which isn't vandalism, and the OPs description of the reason why is completely reasonable. You may disagree with his assertion but that's a separate issue. I personally use an extension to get rid of the tree view due to usability issues of its own but I'm not against rolling this back until it can be revisited with more use cases / lora counts in use. |
For now, let's rather fix issues and offer support for both. See if e0c9361 solves the performance degradation. |
Not very :) 2024-03-08.10-14-19.mp4 |
It's not difficult for me to create a fork, roll back locally, and await the update. However, wouldn't it be wise to roll back now so that other users, who may not be familiar with development, can work comfortably in the meantime?
|
try e0c9361 if you want to see if it improves the performance or not |
|
Oh, right, I misread that as a different hash. Are you seeing no improvement at all? The string catenation thing was taking 30 seconds for me and was reduced to nearly zero. |
I also assumed that it would help, but my performance remained the same. I even recorded the video on the third attempt, during which the page crashed completely once :) Although the performance remained same as here, I didn't encounter that issues previous time. I deliberately didn't test without OBS to maintain consistent conditions. UPD: Without OBS "Refresh" takes 34s (vs 38s on video) with Tree view and 18s with rolled back Buttons. |
a551a43 adds the option to have old dir views, set to True for now. If it's enabled, the I actually can't reproduce performance problems. Granted, I don't actually have 8k loras on disk, I use a loop to add same loras many times. |
I have about 100 loras and I thought it's many 😐 OP, how much loras you have? If its actually about few thousands, so I think it's needed to write a real file manager 😆 |
The guy in the issue has 8k, and since it worked well before, it should work well with this many after the feature. |
I agree, there need to be option for old view I mean for tree view of such amout of loras there should be optimization tricks like in real file manager |
My 10k "loras" are actually blank .safetensors files created for testing purposes. But some users do have several thousand of them — I have seen this repeatedly.
Tree View a551a43 Refresh w/o OBS — 16s treeview.mp4Dirs d062345 Refresh w/o OBS — 13s dirs.mp4 |
Pushed optimizations for sorting and filtering, should be as good now. I also made 10k empty safetensor files and am testing on those. Refresh with current dev takes about three seconds, with this PR's version - nine. |
I just tried the new dev branch, love the buttons! but it broke some XL loras, it gives a "shape" missing error. Could you investigate on that? (for example: Anime Cold Night Style SDXL_LoRA_Pony Diffusion V6 XL) XL.safetensors: AttributeError |
@KohakuBlueleaf this is the code added yesterday |
Tree View (current dev) 7d1368c Dirs (this PR) d062345 Now I have the same refresh and filtering times for both branches. But "By path" sorting still works differently. |
I've nearly finished some updates that will massively improve performance across all extra network tabs. I've pretty much rewritten everything again but this time using Clusterize.js on both the tree view and the cards panes. Still working through some bugs at the moment but so far it is leagues faster. The one tricky thing that I haven't figured out yet is how to just directly pass data from |
|
Known issue caused by me |
I think it can be closed. New option to view selection is good |
natural sort order is in now |
As for me, we can say that at the moment Tree View can be used 90% without pain :) Special thanks for the separate display of sorting options. There are still a few bugs:
Minor bugs:
Since this PR is not needed to fix these bugs, I'm closing it. Many thanks you all for participating! |
Description
#14588 is a great feature, but it has encountered several usability issues that have been noted by users.
The most common complaints include:
I propose implementing this feature as a built-in extension. This approach would provide users with the flexibility to enable or disable it as needed without impacting the main codebase.
This PR aims to rollback the changes related to the tree view, preserving all subsequent commits up to and including 58f7410. @Sj-Si agreed with this approach.
My suggestion is to merge this PR with the dev branch while creating a separate one specifically for the tree view.
Checklist: