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

Save Models in baseModel subfolder #230

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aimerib
Copy link
Contributor

@aimerib aimerib commented Aug 31, 2024

Add the functionality of reading baseModel from civitai metadata, and save models in their base model subfolders. This will look like: Models/Stable-diffusion/
SDXL 1.0/
SD1.5/
Pony/
Flux.1 D/
Flux.1 F/
etc...

This is exactly the same changes as shared in Discord. I welcome any feedback and I'm happy to make any changes necessary. This is just my own little patch that I apply after any updates because I like my models to stay organized in basemodel subfolders since it helps identify them at a glance in the UI.

Add the functionality of reading baseModel from civitai metadata, and save models in their base model subfolders. This will look like:
Models/Stable-diffusion/
  SDXL 1.0/
  SD1.5/
  Pony/
  Flux.1 D/
  Flux.1 F/
etc...
@maedtb
Copy link
Contributor

maedtb commented Sep 1, 2024

With your current implementation, you are effectively forcing everyone to use your own preferred organization method, requiring those who want to sort files in a different way to manually move the files on the server after the download completes (which may or may not be easy if they're not running SwarmUI as a listen server).

If you instead just change the JS / html, so that the model-download form's filename input box uses the base model as a prefix by default (ie, ${BaseModel}/${FileName} instead of just ${FileName}), you will get your desired behaviour, but anyone who wants to organize their files in a different way will be able to easily just by editing the text box.

@maedtb
Copy link
Contributor

maedtb commented Sep 1, 2024

If you don't end up making this a client-side only change, I don't see anything validating / sanitizing the new baseModel input. Be sure to run it through Utilities.StrictFilenameClean() to ensure that there is no possibility for bad actors to inject malicious paths in the baseModel attribute.

@aimerib aimerib force-pushed the automatic-basemodel-subfolder branch from ea2f523 to 3cd3552 Compare September 1, 2024 13:41
@aimerib
Copy link
Contributor Author

aimerib commented Sep 1, 2024

@maedtb actually really good points. I pushed another commit that makes this a user setting default to off. That way it doesn't break existing behavior, and allows users to opt-in to using this option. Would that, plus using Utilities.StrictFilenameClean() be a sufficient approach? I am hesitant to add some behavior to the UI that users might want to disable by default, therefore just annoying them.

By making this a default to off user setting, we avoid the problem of forcing users to use a base model folder when downloading models using the model downloader tool.

Check path with Utilities.StrictFilenameClean() for safety
@aimerib aimerib force-pushed the automatic-basemodel-subfolder branch from 3cd3552 to 26f0b5c Compare September 1, 2024 13:44
@mcmonkey4eva
Copy link
Member

please use regular commits btw, don't force push

@aimerib
Copy link
Contributor Author

aimerib commented Sep 1, 2024

Ah, sorry. I'm used to how my workplace does things. We only ever have a max of 3 commits per PR, so everything ends up with a git commit --amend and a git push --force-with-lease to keep the commits that show in the main repo clean. I will refrain from doing so in the future

@mcmonkey4eva
Copy link
Member

Oughtta tell them that github has a button to squash the PR after

image

So you can make PRs readable and diffs decipherable, and also keep the main repo commit history clean

@@ -273,6 +273,9 @@ public class FileFormatData : AutoConfiguration
[ConfigComment("If true, folders will be discarded from starred image paths.")]
public bool StarNoFolders = false;

[ConfigComment("Whether to automatically use the base model type as part of the model path when downloading using the 'Model Download' tool")]
Copy link
Member

Choose a reason for hiding this comment

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

3x minor doc nits:

  • . at end of the sentence
  • also it's the Model Downloader utility
  • Also doc here should ideally be a bit more clean what it does - something like \nFor example, SDXL models will be placed automatically into an SDXL/` directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it

@aimerib
Copy link
Contributor Author

aimerib commented Sep 1, 2024

We've talked about doing that. We have to be SOC2 and ISO compliant, so it is about having processes and following them, not about the processes making any sense. Changing any process is a huddle because it requires a security committee meeting and approval, and it has to then get documented. It's a lot of hassle for what is easier to just adapt lol

string modelOutPath;
if (!string.IsNullOrWhiteSpace(baseModel) && session.User.Settings.GroupDownloadedModelsByBaseType)
{
modelOutPath = $"{handler.FolderPaths[0]}/{baseModel}/{name}.safetensors";
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just editing the folder path, it should be implemented in the utiltab.js rather than the C# here.
On top of avoiding cleaning/handling/API stability issues, that also gives the perk that you can just emit the folder prefix into the existing folder dropdown field, ie allowing users to edit the path on a case-by-case basis whenever they want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I get access to user settings from the js side? On the C# side I could just reach for the user session pretty easily. I know the JS side must have a helper method for that as well, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/wwwroot/js/genpage/settings_editor.js getUserSetting maybe?

Copy link
Member

Choose a reason for hiding this comment

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

yeah getUserSetting

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.

3 participants