Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V1] Simplify Shutdown #11659

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

robertgshaw2-neuralmagic
Copy link
Collaborator

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic commented Dec 31, 2024

SUMMARY:

  • objects that for manage background processes or IPC implements shutdown and weakref.finalize
  • remove use of __del__ (resolves weird logs when LLM is cleaned up
  • remove need for higher level classes like LLM, LLMEngine, or AsyncLLM to need to do anything special for shutdown, since all the objects responsible for managing their IPC and MP resources handle their own shutdown

Copy link

👋 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 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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic changed the title [V1] Overhaul Clean Shutdown [V1] Clean Shutdown Dec 31, 2024
@robertgshaw2-neuralmagic
Copy link
Collaborator Author

@WoosukKwon - this solves the logs you were seeing

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic changed the title [V1] Clean Shutdown [V1] Simplify Shutdown Dec 31, 2024
Copy link
Collaborator

@tlrmchlsmth tlrmchlsmth left a comment

Choose a reason for hiding this comment

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

LGTM

Note: I'm seeing some shared memory leaks when running examples/offline_inference.py with tensor_parallel_size=2 that we should fixup:

/usr/lib/python3.10/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 1 leaked shared_memory objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '

(Not seeing this when using vllm serve or when running with tensor_parallel_size 1)

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 31, 2024
@mergify mergify bot added the frontend label Dec 31, 2024
@robertgshaw2-neuralmagic robertgshaw2-neuralmagic enabled auto-merge (squash) January 1, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend 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.

3 participants