feat: Add additional env variables to ML container #15398
Conversation
github-actions
bot
added
documentation
1-tempest
changed the title
machine-learning/app/config.py
Outdated
| if isinstance(self.clip, str): | ||
| self.clip = ClipSettings(textual=self.clip, visual=self.clip) | ||
| self.clip_fallback = "True" | ||
| if isinstance(self.facial_recognition, str): | ||
| self.clip = ClipSettings(textual=self.facial_recognition, visual=self.facial_recognition) | ||
| self.facial_recognition_fallback = "True" |
There was a problem hiding this comment.
| if isinstance(self.clip, str): | |
| self.clip = ClipSettings(textual=self.clip, visual=self.clip) | |
| self.clip_fallback = "True" | |
| if isinstance(self.facial_recognition, str): | |
| self.clip = ClipSettings(textual=self.facial_recognition, visual=self.facial_recognition) | |
| self.facial_recognition_fallback = "True" | |
| self.clip_fallback = isinstance(self.clip, str); | |
| if self.clip_fallback: | |
| self.clip = ClipSettings(textual=self.clip, visual=self.clip) | |
| self.facial_recognition_fallback = isinstance(self.facial_recognition, str); | |
| if self.facial_recognition_fallback: | |
| self.facial_recognition = FacialRecognitionSettings(detection=self.facial_recognition, recognition=self.facial_recognition) |
Make the fallback variables bool instead (not included in this suggestion). Also, the facial recognition part wasn't correct.
There was a problem hiding this comment.
When running with this approach versus toggling env variables, mypy fails the validation. Is this a major concern (I tested the code and it ran fine)?
There was a problem hiding this comment.
Did you change the types to bool?
There was a problem hiding this comment.
I meant currently. It doesn't like that these can be str (they never will be with the current logic, however it still fails the check)
clip: ClipSettings | str = ClipSettings()
facial_recognition: FacialRecognitionSettings | str = FacialRecognitionSettings()
There was a problem hiding this comment.
You can remove str from the type with something like assert not isinstance(settings.clip, str) or assert isinstance(settings.clip, ClipSettings) before using it.
There was a problem hiding this comment.
do I have to assert it everywhere it is called in test_main and main? Would this not leave a lot of fragments that could be removed when removing the depreciated env variables?
I can't find a way to add it config.py in a way that it works with the other files.
There was a problem hiding this comment.
Is there a particular reason you changed the clip and facial_recognition fields to be union types?
There was a problem hiding this comment.
If I don't then the container crashes when a depreciated variable is set
Error: Unable to configure handler 'console'
The only working solution I've come up with so far that doesn't leave a mypy warning behind is changing setting environment variables. The mypy warning doesn't understand that I'm currently removing str from clip and facial_recognition
class PreloadModelData(BaseModel):
if os.environ.get("MACHINE_LEARNING_PRELOAD__CLIP") is not None:
os.environ["MACHINE_LEARNING_PRELOAD__CLIP__TEXTUAL"] = os.environ["MACHINE_LEARNING_PRELOAD__CLIP"]
os.environ["MACHINE_LEARNING_PRELOAD__CLIP__VISUAL"] = os.environ["MACHINE_LEARNING_PRELOAD__CLIP"]
os.environ["MACHINE_LEARNING_PRELOAD__CLIP__FALLBACK"] = True
del os.environ["MACHINE_LEARNING_PRELOAD__CLIP"]
if os.environ.get("MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION") is not None:
os.environ["MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION__RECOGNITION"] = os.environ[
"MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION"
]
os.environ["MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION__DETECTION"] = os.environ[
"MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION"
]
os.environ["MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION__FALLBACK"] = True
del os.environ["MACHINE_LEARNING_PRELOAD__FACIAL_RECOGNITION"]
clip: ClipSettings = ClipSettings()
facial_recognition: FacialRecognitionSettings = FacialRecognitionSettings()
There was a problem hiding this comment.
Just handle the deprecated envs in preload_models with os.getenv without all of this messy stuff. It's waaay over-complicating this.
There was a problem hiding this comment.
I don't think I'd know how to do that in a cleaner fashion than what I've pushed. There are now 2 groups of code that need to be removed when it is decided to remove support for the depreciated variables
|
Should this be marked |
|
I left backwards compatibility, the old variable still works - just trying to depreciate it before making a breaking change. I was thinking of making it a breaking change sometime in the future when you're already making other breaking changes |
machine-learning/app/main.py
Outdated
| @@ -75,7 +75,7 @@ async def lifespan(_: FastAPI) -> AsyncGenerator[None, None]: | |||
|
|
|||
|
|
|||
| async def preload_models(preload: PreloadModelData) -> None: | |||
| log.info(f"Preloading models: {preload}") | |||
| log.info(f"Preloading models: " f"clip:{preload.clip} " f"facial_recognition:{preload.facial_recognition}") | |||
There was a problem hiding this comment.
Why is this split into multiple strings? And is it actually needed to print preload.clip etc. instead of preload?
There was a problem hiding this comment.
It cleans up the logs instead of also showing the fallback variables, not required
There was a problem hiding this comment.
Oh, I see. We can keep that for now then and change it once they're removed. I still think that should be a single f-string though.
There was a problem hiding this comment.
I missed the black formatting condensing it to a single line, I fully agree and will make a change
* Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Update config.py * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Apply formatting * minor update * formatting * root validator * minor update * minor update * minor update * change to support explicit models * minor update * minor change * minor change * minor change * minor update * add logs, resolve errors * minor change * add new enviornment variables * minor revisons * remove comments * add additional variables to ML (fixed) * add additional variables to ML (fixed) * add additional variables to ML * formatting * remove comment * remove mypy error * remove unused module * merge f strings
* Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Update config.py * Add additional variables to preload part ML models * Add additional variables to preload part ML models * Apply formatting * minor update * formatting * root validator * minor update * minor update * minor update * change to support explicit models * minor update * minor change * minor change * minor change * minor update * add logs, resolve errors * minor change * add new enviornment variables * minor revisons * remove comments * add additional variables to ML (fixed) * add additional variables to ML (fixed) * add additional variables to ML * formatting * remove comment * remove mypy error * remove unused module * merge f strings
Modify ML Container variables
ADD:
DEPRECIATE: