Skip to content

Conversation

@markmc
Copy link
Member

@markmc markmc commented Nov 21, 2025

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.

#23795 and #7054 (#7053) are two relevant past PRs.

/cc @njhill @youkaichao

…"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 <[email protected]>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

@markmc markmc added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 21, 2025
@markmc markmc requested review from njhill and youkaichao November 21, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant