corbin-hayden13 / SD-Lora-Tagger

A Search Engine For All Your Local Extra Networks (in AUTOMATIC1111/stable-diffusion-webui and vladmandic/automatic)
MIT License
11 stars 2 forks source link

Tag overhaul #13

Open Cruxial0 opened 7 months ago

Cruxial0 commented 7 months ago

closes #10

Added a system to manage loading/saving of tags. Creation/deletion is still pending, but shouldn't be hard to add.

There seems to be an issue with loading the UI tabs, I'm not sure if it's caused by code within this extension or some weird caching issue with Gradio or A1111.

Additional changes:

NOTE: My changes are simply added on the side of everything else as of now, this PR is not merge ready. Feel free to make changes here.

Cruxial0 commented 7 months ago

To provide some more context to the loading issue:

Everything seems to be loading correctly before it reaches the UI. Once the UI is created, it always ends up like this, no matter how many changes I make in the code. image Editing the fields manually and saving them works as it should.

The data used is as followed

[
    {
        "name": "tag_1",
        "description": "An optional description field",
        "models": [
            "model_1",
            "model_2",
            "model_3"
        ]
    },
    {
        "name": "tag_2",
        "description": "An additional optional description field",
        "models": [
            "model_1"
        ]
    }
]
Cruxial0 commented 7 months ago

Upon further inspection, it seems to stem from A1111's ui caching. A1111 keeps a record of all components and their values in a file called 'ui-config.json' found in the base directory.

The way we are currently registering new components does not seem to work, as this cache file uses the component labels instead of the element ids (for some reason), which means all all textboxes in that page will point to the same cached value with the corresponding label.

By adding a unique prefix to the labels, it functions as expected: image I doubt this is the way we want to move forward with this, though

corbin-hayden13 commented 7 months ago

Yeah, I see what you're saying, that's annoying... is there a way to stop caching for specific components rendered? And if so, what effect does this have on the rest of the program? I have a feeling there has to be a way to stop the caching if we really want to by passing some sort of argument to the components upon creation, but I don't know what that argument/process is.

Cruxial0 commented 7 months ago

Yeah, I see what you're saying, that's annoying... is there a way to stop caching for specific components rendered? And if so, what effect does this have on the rest of the program? I have a feeling there has to be a way to stop the caching if we really want to by passing some sort of argument to the components upon creation, but I don't know what that argument/process is.

From my limited research, I wasn't able to find anything in the source code that's available to be used in the scope of extensions. There might be something, though, so feel free to have a look.

Digging through the issues, I found this https://github.com/AUTOMATIC1111/stable-diffusion-webui/issues/13981, wasn't very helpful though.

For now, I implemented a function to manually edit the cache, but that still doesn't fix the issue about having to add a suffix to each label for it to work...

Cruxial0 commented 7 months ago

Another issue I happened to stumble upon is that Gradio does not allow you to dynamically add/remove components after initialization... This is quite problematic, as it limits our actions when it comes to adding tags via UI. If we want to stick with the textboxes, the only way seems to be creating an abundance of hidden elements, then toggling the visibility later on.

corbin-hayden13 commented 7 months ago

Yeah, Gradio elements are super weird. For the current tag editing / search functionality, all I'm doing is changing the visibility of the textbox and save button elements dynamically based on if the textbox has the searched-for tag(s) in it or not.

That's why I'm now kinda on the fence about displaying by Tags then having textboxes of the file names because you can't add the extra network to your prompt from the tag editor so I at least don't see a reason to explicitly display them like that. I prefer the current design because you can change the tags on an extra network by extra network basis, so it makes the process of removing many tags from an extra network a lot more streamlined than displaying by tag. I still think we should store it as a json with tags as the keys and they hold a list of model paths, but the presentation to the use I think should be different than the underlying representation.

Regarding making and deleting elements dynamically, if that's the route we need to go then we'll have to create a custom interface using html, css, and javascript to support dynamic control over ui elements.

Cruxial0 commented 7 months ago

