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

Load training arguments from .yaml, and other small changes #241

Merged
merged 1 commit into from
Mar 18, 2023
Merged

Load training arguments from .yaml, and other small changes #241

merged 1 commit into from
Mar 18, 2023

Conversation

Linaqruf
Copy link
Contributor

Hi, I want to propose some small changes about 2 things:

  1. Some image scrapers, like gallery-dl, write tags or metadata using the .jpg.txt format instead of .txt. As a result, sometimes merge_dd_tags_to_metadata.py and merge_captions_to_metadata.py can't read that format. To address this, I have added a condition to read .jpg.txt if no .txt file is found.

  2. It is becoming difficult to maintain training arguments because there are too many now. I suggest finding a way to pass training arguments to accelerate instead of writing them all down. My suggestion is to use .yaml as they are flexible and easy to maintain, compared to .json.

Example: lora_config.yaml

i've only tested this on train_network.py
image

I know you can do better than mine for the implementation. Thanks!

@kohya-ss
Copy link
Owner

Thank you for the PR! It looks good!

I am trying to manage dataset settings in .toml with another PR.
I'm not familiar with .yaml or .toml, so I'd like to know what you think about using .toml to manage the settings.

@Linaqruf
Copy link
Contributor Author

Hi, I don't know much about .toml, but I think .yaml is easier to use than .toml. .yaml is more widely used and simpler for end-users because it doesn't require strong typing like .toml. e.g. data = "value", but instead uses data: value. One weakness of .yaml probably its indentation can be tricky.

But I think .toml is also a good choice for Python users out there because it uses similar syntax to Python when declaring variables and the data type, while .yaml is more similar to defining dict. Sometimes, I make mistakes with .yaml because it's too simple such as using = instead of :, which breaks my muscle memory.

@green-s
Copy link

green-s commented Feb 28, 2023

YAML is famously over-complicated, but it does seem to be pretty standard for this kind of config in ML/Python. Coming from Rust where TOML is standard, I vote for that. It's not perfect, but it's pretty much just a stricter, extended .ini format, and since most ML configs tend to be flat/unstructured anyway, the (slightly controversial) nesting and array syntaxes are unlikely to be an issue. Either would work fine though.

Edit:

Just discovered that a TOML parser was added to the standard library in 3.11, and that it's the main way of configuring pip now (deprecating setup.py), so maybe the Python ecosystem is shifting towards TOML too?

@kohya-ss
Copy link
Owner

kohya-ss commented Mar 2, 2023

Thank you all for information. I think .toml is better if it is supported natively in Python.

@Linaqruf Is it possible to change it to .toml instead of .yaml?

@fur0ut0
Copy link
Contributor

fur0ut0 commented Mar 3, 2023

I have some suggestions:

  1. How about supporting TOML and YAML at the same time? However, it will be a concern that users may confuse with them.
  2. How about unifying with existing dataset config feature? It may be straightforward to have only single config feature, as long as their options do not conflict with each other. However, there is another concern about how we organize priority of options (current priority: TOML dataset config > CLI options > YAML training config).

@Linaqruf
Copy link
Contributor Author

Linaqruf commented Mar 4, 2023

Hi, sure. I think I can do it, but do you think this PR still relevant? as it only a simple wrapper to wrap training args and pass them to training like .ps1 or .sh, because I've read new config_util and I think it has great potential and we can explore it more.

@kohya-ss
Copy link
Owner

kohya-ss commented Mar 4, 2023

2. How about unifying with existing dataset config feature? It may be straightforward to have only single config feature, as long as their options do not conflict with each other. However, there is another concern about how we organize priority of options (current priority: TOML dataset config > CLI options > YAML training config).

It seems to me that the two configs should be kept separate rather than unified.

  1. we may want to share a dataset across multiple trainings, even for LoRA, DreamBooth or fine tuning.
  2. we may want to train multiple datasets with common training config, for example train multiple characters one by one.

The training config might assume that the dataset config is used. This means that the training config doesn't have dataset configs. The CLI options always take priority.

(However, if the training config is a simple wrapper for CLI options, it can have dataset configs.)

I think I may merge this PR as it is. Whether to use it in .toml format is a difficult. But, because as Linaqruf says, it is a simple wrapper, the code would not be too complicated if both .yaml and .toml were supported in future (except for defining the schema).

