[Frontend] Kill the server on engine death #6594

joerunde · 2024-07-19T21:11:20Z

Slight mitigation for #5901

This PR updates the open ai server to check for cases where the AsyncLLMEngine errors, and terminate itself when it does.

As I understand it, the goal of stopping the AsyncLLMEngine on unknown errors and not restarting is to prevent error loops where we try to restart the engine after an error that's really unrecoverable. However when we run into these issues in production, the server has to sit there responding with errors to all requests until a readiness probe fails, and then wait until the liveness probes fail enough times to trigger a container restart. This results longer downtime than necessary when we can instead terminate the server as soon as we know that the engine has failed.

This is implemented as a sigterm sent to the current PID which allows the uvicorn server to handle the signal and enter the usual graceful shutdown path.

There are two exception handlers added to the server. The first one checks for anything that extends RuntimeError, which I'm assuming will match most exceptions that kill the engine. This handler checks to see if the engine is dead, and kills the server if it is.
The second handler matches the AsyncEngineDeadError explicitly and immediately kills the server, to catch any cases that were not caught by the first handler. This could happen if the engine was shared with another module that accidentally killed it (not speaking from experience here or anything 😉)

PR Checklist (Click to Expand)

Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

[Bugfix] for bug fixes.
[CI/Build] for build or continuous integration improvements.
[Doc] for documentation fixes and improvements.
[Model] for adding a new model or improving an existing model. Model name should appear in the title.
[Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
[Kernel] for changes affecting CUDA kernels or other compute kernels.
[Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
[Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
[Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

We adhere to Google Python style guide and Google C++ style guide.
Pass all linter checks. Please use format.sh to format your code.
The code need to be well-documented to ensure future contributors can easily understand the code.
Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Notes for Large Changes

Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.

What to Expect for the Reviews

The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
After the review, the reviewer will put an action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You

Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

github-actions · 2024-07-19T21:11:30Z

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

Comment /ready on the PR
Add ready label to the PR
Enable auto-merge.

🚀

joerunde · 2024-07-19T21:13:10Z

vllm/engine/async_llm_engine.py

@@ -56,7 +56,7 @@ def _log_task_completion(task: asyncio.Task,
        error_callback(exception)
        raise AsyncEngineDeadError(
            "Task finished unexpectedly. This should never happen! "
-            "Please open an issue on Github. See stack trace above for the"
+            "Please open an issue on Github. See stack trace above for the "


(This has just triggered me every time I see the log)

Yard1 · 2024-07-19T22:26:29Z

vllm/entrypoints/openai/api_server.py

+    @app.exception_handler(RuntimeError)
+    async def runtime_error_handler(_, __):
+        """On generic runtime error, check to see if the engine has died.
+        It probably has, in which case the server will no longer be able to
+        handle requests. Trigger a graceful shutdown with a SIGTERM."""
+        if engine.errored and not engine.is_running:
+            logger.fatal("AsyncLLMEngine has failed, terminating server "
+                         "process")
+            os.kill(os.getpid(), signal.SIGTERM)
+
+        return Response(status_code=HTTPStatus.INTERNAL_SERVER_ERROR)
+
+    @app.exception_handler(AsyncEngineDeadError)
+    async def engine_dead_handler(_, __):
+        """Kill the server if the async engine is already dead. It will
+        not handle any further requests."""
+        logger.fatal("AsyncLLMEngine is already dead, terminating server "
+                     "process")
+        os.kill(os.getpid(), signal.SIGTERM)
+
+        return Response(status_code=HTTPStatus.INTERNAL_SERVER_ERROR)
+


I would recommend gating this behavior behind an env var/argument, since automatic process death may be unexpected.

Why not just raise a normal exception, too? calling os.kill on self seems like an anti-pattern.

+1 I can gate that for sure. But I do think it should be the default behavior for running the server, I don't really see the utility in leaving it up and unresponsive if we know the engine is dead.

Why not just raise a normal exception, too?

In this case raising an exception isn't gonna kill the server, an unhandled exception in an endpoint handler will just cause the server to respond to the request with an empty 500 (which is also what I'm explicitly doing here). The main process is waiting on the server to exit, so the server has to be notified to exit somehow. AFAIK the canonical way of doing this when using uvicorn.run(...) like we are is to use a termination signal. I agree that looks funky here though, It might be possible instead to do something like

server = uvicorn.Server(...) server.run()

in our run_server and then use server.shutdown() in these handlers to try to initiate a shutdown that will then cause the main process to return from server.run() and exit. WDYT?

I think that sounds cleaner, but I don't have much experience with uvicorn, so take that with a grain of salt :)

I think it's ok if death is the default, but we should add a way to turn it off

Okay, I think I'm happy with the server shutdown code now, and I added a flag to gate this logic.

I'm noodling on some tests and going down the route of running run_server() in a thread with the llm engine patched to raise an error on each request, but it is looking a bit funky. Any other suggestions?

I think it's ok for tests

Another approach could be scheduling a periodic task that checks the engine health and stops the http server task when the engine is unhealthy. See #6740

@dtrifiro true, though I think in this case we can avoid a lot of unnecessary checking since we catch the errors as they happen

dtrifiro · 2024-07-25T17:13:05Z

vllm/entrypoints/openai/api_server.py

+    server = uvicorn.Server(config=uvicorn_config)
+
+    # Run the server and block until it exits
+    with contextlib.suppress(KeyboardInterrupt):


Why is this needed?

Your changes overrode this- but this was here to stop an extra error from being printed when a user interrupts the server with CTRL+C. The uvicorn shutdown handlers still handle the signal properly, but the error would raise from here and be unhandled.

I think your changes to catch the server task being cancelled would handle this as well.

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-07-26T15:46:26Z

@Yard1 I banged my head against a wall for a few hours trying to unit test this without massively refactoring the server or injecting a poison pill into the engine to trigger an error, and I'm ready to give up 😦. Do you mind merging as-is without tests?

Yard1 · 2024-07-26T18:41:04Z

@joerunde can you outline what you tried to do?

Signed-off-by: Joe Runde <[email protected]>

vrdn-23 · 2024-08-01T16:53:51Z

@joerunde @Yard1 Is this PR good to merge? We've been experiencing some issues lately where the engine dies but the server is still up and it hence doesn't restart the Kubernetes pod on its own. I'm hoping this fix will help us mitigate this issue!

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-08-01T21:57:06Z

@Yard1 @vrdn-23 Sorry for the delay, been focusing on #6883 instead.

I went ahead and just wrote a simple test that uses a bad lora adapter to crash the server, which should at least work until that particular bug is fixed.

I also tried both:

Subclassing RemoteOpenAIServer to use fork and then mock.patching out the real model loads and injecting an exception into the LLMEngine, but just ran into too many small issues to debug
Running the equivalent of vllm serve in a separate thread to be able to mock out the llm engine in-process, but that turned into 150+ lines of code to maintain for this one little test which I also did not want to foist upon the rest of the maintainers

I see there were some other CI failures, I'll see if I can repro them or see if a rebase fixes it

njhill

Thanks @joerunde!

njhill · 2024-08-01T23:03:51Z

vllm/entrypoints/openai/cli_args.py

@@ -134,6 +134,11 @@ def make_arg_parser(parser: FlexibleArgumentParser) -> FlexibleArgumentParser:
        help="When --max-logprobs is specified, represents single tokens as"
        "strings of the form 'token_id:{token_id}' so that tokens that"
        "are not JSON-encodable can be identified.")
+    parser.add_argument("--keep-alive-on-engine-death",


Only minor but I wonder if this would be better as an env var since I think it would only be used in debugging scenarios...

Ah, yeah I can change that. I only recently learned that there's an explicit split between the cli args and vllm.envs

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-08-02T17:58:57Z

@Yard1 It looks like the entrypoints tests pass, but other tests are failing with timeouts and it looks like I'd need to rebase main in order for the tracing tests to pass.

Do you want me to rebase and re-trigger the CI? I don't want to use up too many CI runs for this one little PR

Yard1 · 2024-08-05T21:42:08Z

@joerunde yeah let's fix the conflicts

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-08-06T22:55:45Z

Cool, yeah with the addition of the common launcher I went ahead and moved these exception handlers there so that they can have access to the server to interrupt it, and I also had to fill out the rest of the interface for the new AsyncEngineRPCClient

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-08-07T22:07:12Z

Added a little bugfix for an edge case I ran into after https://github.com/vllm-project/vllm/pull/7111/files#diff-4bd825485b5162cf8021da41a2ebd3d4026929e617d1137f69bbbe4bda0ee643R208

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2024-08-08T15:25:38Z

@Yard1 @njhill I think we're up to date with ci passing, mind merging?

njhill · 2024-08-08T16:47:17Z

vllm/entrypoints/openai/rpc/server.py

+            # Abort the request in the llm engine.
+            await self.engine.abort(request.request_id)
+        except Exception:
+            logger.warning("Failed to abort request %s", request.request_id)


It might be good for this to be logger.exception(...) so that the stacktrace is logged.

But don't want to trigger another whole round of CI tests just for this, we can address it in follow-on cleanup of the zmq decoupling that's being done anyhow.

njhill · 2024-08-08T16:47:39Z

Thanks again @joerunde!

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Alvant <[email protected]>

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

joerunde commented Jul 19, 2024

View reviewed changes

Yard1 reviewed Jul 19, 2024

View reviewed changes

joerunde force-pushed the kill-the-server branch from be9908f to 99e54d3 Compare July 22, 2024 23:03

joerunde marked this pull request as ready for review July 22, 2024 23:04

Yard1 approved these changes Jul 22, 2024

View reviewed changes

Yard1 enabled auto-merge (squash) July 22, 2024 23:09

github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 22, 2024

Yard1 disabled auto-merge July 22, 2024 23:13

joerunde mentioned this pull request Jul 24, 2024

🥅 Kill servers on engine death opendatahub-io/vllm-tgis-adapter#63

Merged

3 tasks

dtrifiro reviewed Jul 25, 2024

View reviewed changes

joerunde and others added 4 commits July 26, 2024 09:39

🥅 kill server on engine death

0df9465

Signed-off-by: Joe Runde <[email protected]>

🚩 gate automatic shutdown

0f8ffc9

Signed-off-by: Joe Runde <[email protected]>

♻️ shutdown server programatically

6c20c20

Signed-off-by: Joe Runde <[email protected]>

🎨 format

fc386c4

Signed-off-by: Joe Runde <[email protected]>

joerunde force-pushed the kill-the-server branch from 99e54d3 to fc386c4 Compare July 26, 2024 15:41

🐛 fix server ref

d94a88b

Signed-off-by: Joe Runde <[email protected]>

✅ add shutdown test

1537c9d

Signed-off-by: Joe Runde <[email protected]>

njhill approved these changes Aug 1, 2024

View reviewed changes

joerunde added 2 commits August 2, 2024 10:15

🐛 fixup shutdown test for CI

ecd88b3

Signed-off-by: Joe Runde <[email protected]>

♻️ move cli arg to envs

cff5a19

Signed-off-by: Joe Runde <[email protected]>

vrdn-23 mentioned this pull request Aug 5, 2024

v0.5.2, v0.5.3, v0.6.0 Release Tracker #6434

Closed

7 tasks

joerunde added 2 commits August 6, 2024 14:44

Merge remote-tracking branch 'upstream/main' into kill-the-server

bb459bc

♻️ move shutdown handling to launcher

214585f

Signed-off-by: Joe Runde <[email protected]>

Merge remote-tracking branch 'upstream/main' into kill-the-server

b846a86

Yard1 enabled auto-merge (squash) August 7, 2024 19:31

joerunde added 2 commits August 7, 2024 15:48

🐛 fix abort bug

bd491bb

Signed-off-by: Joe Runde <[email protected]>

🐛 add exception handling to abort in rpc server

8e34011

Signed-off-by: Joe Runde <[email protected]>

auto-merge was automatically disabled August 7, 2024 22:06
Head branch was pushed to by a user without write access

joerunde added 2 commits August 7, 2024 16:08

🎨 fmt

f98b441

Signed-off-by: Joe Runde <[email protected]>

🐛 fix log line

34a46ec

Signed-off-by: Joe Runde <[email protected]>

njhill reviewed Aug 8, 2024

View reviewed changes

njhill merged commit 21b9c49 into vllm-project:main Aug 8, 2024
51 checks passed

joerunde deleted the kill-the-server branch August 8, 2024 16:48

sfc-gh-mkeralapura pushed a commit to sfc-gh-mkeralapura/vllm that referenced this pull request Aug 12, 2024

[Frontend] Kill the server on engine death (vllm-project#6594)

392efb3

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

kylesayrs pushed a commit to neuralmagic/vllm that referenced this pull request Aug 17, 2024

[Frontend] Kill the server on engine death (vllm-project#6594)

e623c9d

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

ywang96 mentioned this pull request Aug 18, 2024

[Feature]: Exit on failures #7633

Closed

fialhocoelho pushed a commit to opendatahub-io/vllm that referenced this pull request Aug 22, 2024

[Frontend] Kill the server on engine death (vllm-project#6594)

f22063a

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

flawaetz mentioned this pull request Sep 4, 2024

[Bug]: When use guided choice feature, vllm.engine.async_llm_engine.AsyncEngineDeadError #8100

Open

1 task

Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024

[Frontend] Kill the server on engine death (vllm-project#6594)

01722b8

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Alvant <[email protected]>

KuntaiDu pushed a commit to KuntaiDu/vllm that referenced this pull request Nov 20, 2024

[Frontend] Kill the server on engine death (vllm-project#6594)

bda6baf

Signed-off-by: Joe Runde <[email protected]> Signed-off-by: Joe Runde <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend] Kill the server on engine death #6594

[Frontend] Kill the server on engine death #6594

joerunde commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024

joerunde Jul 19, 2024

Yard1 Jul 19, 2024

joerunde Jul 19, 2024

Yard1 Jul 19, 2024 •

edited

Loading

joerunde Jul 22, 2024

Yard1 Jul 22, 2024

dtrifiro Jul 25, 2024 •

edited

Loading

joerunde Jul 26, 2024

dtrifiro Jul 25, 2024

joerunde Jul 26, 2024

joerunde commented Jul 26, 2024

Yard1 commented Jul 26, 2024

vrdn-23 commented Aug 1, 2024

joerunde commented Aug 1, 2024 •

edited

Loading

njhill left a comment

njhill Aug 1, 2024

joerunde Aug 2, 2024

joerunde commented Aug 2, 2024

Yard1 commented Aug 5, 2024

joerunde commented Aug 6, 2024

joerunde commented Aug 7, 2024

joerunde commented Aug 8, 2024

njhill Aug 8, 2024

njhill commented Aug 8, 2024

[Frontend] Kill the server on engine death #6594

[Frontend] Kill the server on engine death #6594

Conversation

joerunde commented Jul 19, 2024 • edited Loading

PR Title and Classification

Code Quality

Notes for Large Changes

What to Expect for the Reviews

Thank You

github-actions bot commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yard1 Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtrifiro Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joerunde commented Jul 26, 2024

Yard1 commented Jul 26, 2024

vrdn-23 commented Aug 1, 2024

joerunde commented Aug 1, 2024 • edited Loading

njhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joerunde commented Aug 2, 2024

Yard1 commented Aug 5, 2024

joerunde commented Aug 6, 2024

joerunde commented Aug 7, 2024

joerunde commented Aug 8, 2024

Choose a reason for hiding this comment

njhill commented Aug 8, 2024

joerunde commented Jul 19, 2024 •

edited

Loading

Yard1 Jul 19, 2024 •

edited

Loading

dtrifiro Jul 25, 2024 •

edited

Loading

joerunde commented Aug 1, 2024 •

edited

Loading