My main problem about displaying by models is the fact you can have a lot of models, in my case I have well over 300 LoRAs. Going through and adding the same tags for each and every one of those models is a lot of work, which is why I would rather define tags, and apply those to the models through some well designed UI. Ofcourse, the final decision is up to you, but I think the extension would benefit from atleast having it be an option.

corbin-hayden13 commented 7 months ago

I see the value in organizing by model and organizing by tag, as both come with their respective trade-offs; It becomes increasingly cumbersome to add multiple different tags to the same file when organizing by tag and it becomes increasingly cumbersome to add the same tag to multiple files when organizing by model. Therefore, I think we should do both. Have one tab for the Tag Editor for editing by model and another tab for the Tag Editor for editing by tag. We'll only have one backend tag file data structure representation, but the user can decide how they want the information to be displayed based on their needs. I think we should store the information on the backend the way you describe it above, so file names are values in a list with the tag as the key.

We can have it so that when you edit the files of one tag or edit the tags of one file, it updates accordingly on the other tab so that everything is consistent when the "save" button is clicked. I think this method of having two different ways of organizing is the best method, but let me know what your thoughts are about this.

corbin-hayden13 commented 7 months ago

While exploring other ways of designing the interface, I asked ChatGPT and these are some examples of the same problem and the solutions that it presented:

Email Clients (Gmail, Outlook): Email clients often allow users to organize emails by folders (akin to tagging by model) and also offer tagging or labeling features to categorize emails. This dual approach helps users manage large volumes of emails more effectively.

Photo Management Software (Google Photos, Adobe Lightroom): These applications allow users to tag photos and also organize them into albums. The tagging feature is particularly useful for finding all photos with a common subject, while albums are great for organizing photos from a specific event or project.

File Management Systems (Windows Explorer, macOS Finder): Modern operating systems offer both folder-based organization and tagging capabilities. Users can assign multiple tags to files and also organize them into folders, providing flexibility in file management.

Content Management Systems (WordPress, Drupal): These platforms often offer categories (for hierarchical organization) and tags (for non-hierarchical organization), allowing content creators to organize posts and articles in multiple ways.

TL;DR - The main idea is that each file gets a set of descriptors, in our case tags, and then each file is sorted into a category like a folder.

A feature request for bulk tagging (see this issue) was already proposed and I believe it's relevant to this problem. While I believe that the Two-Tabs approach is the best UI design implementation, I think we might be able to expand upon the idea or iterate on it with a file-system-like design. Again, let me know what you think of this, I absolutely agree that being able to organize by tag instead of model is valuable, but I see the good in having both vs having to compromise with one over the other.

Cruxial0 commented 7 months ago

I agree, but I don't think two tabs are strictly necessary. If you don't mind, I'd like you to check out the extension with my latest changes. I moved the UI towards a data-table format, which at least in my opinion makes it way better to look at. This will also allow us to easily be able to swap between any display format.

Ideally, I would like the last column to be similar to a Dropdown component with the multiselect setting enabled, similar to how your search bar works. That would allow us to search for either models or tags, and add a range of them with relative ease. That requires making custom components though. I am more than willing to look into that if you agree with the idea.

If you have any concerns or input regarding my changes, please let me know.

corbin-hayden13 commented 7 months ago

Ok, I took a look at your branch, here are my thoughts:

Overall, great work! I can see that a lot of thought has gone into this and I will say I didn't even think to use a Gradio table for the tag representation so that's super clever.

I know you have a lot of work on your own fork, but I've made over 30 commits since you started doing this so I would prefer you make a new branch on this repo under this issue and copied your work to that so that you're developing on top of the latest version of the extension, not an outdated one. I think this is also important because the current version of the extension on your fork is missing key directories (mainly the javascript folder) that it relies on to exist so that the javascript override files can be copied from staging to that particular folder to allow for the model cards to be searched properly. These exist on my repo so we shouldn't run into any issues if you start a new branch and copy your work.

Also, I know it's a WIP but there are a couple things that are not handled that I didn't want to go overlooked:

Finally, I know there's not really a standard right now for reviewing PR's between the two of us, but because this is such a drastic change to both the UI and the backend, I'd appreciate if when you feel like you have a production-ready implementation you open a PR for me to review so that we make sure there's not weird issues and bugs that pop up.

Cruxial0 commented 7 months ago

Thanks for the feedback. I feel like I should address some of these points.

I think the search bar should search by both tag, model, and description, not just tag and model.

It does. If it didn't work for you, then that's an unintended bug.

def search(search_term: str):
    if search_term == '' or search_term is None:
        return to_dataframe()

    filtered_tags = []
    for tag in tags:
        if search_term in f'{tag.name} {tag.description} {tag.models}':
            filtered_tags.append(tag)
    return to_dataframe(filtered_tags)

I think there should be an option to add and remove multiple tags from one file, so I think the inclusion of a of ["dropdown", "textbox", "optional_save_button"?] that would allow the user to choose from one of their model files through the dropdown, have the textbox auto-populate with the tags that that model is associated with, then have some functionality through a save button to write / update the file name under the JSON structure according to the edited list of tags the user created in the textbox to allow the editing of multiple tags for one file.

I'm not sure if I understand what you're getting at here. I mentioned adding a custom component that displays arrays as an embedded multiselect dropdown component within the cells. If this is not what you meant, then I'd appreciate if you could elaborate a bit more.

I know you have a lot of work on your own fork, but I've made over 30 commits since you started doing this so I would prefer you make a new branch [...]

It shouldn't really be necessary. I'll keep updating my branch from main and solve any merge conflicts that appear. It's actually kind of counter-intuitive to create a new branch, as we will lose the commit history of this one, making it harder to track down eventual bugs. I intend to continue developing on this PR until I feel like we have something production-ready. If there is any specific reason you want me to create a new branch, let me know.

The tags.json file cannot be initialized by an empty string because it causes a JSON error and the editor doesn't render when starting the webui. [...]

Nice catch, I'll have that fixed ASAP.

The tag saving logic doesn't work as intended. I'm not quite sure what the issue is yet, but I think it's not overwriting the original tags.json file with the content of the table so no tags can actually be removed.

Yeah, the codebase for the tag manager is still in an early stage. It should be seeing improvments with every commit I make from now on out.

[...] I'd appreciate if when you feel like you have a production-ready implementation you open a PR for me to review [...]

I just want to assure you that I will never merge anything into main without having it reviewed first. That's a key part of developing with git anyway. As mentioned above, if you don't have any specific reason to create a new PR/branch, I'll continue working on this one and request your review once I feel it is ready to be merged.


I also want to go mention some additional details that need to be addressed.

We need to think about users updating from an older version of the extension. Some system should be put in place to auto-update from the old system to the new one. While on that topic, it might be useful to also think about future-proofing, in case we want to update the tag data structure again for some reason.

corbin-hayden13 commented 7 months ago

I read you loud and clear, sounds like you’ve got your own stuff covered, I’ll leave you to it👍

For implementing the update rollout logic, that’s fairly simple as we can add some startup stuff that checks if the current “network_descriptions” folder exists and write all of the current tags to the tags.json folder. I would say that we shouldn’t forcefully delete the network_descriptions folder but should print a warning or something saying that the network_descriptions tag scheme has been deprecated (and maybe remove the files in a future update🤷‍♂️)

As for future proofing, I’m not quite sure where to start, so if you have some ideas and concerns throw them at me and I’ll start working on a way to patch them.

Finally, for the in-cell dropdown you mentioned, I’m not quite sure what your vision for that is, but later today I’ll comment a quick drawn mock-up of the vision I have to give you a better idea.

corbin-hayden13 commented 7 months ago

Regarding future proofing, one thing that I just thought of is that we should have an interface as a facade to the tag file read and write so that we can make changes to the backend with affecting the frontend; Something like a ‘read_all_tags()’ function that always returns a dict of tags as keys and lists of models as values, same for writing tags to the database. This way, all we would have to change is the read and write logic in the interfaces and the frontend works just the same.

Cruxial0 commented 7 months ago

When you mention an interface, were you thinking something along the lines of this?

class TagManagerAPI():
    from enum import Enum
    import scripts.helpers.tag_manager as tm

    class DisplayMethod(Enum):
        TAG = 0
        MODEL = 1

    def read_all_tags() -> list[list[str]]:
        pass

    def search() -> list[list[str]]:
        pass

    def add_row() -> list[list[str]]:
        pass

    def del_row() -> list[list[str]]:
        pass

    def save() -> list[list[str]]:
        pass

Where the UI interacts with the API object, and the API object interacts with the back-end.

corbin-hayden13 commented 7 months ago

Yes, exactly! I started implementing something like it by trying to abstract the specific read/write operations away from utils.py, etc. as much as possible. Making it a tag manager object I think would be really good too. Right now the read and write is done on a network-type basis as each group of tag files is organized by their respective network, but are we going to move away fro that model when implementing the json file and just have them relatively disorganized in the file?

Also, here’s the image of the model->tag editor option I was talking about: image

The “Model \/“ is a dropdown menu with all of the names of the current networks so you can find and search and select by model name to edit the tags of that one particular file. The textbox/area next to it hold the model’s tags and allow editing of them.

I notice you have an Enum class for differentiating between the display types. I think we only need to have one display (because I’m not sure how to have two display types without doubling the memory usage when loading/creating dictionaries in the table object), I just want to have a way to edit multiple tags of one model easily.

Cruxial0 commented 7 months ago

For organizing, we could either add a model_type field to the json structure, or we could add the model type to the 'extras' part of the search_term on start up. Doing so would allow us to either search for, or add a filter option to the tag editor. I don't think having multiple json files is beneficial for other means than manually editing it, which shouldn't really be needed, given we can create a user-friendly front-end design.

I can see the convinience with having that extra piece of UI. Personally, I don't like how the dropdown component feels, so I wanted to avoid them as much as possible. However, this seems like an intuitive way to edit tags on the fly.

As for the display type, my thought process behind that was just editing the data on demand. We still only need to load the tags once, but the data will be reorganized before making it to the front/back-end. It will add some slight computational load when reading/writing tags, but it should be negligible unless you have thousands of tags. I was thinking this would be a better way of achieving the "two-tab" approach we discussed earlier.

Cruxial0 commented 7 months ago

I went ahead and implemented a suggestion for the interface structure. With this, I feel like we are nearing a point where I'm happy with the features. Some things are still missing, such as the rollout logic and the extra piece of UI you described above. But in terms of functionality, it seems to be mostly functional now.

If you want to test it out, note that a few lines in sd_lora_tagger.py may need be edited manually. The last line controls how the table will display the tags (0 = by tag, 1 = by model).

api = TagManagerAPIv1()
ui = TagEditorUI(api)
api.set_display_method(1)
corbin-hayden13 commented 7 months ago

Yeah, I’ll take a look at it and let you know what I think. I’m actually super excited to see the final product of the restructuring!

corbin-hayden13 commented 7 months ago

After testing your branch, here is what I found:

All recorded when restarting server from within UI

All recorded after hard restarting client and server

What Went Well

Backend tag file representation looks good!

I like that the table updates every time the user clicks out of the cell, it's good to keep a working copy of the table on the backend so that work isn't lost.

Once this is feature complete I'd like to do a stress test of the system to ensure it's robust enough to handle hundreds of models and tags.

Interesting choice to auto-display tags alphabetically, I'm not quite sure how much I like that, maybe have a toggle to enable/disable that behavior?

Cruxial0 commented 7 months ago

Thanks for the extensive feedback. I tackled most of the bugs you mentioned. I'm waiting to patch a few until #32 is merged.

Interesting choice to auto-display tags alphabetically, I'm not quite sure how much I like that, maybe have a toggle to enable/disable that behavior?

Do you have any suggestion as what to do instead? If it doesn't get sorted, it will remain in the order of the tags.json file, which is not organized in any way, shape or form.

corbin-hayden13 commented 7 months ago

Eh that was more nit-picky than anything else, it just threw me off a bit when I added all of my tags to a model and then clicked off the table and the tags reorganized themselves. The auto-order works and is a nice way of organizing the tags and models so lets stick with it

Cruxial0 commented 7 months ago

Sorry for the lack of progress over the past week. Had some IRL stuff take up most of my time.

I spent some hours going over and polishing some stuff. I feel like this PR is getting merge-ready relatively soon, though realistically there are probably tons of things I overlooked. Here is a quick breakdown on what's been implemented:

When you have time, I'd like you to once again review this PR, and note down any issues/feedback you run into along the way.

corbin-hayden13 commented 6 months ago

Hey, no problem! Thanks for putting a lot of effort into this, I'll take a look at it! I just spent the last two weeks playing Helldivers and took a break from development so I'm back to see this through.

corbin-hayden13 commented 6 months ago

Semi-Comprehensive Review

Fails if the "network_descriptions" folder does not exist in the extension directory on clean install

shared.opts.data = cmd_opts from modules.shared

See try-except block below because extensions fails to load after fresh install currently as there is no key set in cmd_opts on fresh install

try:
    display_mode_data = int(shared.opts.data[display_mode_key])
except KeyError:
    display_mode_data = 0  # Default value

I think the "Table Display Method" setting could just be a dropdown with "by Tag" and "By Model" as options and the backend could convert those strings to ints using a simple comparison. This I think would make the setting more intuitive and less confusing

The search works phenominally!

Saving works!

Add and Remove both properly add and remove rows from the table!

For the extras section, I think it would be a good idea to take some inspiration from the main UI and put the Extras dropdowns in their own slim column on the left and have the table in a wider column on the right.

Additionally, I think it would be nice to have an argument in the create() method to override the label of the extras accordion so that there's not potentially many accordions called "Extras"

After changing the display type from "By Tag" to "By Model" the table initially starts empty with headers | 0 | 1 | 2 | but after licking "Add" the table headers get set except for the descriptions column which the header turns blank.

Furthermore, after switching the display type, none of the saved tags and models are displayed in the table. This leads me to elieve there was some sort of error that occured which returned a generic table instead of a populated one, but there was no error message.

I was able to produce a weird issue where the table just displays a bunch of blank rows after mass removing tags after switching from "By Model" to "By Tag" and just reloading the UI

After reloading the UI and mass removing tags, refreshing the page without reloading the UI displays all of the tags I tried to remove previously, which is what might be contributing to the large number of blank rows I had earlier.

Cruxial0 commented 6 months ago

I realise my commit history says 9 hours ago, but a lot of these commits were made about half an hour ago. I am currently working as we speak :D

I haven't been able to replicate the issues you're facing with the table headers not being displayed properly - is this a SD.Next exclusive issue?

I will get around to adding a fail check for the network_descriptions folder, and the old tag system will be completely disabled when I feel confident the update logic works flawlessly.

I am a bit unsure how to fix the add/remove buttons as of now. However, I do agree it should not add a new Tag python object, but instead a new row following whichever tag representation you're using.

Finally, the existence of an empty tag in the backend is neccessary to store models without tags. Alternatively, we can also create a tag with a unique placeholder name such as <EMPTY>. Let me know what you would prefer. As for everything I didn't address, I will be getting around to it.

Also, on a side note. A1111 added a feature that adds model descriptions to search_terms, effectively invalidating this extension lol (see your PR on stable-diffusion-webui-extensions for more details). Personally, I still think this extension provides value in terms of accessibility - our UI is a lot more intuitive than A1111's metadata editor. I would also like to mention that NSFW filtering is currently broken for A1111 on the main branch, as I had to roll out a hotfix for the latest version of A1111 which updated their javascript. If you have time, I'd like you to look into that, as I don't really know where to start with that.

EDIT:

On clean install, extra network cards on the main UI tab are hidden for some reason

Are you on automatic1111 1.8+? Chances are it's broken on earlier versions.

corbin-hayden13 commented 6 months ago

Ok, I can take a look at the javascript and see what's up... if automatic is adding more stuff to the search terms like us then the flag is probably broken as well, I'll see if I can't come up with a fix for that.

Also I did all of my testing on a1111, so I think I might have had an outdated version

