Conversation
VIT model inference processes spawned by the visual server had no mechanism to detect parent death and self-terminate. The process handles were also discarded immediately after spawning, making explicit cleanup impossible. - Add start_parent_check_thread() to VIT model worker processes so they monitor the visual server and self-terminate if it dies - Return process handles from start_model_process() and store them in VisualManager and VisualOnlyManager - Implement clean_up() to kill tracked VIT processes on shutdown
There was a problem hiding this comment.
Code Review
This pull request introduces process tracking and cleanup for VIT model processes within the visual server. It updates the model startup sequence to return process handles, adds a parent process check thread, and implements a cleanup method. The review feedback suggests optimizing the cleanup logic in both VisualManager and VisualOnlyManager by signaling all processes to terminate before joining them, which prevents cumulative delays during shutdown.
| for proc in getattr(self, "model_procs", []): | ||
| try: | ||
| if proc.is_alive(): | ||
| logger.info(f"Killing VIT model process {proc.pid}") | ||
| proc.kill() | ||
| proc.join(timeout=5) | ||
| except (ProcessLookupError, OSError): | ||
| pass |
There was a problem hiding this comment.
The current cleanup logic kills and joins processes sequentially. If multiple processes are unresponsive, the cumulative timeout (5 seconds per process) could significantly delay the shutdown of the visual server. It is more efficient to signal all processes to terminate first, and then join them.
| for proc in getattr(self, "model_procs", []): | |
| try: | |
| if proc.is_alive(): | |
| logger.info(f"Killing VIT model process {proc.pid}") | |
| proc.kill() | |
| proc.join(timeout=5) | |
| except (ProcessLookupError, OSError): | |
| pass | |
| procs = getattr(self, "model_procs", []) | |
| for proc in procs: | |
| try: | |
| if proc.is_alive(): | |
| logger.info(f"Killing VIT model process {proc.pid}") | |
| proc.kill() | |
| except (ProcessLookupError, OSError): | |
| pass | |
| for proc in procs: | |
| try: | |
| proc.join(timeout=5) | |
| except (ProcessLookupError, OSError): | |
| pass |
| for proc in getattr(self, "model_procs", []): | ||
| try: | ||
| if proc.is_alive(): | ||
| logger.info(f"Killing VIT model process {proc.pid}") | ||
| proc.kill() | ||
| proc.join(timeout=5) | ||
| except (ProcessLookupError, OSError): | ||
| pass |
There was a problem hiding this comment.
Similar to the VisualManager, the cleanup logic here is sequential. Signaling all processes to terminate before joining them would improve shutdown efficiency, especially when dealing with multiple model processes.
procs = getattr(self, "model_procs", [])
for proc in procs:
try:
if proc.is_alive():
logger.info(f"Killing VIT model process {proc.pid}")
proc.kill()
except (ProcessLookupError, OSError):
pass
for proc in procs:
try:
proc.join(timeout=5)
except (ProcessLookupError, OSError):
pass
Summary
start_parent_check_thread()to VIT model worker processes so they self-terminate if the visual server dies, matching the pattern used by router, audio, and other server componentsstart_model_process()and store them inVisualManagerandVisualOnlyManagerclean_up()to kill tracked VIT processes on shutdown, with race-condition-safe exception handlingTest plan
clean_up()terminates all spawned VIT processes