feat(mm): restore relative paths for invoke-managed models #6087
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We switched all model paths to be absolute in #5900. In hindsight, this is a mistake, because it makes the `models_dir` non-portable. This change reverts to the previous model pathing: - Invoke-managed models (in the `models_dir`) are stored with relative paths - Non-invoke-managed models (outside the `models_dir`, i.e. in-place installed models) still have absolute paths. ## Why absolute paths make things non-portable Let's say my `models_dir` is `/media/rhino/invokeai/models/`. In the DB, all model paths will be absolute children of this path, like this: - `/media/rhino/invokeai/models/sd-1/main/model1.ckpt` I want to change my `models_dir` to `/home/bat/invokeai/models/`. I update my `invokeai.yaml` file and physically move the files to that directory. On startup, the app checks for missing models. Because all of my model paths were absolute, they now point to a nonexistent path. All models are broken. There are a couple options to recover from this situation, neither of which are reasonable: 1. The user must manually update every model's path. Unacceptable UX. 2. On startup, we check for missing models. For each missing model, we compare its path with the last-known models dir. If there is a match, we replace that portion of the path with the new models dir. Then we re-check to see if the path exists. If it does, we update the models DB entry. Brittle and requires a new DB entry for last-known models dir. It's better to use relative paths for Invoke-managed models.
I have tested main, controlnet and vae checkpoint conversions.
…ths for legacy config files
github-actions
bot
added
python
added 2 commits
March 29, 2024 10:21
…ine relative directory positioning
lstein
approved these changes
Mar 29, 2024
There was a problem hiding this comment.
Looks good.
I made a small change to migrate_8. At some point it looks like legacy conf paths were being written to the database using root-relative paths rather than legacy_conf_dir-relative paths. I added a line to detect and correct this.
I have also changed the way that the migrate script detects when the model and config file paths are relative to the models directory. Previously a startswith() string match was used, but this would incorrectly match '/home/invokeai/models_foo' to '/home/invokeai/models' and generate an exception on the subsequent call to relative_to(). I changed these lines to use is_relative_to().
lstein
requested review from
blessedcoolant,
GreggHelt2,
brandonrising,
RyanJDick and
hipsterusername
as code owners
hipsterusername
approved these changes
Mar 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
See first commit message for why.
Includes a DB migration as a convenience for RC users whose databases are now full of absolute paths.
Related Issues / Discussions
Discord thread: https://discord.com/channels/1020123559063990373/1049495067846524939/1223169963624366080
QA Instructions
Test against:
Both test cases must have some checkpoint models to test the legacy config paths, which have the same issue.
I've done this testing already using some local DB fixtures, but I'm tired and don't trust my own work yet. I'd like to test again tomorrow, or somebody else can pick it up from here.
Merge Plan
n/a
Checklist