w-e-w commented 6 months ago

Yeah, I see what you're saying, that's annoying... is there a way to stop caching for specific components rendered? And if so, what effect does this have on the rest of the program? I have a feeling there has to be a way to stop the caching if we really want to by passing some sort of argument to the components upon creation, but I don't know what that argument/process is.

From my limited research, I wasn't able to find anything in the source code that's available to be used in the scope of extensions. There might be something, though, so feel free to have a look.

Digging through the issues, I found this AUTOMATIC1111/stable-diffusion-webui#13981, wasn't very helpful though.

For now, I implemented a function to manually edit the cache, but that still doesn't fix the issue about having to add a suffix to each label for it to work...

not sure if this is too late, I didn't read the entire conversation

irrc the rule in A41 for gradio component is "if a gradio component has a label then it the value will be save to ui-config.json" this is to allow user to customise the default value of components

checkbox ture / fales, slider values and (range), input box values etc

a usere can change the values buy using the Settings > Defaults page for advance stuff like slider range and step size the have edit ui-config.json manuly

the importent bit this enabled by default but Developers can opt-out a component from this system by adden the do_not_save_to_config' to a gradio component

ui_loadsave code https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/8ac4a207f3a57c31f7fe4dbae2f256eb39453169/modules/ui_loadsave.py#L47

example

# for a single component
test_cb = gr.Checkbox()
test_cb.do_not_save_to_config = True

# you have lots of components because applying the attributes in and loop

c_list = []
c1 = gr.XXX()
c2 = gr.XXX()
...
c_list = [c1, c2, .....]
[setattr(c, 'do_not_save_to_config ', True) for c in c_list]

note this is hand types from memory there could be some simple mistakes

w-e-w commented 6 months ago

Also, on a side note. A1111 added a feature that adds model descriptions to search_terms, effectively invalidating this extension lol (see your https://github.com/AUTOMATIC1111/stable-diffusion-webui-extensions/pull/285 on stable-diffusion-webui-extensions for more details). Personally, I still think this extension provides value in terms of accessibility - our UI is a lot more intuitive than A1111's metadata editor.

I like to clarify even though someone added a similar feature in base webui you and this does not prevent this extension from being added to the index the reason I didn't add it was because it was broken at the time of testing

I only mention about the upcoming future in dev brunch as a heads up so that you people are aware and letting you decide whether or not continue to development if you do wish to continue criteria of being listed on the index is the same it has to work for webui

Cruxial0 commented 6 months ago

@w-e-w

this enabled by default but Developers can opt-out a component from this system by adden the do_not_save_to_config' to a gradio component

I knew there probably had to be a way of disabling the caching, but I wasn't able to find it. Thanks for clearing that up!

As for 1.8 support, are you sure you tested the latest version? The error message you posted is the exact same as the one I got before implementing the patch. I am not able to reproduce as of 0055952 being merged.

w-e-w commented 6 months ago

As for 1.8 support, are you sure you tested the latest version? The error message you posted is the exact same as the one I got before implementing the patch. I am not able to reproduce as of 00559522cb32be23682b80b9f8b3abd0bfe4f54f being merged.

did a another test just now to make sure 00559522cb32be23682b80b9f8b3abd0bfe4f54f is brokens on webui 1.8 master but works (at least no error) on dev branch https://github.com/AUTOMATIC1111/stable-diffusion-webui/commit/c4664b5a9c2f2302dae8be1c1daab94ad8a80001

KeyError: 'shorthash'
corbin-hayden13 commented 6 months ago

This is getting very close to done...

nsfw filter doesn't work

Tags are duplicated when switching from "By Model" to "By Tag" and tags added and saved in "By Tag" are not displayed when switching to "By Model"

Cruxial0 commented 6 months ago

Tags are duplicated when switching from "By Model" to "By Tag" and tags added and saved in "By Tag" are not displayed when switching to "By Model"

Not able to replicate this. Is this on A1111 or SD.Next? I fixed an issue where tags would be overwritten if the old folders were regenerated, which would happen if you were to change branches. Not sure if that's related to the issues you're having or not.