Skip to content

fix: serialize concurrent vLLM engine startup - #282

Merged
Yunnglin merged 4 commits into
mainfrom
feat/dashserving-adapter
Sep 14, 2026
Merged

Yunnglin merged 4 commits into
mainfrom
feat/dashserving-adapter

Conversation

@Yunnglin

@Yunnglin Yunnglin commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • serialize concurrent vLLM engine initialization within a pod to avoid the vLLM 0.21 get_open_port() race that caused one TP=2 sampler replica to fail with EADDRINUSE

Validation

  • conda run -n twinkle python -m pytest tests/sampler/test_vllm_startup_lock.py -q
  • pre-commit run --files src/twinkle/sampler/vllm_sampler/vllm_sampler.py tests/sampler/test_vllm_startup_lock.py

Deployment note

The vLLM lock is local to a pod and held only while an engine reaches ready state. It does not alter Ray or Megatron process-group ports, GPU allocation, sampling request concurrency, or DashServing-related code.

@Yunnglin
Yunnglin force-pushed the feat/dashserving-adapter branch from 162d5da to 992349b Compare September 14, 2026 03:04
@Yunnglin Yunnglin changed the title feat: add DashServing adapter and stabilize vLLM sampler startup fix: serialize concurrent vLLM engine startup Sep 14, 2026
@Yunnglin
Yunnglin marked this pull request as ready for review September 14, 2026 03:13
Copilot AI lite review requested due to automatic review settings September 14, 2026 03:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The lock scope and test synchronization need correction before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request serializes concurrent vLLM engine startup within a pod and adds regression coverage for cross-process locking.

Changes:

  • Adds a pod-local startup lock around vLLM engine initialization.
  • Adds a multiprocessing lock-serialization test.
File summaries
File Review summary
tests/sampler/test_vllm_startup_lock.py The test may not reliably prove the second process attempted lock acquisition.
src/twinkle/sampler/vllm_sampler/vllm_sampler.py Model downloading occurs while the startup lock is held, broadening the critical section.
Review details

Suppressed comments (1)

src/twinkle/sampler/vllm_sampler/vllm_sampler.py:130

  • This lock is acquired before VLLMEngine construction, but VLLMEngine.__init__ calls HubOperation.download_model(model_id) before creating the vLLM engine. Consequently, a slow or failed hub/filesystem download holds the pod-wide mutex and blocks every other sampler startup, so the lock is not limited to the port-sensitive engine startup described in the deployment note. Resolve/cache the model before taking the lock, or narrow the critical section to the AsyncLLM creation itself.
        with _vllm_engine_startup_lock():
            self.engine: VLLMEngine = self._run_in_loop(self._create_engine_async(VLLMEngine, model_id, engine_kwargs))
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/sampler/test_vllm_startup_lock.py
@Yunnglin
Yunnglin merged commit 6cc9657 into main Sep 14, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants