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

feat: preloading of machine learning models #7540

Merged
merged 12 commits into from Mar 4, 2024

Conversation

DawidPietrykowski
Copy link
Contributor

@DawidPietrykowski DawidPietrykowski commented Feb 29, 2024

In response to this discussion

Implements additional step of preloading select models at the start of the machine-learning process into memory permanently.

Models can be selected by changing the environment variable, for example:
MACHINE_LEARNING_PRELOAD="CLIP:ViT-B-32__openai,FACIAL_RECOGNITION:buffalo_s"

Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I think this can be simplified a lot. It should be good after the changes I mention in the comments.

machine-learning/app/config.py Outdated Show resolved Hide resolved
machine-learning/app/models/cache.py Outdated Show resolved Hide resolved
machine-learning/app/models/cache.py Outdated Show resolved Hide resolved
machine-learning/app/models/cache.py Outdated Show resolved Hide resolved
machine-learning/app/main.py Outdated Show resolved Hide resolved
machine-learning/app/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Great job! Just a few minor comments.

If you'd like, you can also change the env to the PRELOAD__CLIP=['model-name'] style based on this.

machine-learning/app/main.py Outdated Show resolved Hide resolved
machine-learning/app/main.py Outdated Show resolved Hide resolved
@DawidPietrykowski
Copy link
Contributor Author

Based on the suggestions I implemented a pydantic config that parses:

MACHINE_LEARNING_PRELOAD__CLIP="ViT-B-32__openai"
MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION="buffalo_s"

which seems like the least error-prone solution. As I mentioned in a comment above, this limits the env to storing a single model per type but it seems perfect for our use case as there shouldn't be more than one of each type as far as I know.

@mertalev
Copy link
Contributor

mertalev commented Mar 3, 2024

Oh, that's a good point! I don't see a use-case for loading multiple of a model type.

Copy link
Contributor

@mertalev mertalev left a comment

Choose a reason for hiding this comment

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

Awesome stuff :)

@DawidPietrykowski
Copy link
Contributor Author

Thanks a lot for the detailed review and comments :)

@mertalev
Copy link
Contributor

mertalev commented Mar 4, 2024

The formatting checks can be a bit strict haha. Running ruff app --fix should take care of it

@DawidPietrykowski
Copy link
Contributor Author

Yeah, good lesson on running these types of checks before pushing though

@mertalev mertalev merged commit e8b001f into immich-app:main Mar 4, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants