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

refactor: simplified config system #5920

Merged
merged 54 commits into from
Mar 18, 2024

Conversation

psychedelicious
Copy link
Collaborator

@psychedelicious psychedelicious commented Mar 11, 2024

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix

Have you discussed this change with the InvokeAI team?

  • Yes
  • No, because:

Have you updated all relevant documentation?

  • Probably missed a few spots

Description

I ran into some issues with interactions between argparse, omegaconf and pydantic validation + existing issues:

  • argparse autogenerated CLI args for every config setting and this cannot handle structured config data (for example, the list of url regex & token pairs). It was not simple to just skip those settings due to the tight coupling of argparse and the config object.
  • We were writing every default config setting to the config file. This is a longstanding issue. With this PR, that will no longer happen, and a simple migration from v3 config to v4 partially tidies all configs. I sneakily changed the name of png_compress_level to pil_compress_level - this will get everybody using the better default value of 1.
  • There was an awkward API with InvokeAIAppConfig.get_config.

It was all very intertwined and I couldn't figure out an easy fix, so I refactored the whole thing and simplified it substantially.

See the commit messages for details on the changes, especially 6b066f3 which has the meat and potatoes. Most changes are just small changes to use the revised config system.

The API did change slightly from that commit, to support testing and the configurator.

QA Instructions, Screenshots, Recordings

  • Start up the app, your invokeai.yaml should change to the new flat format, and the old one backed up to invokeai.yaml.bak. Your YAML should now only contain settings that are different from defaults. Everything should work exactly the same as it did before - like custom paths for models, host, port.
  • Restore your backed up YAML & delete the .bak file. Run the configurator - it should migrate the YAML as it starts and back it up, same as before. Change a couple settings and apply changes, the YAML should change accordingly.

It's hard to fully test this change because the config is used all over the place, but I was pretty darn careful when updating all the places the config is used.

Merge Plan

This PR can be merged when approved

Added/updated tests?

  • Yes
  • No : please replace this line with details on why tests
    have not been included

Closes #5964

@github-actions github-actions bot added api python PRs that change python files invocations PRs that change invocations backend PRs that change backend files services PRs that change app services python-tests PRs that change python tests docs PRs that change docs labels Mar 11, 2024
@hipsterusername
Copy link
Member

hipsterusername commented Mar 11, 2024

I've run the proposed test, but the resulting config file doesn't seem to have some of my previous settings (like compress level, etc)

This is all that's in the resulting config

# Internal metadata
meta:
  schema_version: 4

# User settings
outputs_dir: C:\Users\kentr\Invoke\outputs
log_level: debug
precision: bfloat16

@psychedelicious
Copy link
Collaborator Author

Please paste your original config.

The compress level is supposed to be reset and not be there. The new default is the best setting. For users who have the old default of 6 baked into their config, the new default of 1 represents up to 30% improvement every time we save an image.

@psychedelicious
Copy link
Collaborator Author

psychedelicious commented Mar 11, 2024

Re: failing tests - looks like the default YAML file isn't being created. This didn't come up locally for me because I already had a YAML file. I'll figure that out tomorrow & test with the installer.

@hipsterusername
Copy link
Member

  Web Server:
    host: 127.0.0.1
    port: 9090
    allow_origins: []
    allow_credentials: true
    allow_methods:
    - '*'
    allow_headers:
    - '*'
  Features:
    esrgan: true
    internet_available: true
    log_tokenization: false
    patchmatch: true
  Memory/Performance:
    always_use_cpu: false
    free_gpu_mem: false
    max_cache_size: 12.0
    max_vram_cache_size: .25
    precision: auto
    sequential_guidance: false
    xformers_enabled: false
    tiled_decode: false
    lazy_offload: true
    log_memory_usage: false
    png_compress_level: 1
  Paths:
    autoimport_dir: autoimport
    lora_dir: autoimport\lora
    embedding_dir: autoimport\embedding
    controlnet_dir: autoimport\controlnet
    conf_path: configs\models.yaml
    models_dir: models
    legacy_conf_dir: configs\stable-diffusion
    db_dir: databases
    outdir: C:\Users\kentr\Invoke\outputs
    use_memory_db: false
  Logging:
    log_handlers:
    - console
    log_format: color
    log_level: debug```

@psychedelicious
Copy link
Collaborator Author

@hipsterusername Your resultant config file is almost exactly what it should be. Those values are all default values, they should not be in a config file that is meant to override defaults.

The things that are not correct are the cache sizes. Turns out you are using "legacy" config settings:

    max_cache_size: 12.0 # This has been called "ram" for a while now
    max_vram_cache_size: .25 # This has been called "vram" for a while now

So what's needed to make your new yaml 100% correct is to add handling for these two settings into the simple migration logic. I'll fix that.

@psychedelicious psychedelicious force-pushed the psyche/refactor/simplified-config branch 2 times, most recently from bf04279 to 0482b38 Compare March 11, 2024 22:34
@blessedcoolant
Copy link
Collaborator

blessedcoolant commented Mar 13, 2024

Works as intended on my end. A couple of small issues that I saw were some of the paths stayed in the new config even though they were the same as the defaults -- but the reason that might have happened is coz I was using absolute paths for those rather than relative.

For example <fullpath>/models was kept by the new config despite it being the exact same location as <root>/models

Rest of it seems good.

@blessedcoolant
Copy link
Collaborator

Is there anything holding this back?

@psychedelicious
Copy link
Collaborator Author

@blessedcoolant I'll be reviewing it again today, thinking through edge cases and testing interactions with the installer/configurator. Aiming to have this in RC3.

@psychedelicious psychedelicious force-pushed the psyche/refactor/simplified-config branch from 0482b38 to 490e152 Compare March 15, 2024 00:05
@github-actions github-actions bot added Root python-deps PRs that change python dependencies labels Mar 15, 2024
@psychedelicious
Copy link
Collaborator Author

I added two notable changes:

feat: single app entrypoint with CLI arg parsing

We have two problems with how argparse is being utilized:

  • We parse CLI args as the api_app.py file is read. This causes a problem pytest, which has an incompatible set of CLI args. Some tests import the FastAPI app, which triggers the config to parse CLI args, which receives the pytest args and fails.
  • We've repeatedly had problems when something that uses the config is imported before the CLI args are parsed. When this happens, the root dir may not be set correctly, so we attempt to operate on incorrect paths.

To resolve these issues, we need to lift CLI arg parsing outside of the application code, but still let the application access the CLI args. We can create a external app entrypoint to do this.

  • InvokeAIArgs is a simple helper class that parses CLI args and stores the result.
  • run_app() is the new entrypoint. It first parses CLI args, then runs invoke_api to start the app.

The invokeai-web project script and invokeai-web.py dev script now call run_app() instead of invoke_api().

The first time get_config() is called to get the singleton config object, it retrieves the args from InvokeAIArgs, sets the root dir if provided, then merges settings in from invokeai.yaml.

CLI arg parsing is now safely insulated from application code, but still accessible. And we don't need to worry about import order having an impact on anything, because by the time the app is running, we have already parsed CLI args. Whew!

fix(install): resolve config-related issues with configurator

  • Do not use the singleton app config, this causes a lot of weirdness
  • Update logic to use new config object
  • Remove unused code

@psychedelicious
Copy link
Collaborator Author

I've spruced up the config docs a bit.

Also fixed a problem where user-defined models.yaml paths weren't being migrated properly. This issue was brought to my attention in a discord comment about RC2. I noticed I had broken that migration logic in this PR and fixed it. I have not fixed the issue on main.

psychedelicious and others added 17 commits March 18, 2024 22:39
Hold onto `conf_path` temporarily while migrating `invokeai.yaml` so that it gets migrated correctly as the model installer starts up. Stashed as `legacy_models_yaml_path` in the config, excluded from serialization.
Add testing for the settings that had to be explicitly migrated.
When running the configurator, the `legacy_models_conf_path` was stripped when saving the config file. Then the migration logic didn't fire correctly, and the custom models.yaml paths weren't migrated into the db.

- Rework the logic to migrate this path by adding it to the config object as a normal field that is not excluded from serialization.
- Rearrange the models.yaml migration logic to remove the legacy path after migrating, then write the config file. This way, the legacy path doesn't stick around.
- Move the schema version into the config object.
- Back up the config file before attempting migration.
- Add tests to cover this edge case
- fix places where `get_config()` is being called at import time rather
  than at run time.

- add regression test for import time get_config() calling.
Fixes converting ckpt main models
@psychedelicious psychedelicious force-pushed the psyche/refactor/simplified-config branch from e0beb44 to 87f769b Compare March 18, 2024 11:39
@psychedelicious psychedelicious merged commit 9d04596 into main Mar 18, 2024
14 checks passed
@psychedelicious psychedelicious deleted the psyche/refactor/simplified-config branch March 18, 2024 22:24
@skunkworxdark
Copy link
Contributor

Now that the Inkokeai.yaml will initially only contain the settings that are not default. Will there be a method to output a full config including all the default settings. This could be used to help diagnose support issues. Obviously this should include a way to censor sensitive information like website download tokens.

@psychedelicious
Copy link
Collaborator Author

@skunkworxdark The docs here have all settings: https://invoke-ai.github.io/InvokeAI/features/CONFIGURATION/#all-settings

Maybe we could have the config file be written with settings commented out? I'm wary of putting all the settings there:

  • Most people should only change a very small number of settings.
  • If you accidentally leave a default manually set, and we determine a better default setting, users will be stuck on an old default.

@skunkworxdark
Copy link
Contributor

@skunkworxdark The docs here have all settings: https://invoke-ai.github.io/InvokeAI/features/CONFIGURATION/#all-settings

Maybe we could have the config file be written with settings commented out? I'm wary of putting all the settings there:

  • Most people should only change a very small number of settings.
  • If you accidentally leave a default manually set, and we determine a better default setting, users will be stuck on an old default.

I see all the settings there but not what the default will be if it is omitted from the yaml. I can find what they are by looking at the code but most users will not be able to do this.

Commented defaults sounds like a nice compromise. But would they change if a default is updated.

Maybe a way of viewing a list of what the current settings are via the frontend including defaults that are not in the yaml file. As I said before would need sensitive info like tokens censored. Then you could keep the yaml clean and mark in the frontend list which are defaults and which are defaults but omitted etc.. could even add the descriptions that you linked in the docs.

I'm not suggesting adding a way of editing the config just viewing them with defaults, but that might be nice in the long term.

@psychedelicious
Copy link
Collaborator Author

@skunkworxdark Ya I understand. An example config file will be written to invokeai.example.yaml:

# This is an example file with default and example settings. Use the values here as a baseline.

# Internal metadata - do not edit:
schema_version: 4.0.0

# Put user settings here - see https://invoke-ai.github.io/InvokeAI/features/CONFIGURATION/:
host: 127.0.0.1
port: 9090
allow_origins: []
allow_credentials: true
allow_methods:
- '*'
allow_headers:
- '*'
log_tokenization: false
patchmatch: true
autoimport_dir: autoimport
models_dir: models
convert_cache_dir: models/.cache
legacy_conf_dir: configs
db_dir: databases
outputs_dir: outputs
custom_nodes_dir: nodes
log_handlers:
- console
log_format: color
log_level: info
log_sql: false
use_memory_db: false
dev_reload: false
profile_graphs: false
profiles_dir: profiles
ram: 7.5
vram: 0.25
convert_cache: 20.0
lazy_offload: true
log_memory_usage: false
device: auto
precision: auto
sequential_guidance: false
attention_type: auto
attention_slice_size: auto
force_tiled_decode: false
pil_compress_level: 1
max_queue_size: 10000
node_cache_size: 512
hashing_algorithm: blake3
remote_api_tokens:
- url_regex: cool-models.com
  token: my_secret_token
- url_regex: nifty-models.com
  token: some_other_token

I'll also be getting the JSON schema for the config put onto the schema store. This will enable editors to get autocomplete and error highlighting in invokeai.yaml. VSCode supports this automatically, with no user configuration - I'm sure other editors do too.

@skunkworxdark
Copy link
Contributor

skunkworxdark commented Mar 19, 2024

I have tried using the env variables to configure some folders for dev however I have come across an issue.

When I use the below launch.json to start invoke for dev. It writes out these env settings to the invokeai.yaml file overwriting the existing settings. I wasn't expecting that. As this is pointing to my normal invoke install this would mean that I would have to change the settings back to normal before I could run the normal installation.

Additionally I don't see the invokeai.example.yaml you mentioned.

  "configurations": [
    {
      // Run the InvokeAI backend & serve the pre-built UI
      "name": "InvokeAI Server New",
      "type": "debugpy",
      "request": "launch",
      "program": "scripts/invokeai-web.py",
      "console": "integratedTerminal",
      "justMyCode": true,
      "args": ["--root", "d:\\ai\\invokeai"],
      "env": {
        // Access the app from anywhere on your local network
        "INVOKEAI_HOST": "0.0.0.0",
        // DB Folder
        "INVOKEAI_DB_DIR": "d:\\git\\invoke-devdb",
        //Nodes Folder
        "INVOKEAI_CUSTOM_NODES_DIR": "d:\\git\\backup\\nodes-test",
        //MemoryDB
        //"INVOKEAI_USE_MEMORY_DB": "true"
        },
    },

@psychedelicious
Copy link
Collaborator Author

@skunkworxdark
The example file (and everything I am describing here) is in a different, pending pr: #5991

That PR also includes the ability to use an arbitrary config file. So you can pass --config /path/to/invokeai-dev.yaml or whatever you'd like. Hopefully that's easier than using env vars and supports your use case.

Re: env vars being written, I expect this to occur if you have them specified along with an uninitialized root directory. Not sure how to get around this, bc env vars are handled by the settings library and I don't see a way to defer merging them without disabling it completely.

@skunkworxdark
Copy link
Contributor

skunkworxdark commented Mar 20, 2024

@skunkworxdark The example file (and everything I am describing here) is in a different, pending pr: #5991

That PR also includes the ability to use an arbitrary config file. So you can pass --config /path/to/invokeai-dev.yaml or whatever you'd like. Hopefully that's easier than using env vars and supports your use case.

Re: env vars being written, I expect this to occur if you have them specified along with an uninitialized root directory. Not sure how to get around this, bc env vars are handled by the settings library and I don't see a way to defer merging them without disabling it completely.

@psychedelicious

The --config option is great and does solve my use case issues.

What do you mean by uninitialized root directory? If by that you mean you don't have a schema_version: 4.0.0 style yaml then that is not what I am seeing. With a 4.0 yaml file in place, I still get the env vars updating the yaml file.

Also, I am seeing the yaml file getting updated even if nothing has been changed inside the file every time I run my dev server.

I don't think either of these things is game-breaking but they are not how I would have expected them to work. I would have thought that the env vars would be overrides to the config not updates to it. Also, I wouldn't have expected the config to be updated unless some config was changed within the app.

@psychedelicious
Copy link
Collaborator Author

@skunkworxdark By uninitialized root directory, I meant a root directory without a config file.

I'll figure out how to prevent env vars from being written to file, I agree that this is incorrect behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api backend PRs that change backend files docs PRs that change docs invocations PRs that change invocations python PRs that change python files python-deps PRs that change python dependencies python-tests PRs that change python tests Root services PRs that change app services
Projects
None yet
6 participants