[Test] Fix pytest termination with @create_new_process_for_each_test("fork")#29130
[Test] Fix pytest termination with @create_new_process_for_each_test("fork")#29130markmc wants to merge 1 commit intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request provides a robust fix for the pytest termination issue when using @create_new_process_for_each_test("fork"). The changes correctly move the os.setpgrp() call into the child process, ensuring it becomes the leader of its own process group. This is the standard and correct way to isolate a child process and its descendants for group-based signaling.
As a result, the parent process no longer needs to handle SIGTERM itself, which simplifies the code and removes the problematic signal-ignoring logic that was failing in some environments. The addition of contextlib.suppress(ProcessLookupError) when killing the process group is also a great improvement, making the cleanup process more resilient to race conditions where the child processes might have already exited.
Overall, the changes are clean, correct, and a significant improvement in robustness for the test utility.
…"fork")
I am seeing pytest terminating itself when cleaning up child processes
for any test that uses @create_new_process_for_each_test("fork"):
```
$ pytest -vs tests/v1/engine/test_engine_core.py::test_engine_core
...
Running 1 items in this shard: tests/v1/engine/test_engine_core.py::test_engine_core
tests/v1/engine/test_engine_core.py::test_engine_core Fork a new process to run a test 3071367
huggingface/tokenizers: The current process just got forked, after parallelism has already been used. Disabling parallelism to avoid deadlocks...
To disable this warning, you can either:
- Avoid using `tokenizers` before the fork if possible
- Explicitly set the environment variable TOKENIZERS_PARALLELISM=(true | false)
Fork a new process to run a test 0
...
INFO 11-20 19:04:34 [v1/engine/core.py:250] init engine (profile, create kv cache, warmup model) took 4.02 seconds
Terminated
```
It looks like the SIG_IGN isn't working for some reason and, maddeningly,
I haven't been able to isolate it with a more simple reproducer. Also, of
course, this doesn't seem to be happening in the `docker run --init`
environment in CI.
While it would be good to understand exactly what's going on, it seems
pretty clear that a simple fix is to put the child process in its
own process group, and terminate that ... rather than terminating the
parent and relying on SIG_IGN.
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
I am seeing pytest terminating itself when cleaning up child processes for any test that uses
@create_new_process_for_each_test("fork"):It looks like the
SIG_IGNisn't working for some reason and, maddeningly, I haven't been able to isolate it with a more simple reproducer. Also, of course, this doesn't seem to be happening in thedocker run --initenvironment in CI.While it would be good to understand exactly what's going on, it seems pretty clear that a simple fix is to put the child process in its own process group, and terminate that ... rather than terminating the parent and relying on
SIG_IGN.#23795 and #7054 (#7053) are two relevant past PRs.
/cc @njhill @youkaichao