[FIX] Suppression of stacktrace on a shutdown

[wallashss]

This PR adds a workaround to suppress logs that are dumped at the shutdown of the engine (only on V1) when vllm runs with multiprocess. The undesired behavior happens because g3log from Spyre stack/runtime overrides the signal handler from vLLM when it starts a process for the engine code. Therefore, the engine does not have a chance to gracefully shutdown.

[github-actions]

👋 Hi! Thank you for contributing to vLLM support on Spyre.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, first install the linting requirements, then run format.sh and commit the changes. This can be done with uv directly:

uv sync --frozen --group lint --active --inexact

Or this can be done with pip:

uv pip compile --group lint > requirements-lint.txt
pip install -r requirements-lint.txt
bash format.sh

Now you are good to go 🚀

[wallashss]

bot:test
TEST_FILE=tests/e2e/test_spyre_basic.py MARKERS="spyre"

[joerunde]

I've had a bit of experience messing with signal handlers, and the annoying problem is that setting a handler with signal.signal(sig, handler) simply overrides any handler that was previously registered for that signal. One thing I worked on back on caikit was a signal handler that would iteratively call all handlers registered for that signal, instead of just blanket overriding whatever the current handler is: https://github.com/caikit/caikit/blob/ce3fa2c129ce15a5e2095d466a8f01ec2e0c577d/caikit/runtime/server_base.py#L179
But of course, all handlers for a signal had to be added with that utility :(

Maybe we can use a similar strategy here though, to setup a handler that calls the vllm one, plus whatever is already registered:

def make_handler(signum):
    original_hander = signal.getsignal(signum)
    def handler(signal_, stack_):
        vllm.run_engine_core.signal_handler(signal_, stack_)
        original_hander(signal_, stack_)

    return handler

...

signal.signal(SIGTERM, make_handler(SIGTERM))

However, if we think that the one set by the g3log is not required for proper shutdown, then we should probably just override it to the vllm one and call it a day

[tjohnson31415]

I did some investigation. As I understand it, Python's getsignal would only work to see what handlers have been registered with Python. But g3log is a C++ library, so Python knows nothing about its signal registration. Other than re-compiling g3log or writing a small C++ wrapper, I don't think we can call out to its signal handler or changes its behavior.

So I think the change here to override the signal handler is a good fix. Also, the change to the handler happens after the warmup, so the g3log handler is still be active during the compliation phase when we would want to have the low-level stack trace if there is a crash.

[wallashss]

Thanks @joerunde for the feedback and suggestions. Pretty cool ideas.

However, if we think that the one set by the g3log is not required for proper shutdown, then we should probably just override it to the vllm one and call it a day

I guess it is more like that. What I observed is that g3log is overriding the signals, the original signal handler of vllm is completely overwritten and it is never called, so I pretty sure that would do to any signals as well... I wrote this PR more thinking as workaround than an ideal solution. I think an ideal solution would be setup g3log to not override other signals. Unfortunately, I did not saw in g3log docs or code ways to setup the signal handler indirectly like using environment variables. It looks like this should be done by the application itself.

While I appreciate your solution, I don't think we'll have extra benefits, because g3log have already clean up everything.

[wallashss]

@tjohnson31415 just share some thoughts at the same time as me 😄 . Thanks!

[tjohnson31415]

This is currently setting the value of VLLM_SPYRE_OVERRIDE_SIGNALS_HANDLER to the string "0" which is truthy, and it would require an empty env var for a falsey value.

Also, the default should be to have a graceful shutdown, i.e. enable the override.

Suggested change

	lambda: os.getenv("VLLM_SPYRE_OVERRIDE_SIGNALS_HANDLER", "0"),
	lambda: bool(int(os.getenv("VLLM_SPYRE_OVERRIDE_SIGNALS_HANDLER", "1"))),

[wallashss]

Ok.

I set first disabled as default, because the libraries that installed g3log might need it to dump their log. However, there are other signals that we do not touch that might happen in runtime errors. So for the SIGTERM and SIGINT, which are for graceful shutdown, the override would be fine.

[tjohnson31415]

This PR took me down a bit of a rabbit hole on signal handling 😅

Great fix though. LGTM

[joerunde]

Ah, yeah TIL about the C++ vs Python signal handling 🤦

LGTM!