@fur0ut0
Copy link
Contributor

fur0ut0 commented Mar 5, 2023

It seems to me that the two configs should be kept separate rather than unified.

  1. we may want to share a dataset across multiple trainings, even for LoRA, DreamBooth or fine tuning.
  2. we may want to train multiple datasets with common training config, for example train multiple characters one by one.

The training config might assume that the dataset config is used. This means that the training config doesn't have dataset configs. The CLI options always take priority.

(However, if the training config is a simple wrapper for CLI options, it can have dataset configs.)

I see.

I think we can point this by unifying the config schema and supporting splitted config files feeded via CLI. However, this seems off topic of this PR. Sorry for that. I will consider of this feature in future.

I think I may merge this PR as it is. Whether to use it in .toml format is a difficult. But, because as Linaqruf says, it is a simple wrapper, the code would not be too complicated if both .yaml and .toml were supported in future (except for defining the schema).

Yes, adding config file format does not require hard work. Whether to adopt it depends on designing preference. I personally prefer providing several alternatives for config file format, but I understand decision not to provide many formats for clean design and clear usage.

@Linaqruf
Copy link
Contributor Author

Linaqruf commented Mar 5, 2023

Thanks, I'll do my best, but first I need to make sure latest commit running without error because I can't run it in colab notebook since the last update. 😅

@bmaltais
Copy link
Contributor

If this move forward it might be interesting to unify the config file with the one for the GUI so that the output of the GUI config could also be used with the CLI and vice versa... Will keep an eye on this. I personally use json to store the GUI config for sd-scripts as it is easy to deal with using python.

@Linaqruf Linaqruf closed this Mar 12, 2023
@Linaqruf
Copy link
Contributor Author

sdaddadh accidentally discarded commit

@Linaqruf Linaqruf reopened this Mar 12, 2023
@Linaqruf
Copy link
Contributor Author

Hi, sorry for the long wait, I need to make sure the code is working because I don't really know much about .toml and the nesting stuffs.

This is how training config should be passed from .toml. Nesting here is just a 'glorified' comments because I don't know how should I use it. So the code ignore it and just read the key and value of the config.

This is training config I use to train with this code : config

[model_arguments]
v2 = false
v_parameterization = false
pretrained_model_name_or_path = "/content/pretrained_model/Anything-v3-1.safetensors"

[additional_network_arguments]
no_metadata = false
unet_lr = 0.0001
text_encoder_lr = 5e-5
network_module = "locon.locon_kohya"
network_dim = 64
network_alpha = 32
network_args = [ "conv_dim=64", "conv_alpha=32",]
network_train_unet_only = false
network_train_text_encoder_only = false

[optimizer_arguments]
optimizer_type = "AdamW8bit"
learning_rate = 0.0001
max_grad_norm = 1.0
optimizer_args = [ "weight_decay=0.6",]
lr_scheduler = "constant"
lr_warmup_steps = 0

[dataset_arguments]
cache_latents = true
debug_dataset = false

[training_arguments]
output_dir = "/content/LoRA/output"
output_name = "hitokomoru_locon"
save_precision = "fp16"
save_every_n_epochs = 1
train_batch_size = 6
max_token_length = 225
mem_eff_attn = false
xformers = true
max_train_epochs = 20
max_data_loader_n_workers = 8
persistent_data_loader_workers = true
gradient_checkpointing = false
gradient_accumulation_steps = 1
mixed_precision = "fp16"
clip_skip = 2
logging_dir = "/content/LoRA/logs"
log_prefix = "hitokomoru_locon"
noise_offset = 0.1
lowram = true

[sample_prompt_arguments]
sample_every_n_epochs = 1
sample_sampler = "ddim"

[dreambooth_arguments]
prior_loss_weight = 1.0

[saving_arguments]
save_model_as = "safetensors"

@kohya-ss
Copy link
Owner

Thank you for update! I will review and merge as soon as I have time!

@kohya-ss kohya-ss changed the base branch from main to dev March 18, 2023 09:49
@kohya-ss kohya-ss merged commit 563a4dc into kohya-ss:dev Mar 18, 2023
@bmaltais bmaltais mentioned this pull request Mar 22, 2023
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.

None yet

5 participants