Skip to content

Conversation

@chaunceyjiang
Copy link
Collaborator

@chaunceyjiang chaunceyjiang commented Aug 12, 2025

Essential Elements of an Effective PR Description Checklist

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

FIX #22638 (comment)

Purpose

For some PD Connectors, it is necessary to perform certain cleanup actions, such as closing connections, when shutting down D.

Test Plan

Test Result

(Optional) Documentation Update

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 introduces a shutdown method to the KVConnector API, aimed at ensuring proper resource cleanup during shutdown. The implementation registers this new method with Python's atexit module to be called on process exit. While this addresses the need for cleanup, relying on atexit can be unreliable in some scenarios and may cause issues in complex applications. My review highlights this potential issue and suggests integrating the shutdown logic into vLLM's existing explicit shutdown sequence for better robustness.

@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@chaunceyjiang
Copy link
Collaborator Author

/cc @njhill PTAL

Copy link
Collaborator

@KuntaiDu KuntaiDu left a comment

Choose a reason for hiding this comment

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

LGTM, but also we need to let @njhill take a look.

@chaunceyjiang
Copy link
Collaborator Author

@NickLucche PTAL.

Copy link
Collaborator

@NickLucche NickLucche left a comment

Choose a reason for hiding this comment

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

looking cleaner thanks!

@panpan0000
Copy link
Contributor

@KuntaiDu will diff connector like LMCache need to implement their own shutdown()?

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chaunceyjiang, please see inline comments.

In addition I think we also need to call the executor shutdown method from EngineCore.shutdown() in core.py.

@njhill
Copy link
Member

njhill commented Sep 2, 2025

@KuntaiDu will diff connector like LMCache need to implement their own shutdown()?

@panpan0000 they don't need to but can do if it makes sense.

@njhill
Copy link
Member

njhill commented Sep 2, 2025

@chaunceyjiang another missing link is that the multiproc worker proc should call shutdown() in its handling of death_pipe.

What might be better there instead of calling os.kill is to put a sentinel message on the input queue and use a shutdown event in the dequeue() method of the worker busy loop. The death_pipe logic can call this instead, and then that loop can call self.shutdown() before it exits.

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 7, 2025
@chaunceyjiang
Copy link
Collaborator Author


packages/vllm/distributed/device_communicators/cuda_communicator.py", line 52, in __init__
--
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]     self.pynccl_comm = PyNcclCommunicator(
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]                        ^^^^^^^^^^^^^^^^^^^
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]   File "/usr/local/lib/python3.12/dist-packages/vllm/distributed/device_communicators/pynccl.py", line 106, in __init__
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]     self.all_reduce(data)
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]   File "/usr/local/lib/python3.12/dist-packages/vllm/distributed/device_communicators/pynccl.py", line 127, in all_reduce
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]     self.nccl.ncclAllReduce(buffer_type(in_tensor.data_ptr()),
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]   File "/usr/local/lib/python3.12/dist-packages/vllm/distributed/device_communicators/pynccl_wrapper.py", line 314, in ncclAllReduce
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]     self.NCCL_CHECK(self._funcs["ncclAllReduce"](sendbuff, recvbuff, count,
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]   File "/usr/local/lib/python3.12/dist-packages/vllm/distributed/device_communicators/pynccl_wrapper.py", line 272, in NCCL_CHECK
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588]     raise RuntimeError(f"NCCL error: {error_str}")
  | [2025-09-07T21:33:01Z] (EngineCore_0 pid=278) (VllmWorker pid=299) ERROR 09-07 14:33:01 [multiproc_executor.py:588] RuntimeError: NCCL error: unhandled cuda error (run with NCCL_DEBUG=INFO for details)


Hi @DarkLight1337, Could you please help re-run the e2e test?

@vllm-bot vllm-bot merged commit 61aa4b2 into vllm-project:main Sep 8, 2025
48 of 51 checks passed
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
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 tpu Related to Google TPUs v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants