feat(ml): Offer environment variables to set the listener host and port#24304
feat(ml): Offer environment variables to set the listener host and port#24304dionysius wants to merge 1 commit intoimmich-app:mainfrom
Conversation
|
Label error. Requires exactly 1 of: changelog:.*. Found: 🧠machine-learning. A maintainer will add the required label. |
|
It is configurable, but the variable names are not prefixed with |
|
We already support this: https://docs.immich.app/install/environment-variables#ports a PR to add a column to the documentation for “server, machine learning” would probably be good. |
|
I could look for a doc update and another column would make it quick and consistent. But this leads to following questions: What to do with the other undocumented environment variables? To name a few, but it's hard for me to compile them all since there's magic conversion...
The IMMICH_BUILD_DATA and IMMICH_MACHINE_LEARNING_* were actually the most important in my case. so basically the question is whether https://docs.immich.app/install/environment-variables should list all (I assume non-test and "production-ready") environment variables? Above list could be a baseline for a new issue (I can make if I get a quick agree or something) |
|
I dont think all of those need to be added. Host and port is fine. We don’t usually open issues for docs clarifications, you can just PR it if you’d like. |
Description
The machine-learning server listens hard coded on 0.0.0.0 and port 3003. There are currently no options to define listener host or port. I propose to introduce environment variables
MACHINE_LEARNING_HOSTandMACHINE_LEARNING_PORTto offer such configuration. If those variables are not set, the previously existing default apply.The environment variable names were chosen for consistency. All/Most ML specific environment variable start with
MACHINE_LEARNING_, all/most main server specifics start withIMMICH_This is useful for packaged builds and especially for installations on the same machine, where the machine-learning server can be set to localhost.
Any server should have a configuration to set the listener address.
No documentation has been modified as there is no mention needed for the official docker compose build (defaults stay the same). There are also other environment variables not documented which are also not relevant for the docker compose build.
There could be a documentation section for a complete list of available environment variables focused per service, but this is out of the scope for this PR.
I'm currently using this exact change as a patch in my downstream packaging.
Fixes no reported issue
How Has This Been Tested?
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
I've used an LLM to suggest me the change as a shortcut since I'm not familiar with python. I've verified the changes manually.