Skip to content

Conversation

@keturn
Copy link
Contributor

@keturn keturn commented Jul 29, 2023

The motivation here went something like this:

  • I want to add a new field to the model manager listing.
  • It looked like the right place to do that is in invokeai.backend.model_management.models.base.ModelBase, because that's where get_size is. It also seems to be the class that has access to path where the model is stored instead of just its run-time data.
  • It should be accessible without loading the full model weights in memory and locking it with ModelLocker.

I got stumped trying to figure out how to get the ModelManager to give me a ModelBase instance for a model it's managing.

This PR includes some refactoring to make that more accessible, and sketches a new ModelManager method to make those instances — but we could also revert that last bit, leaving this PR to be pure refactoring and then work out the details of the new method separately.

Usage looks something like:

from invokeai.app.services.config import InvokeAIAppConfig
from invokeai.backend.model_management import ModelManager

config = InvokeAIAppConfig.get_config()
manager= ModelManager(config.model_conf_path)
m = manager._instantiate('aZovyaPhotoreal_v2', 'sd-1', 'main')
print(m.get_size())

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

  • Refactor
    • except for the last commit, which demonstrates the motivating factor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Community Node Submission

Have you discussed this change with the InvokeAI team?

Have you updated all relevant documentation?

  • Yes

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added/updated tests?

  • Yes
  • No, not yet but geez I hope there are tests for this class

@keturn
Copy link
Contributor Author

keturn commented Jul 29, 2023

Something else I stumbled over while working on this was the necessity to call create_key everywhere, and sometimes parse_key would even do string parsing on that to try to restore information from it.

Is there still an API requirement for the key of ModelManager.models to be a string, or may we revert to using a (name: str, BaseModelType, ModelType) tuple for they key, skipping the stringificiation?

That doesn't need to be in the scope of this PR, but it would allow streamlining these functions while refactoring them.

@keturn keturn added the techdebt Toil to reduce technical debt label Jul 29, 2023
@keturn keturn mentioned this pull request Jul 29, 2023
11 tasks
@keturn
Copy link
Contributor Author

keturn commented Jul 30, 2023

Now it turns out this does fix a bug: the changes by #4061 missed the case of a relative path being used in the VAE override.

@lstein
Copy link
Collaborator

lstein commented Jul 31, 2023

Now it turns out this does fix a bug: the changes by #4061 missed the case of a relative path being used in the VAE override.

Thanks for catching that.

Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

This seems to clean up some of the awkwardness nicely. I’ll look at it in greater detail later today and do my review.

@keturn keturn force-pushed the refactor/model_manager_instantiate branch from 7deba79 to bacdf98 Compare July 31, 2023 16:16
@keturn keturn marked this pull request as ready for review July 31, 2023 16:17
@keturn keturn added the bug Something isn't working label Aug 1, 2023
@keturn keturn changed the title refactor(ModelManager): factor out _get_implementation method fix(ModelManager): fix overridden VAE with relative path Aug 5, 2023
Copy link
Collaborator

@lstein lstein left a comment

Choose a reason for hiding this comment

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

LGTM - thank you

@keturn keturn merged commit 4367061 into main Aug 7, 2023
@keturn keturn deleted the refactor/model_manager_instantiate branch August 7, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working model manager techdebt Toil to reduce technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug]: fails to load overridden VAE when path is relative

3 participants