-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add simplified model manager install API to InvocationContext #6132
base: main
Are you sure you want to change the base?
Conversation
9cc1f20
to
af1b57a
Compare
I have added a migration script that tidies up the |
537a626
to
3ddd7ce
Compare
3ddd7ce
to
fa6efac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I was expecting the implementation to be, but it definitely wasn't as simple as this - great work.
I've requested a few changes and there's one discussion item that I'd like to marinate on before we change the public invocation API.
invokeai/app/services/shared/sqlite_migrator/migrations/migration_10.py
Outdated
Show resolved
Hide resolved
@psychedelicious This is ready for your review now. There are now just two calls: |
@psychedelicious I just updated the whole thing to work properly with the new (and very nice) Pydantic-based events. I've also added a new migration. Please review when you can. I'm having to resolve merge conflicts fairly regularly! |
- Any mypy issues are a misconfiguration of mypy - Use simple conditionals instead of ternaries - Consistent & standards-compliant docstring formatting - Use `dict` instead of `typing.Dict`
config: AnyModelConfig | ||
_locker: ModelLockerBase | ||
config: Optional[AnyModelConfig] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a notable API change. This means we cannot rely on a LoadedModel to have a config. Maybe this should be two separate classes, LoadedModelWithoutConfig
and LoadedModel(LoadedModelWithoutConfig)
...
I removed a number of unnecessary changes in I also moved |
- Set `self._context=context` instead of changing the type signature of `run_processor` - Tidy a few typing things
- Set `self._context=context` instead of passing it as an arg
Just a bit of typo protection in lieu of full type safety for these methods, which is difficult due to the typing of `DownloadEventHandler`.
It's inherited from the ABC.
def diffusers_load_directory(directory: Path) -> AnyModel: | ||
load_class = GenericDiffusersLoader( | ||
app_config=self._app_config, | ||
logger=self._logger, | ||
ram_cache=self._ram_cache, | ||
convert_cache=self.convert_cache, | ||
).get_hf_load_class(directory) | ||
result: AnyModel = load_class.from_pretrained(model_path, torch_dtype=TorchDevice.choose_torch_dtype()) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is unused - I think the logic to get the loader should be checking if it's a directory? I'm not sure how to fix this myself bc the diffusers_load_directory
function has a different type signature than the other loader function options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing. I've tidied a few things and tested everything, working great!
Two minor issues noted.
Summary
This adds two model manager-related methods to the InvocationContext uniform API. They are accessible via
context.models.*
:load_and_cache_model(source: Path|str|AnyHttpURL, loader: Optional[Callable[[Path], Dict[str, Tensor]]] = None) -> LoadedModel
Load the model located at the indicated path, URL or repo_id.
This will download the model from the indicated location , cache it locally, and load it into the model manager RAM cache if needed. If the optional loader argument is provided, the loader will be invoked to load the model into memory. Otherwise the method will call
safetensors.torch.load_file()
ortorch.load()
(with a pickle scan) as appropriate to the file suffix. Diffusers models are supported via HuggingFace repo_ids.Be aware that the LoadedModel object will have a
config
attribute of None.Here is an example of usage:
download_and_cache_model( source: str | AnyHttpUrl, access_token: Optional[str] = None, timeout: Optional[int] = 0) -> Path
Download the model file located at source to the models cache and return its Path.
This will check
models/.download_cache
for the desired model file and download it from the indicated source if not already present. The local Path to the downloaded file is then returned.Other Changes
This PR performs a migration, in which it renames
models/.cache
tomodels/.convert_cache
, and migrates previously-downloaded ESRGAN, openpose, DepthAnything and Lama inpaint models from themodels/core
directory intomodels/.download_cache
.There are a number of legacy model files in
models/core
, such as GFPGAN, which are no longer used. This PR deletes them and tidies up themodels/core
directory.Related Issues / Discussions
I have systematically replaced all the calls to
download_with_progress_bar()
. This function is no longer used elsewhere and has been removed.QA Instructions
I have added unit tests for the three new calls. You may test that the
load_and_cache_model()
call is working by running the upscaler within the web app. On first try, you will see the model file being downloaded into the models.cache
directory. On subsequent tries, the model will either load from RAM (if it hasn't been displaced) or will be loaded from the filesystem.Merge Plan
Squash merge when approved.
Checklist