Skip to content

fix(server): enable concurrent Tinker and Twinkle sampling - #303

Merged
Yunnglin merged 5 commits into
modelscope:mainfrom
rycerzes:fix/297-concurrent-tinker-sample
Oct 14, 2026
Merged

Yunnglin merged 5 commits into
modelscope:mainfrom
rycerzes:fix/297-concurrent-tinker-sample

Conversation

@rycerzes

@rycerzes rycerzes commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #297: ordinary Tinker and Twinkle sampling requests reach vLLM concurrently instead of waiting in the serial training compute queue.

  • Reuse ComputeWorker admission, persisted futures, timeouts and error mapping; retain capacity until an operation actually exits. A timeout fails that request and allows other stable-weight requests to continue.
  • Remove per-request template setup and prefix-cache reset. Initialize native output templates once; Tinker consumes encoded tokens independently of native template settings.
  • Keep full-checkpoint load plus generation exclusive with shared-state mutations. Validate checkpoint files before writing; only uncertain weight writes fail the Serve health check and trigger replica replacement. Ordinary request failures do not disable the replica.
  • Await sibling cleanup after batch failure. Only the server's async backend opts into concurrent actor calls; native async RL retains its concurrency defaults and cancellation APIs.

Scope

Based on main ec8fd1f0. Streaming lifecycle cleanup and LoRA-enabled full-weight reload compatibility remain local and unpushed. The stream engine, full-weight worker extension and streaming bridge match main. Same-checkpoint shared generation remains a separate optimization.

No documentation changes. Coverage is grouped into queue, sampler behavior, HTTP route and real E2E tests. The latest cleanup reduces the PR from 31 to 23 files and removes one obsolete access-context test plus one duplicate pre-start shutdown test; all other moved assertions and parameterization are preserved. This cleanup does not change runtime source.

concurrent_task_limit=128 bounds admitted unfinished requests, not simultaneous engine execution, which also depends on executor, actor and vLLM configuration. Training remains serial.

Validation

Current head: addab02051e8cf55faa849e2a86145cfb2892e7e.

  • Local affected regression: 721 passed, 78 skipped, 0 failures/errors. Skips include hardware/external-service tests; 13 Redis tests passed in the prior run but Redis was unavailable for this cleanup run. Ruff F checks on reorganized tests and diff checks pass.
  • Runtime source is byte-for-byte unchanged from e39acd6266e7ba5c958784471d502e824204710f, which has 22 real PPU E2E passes across both SDKs, 1/8/32 concurrency, mapping, mixed clients, LoRA isolation, full-checkpoint isolation, invalid checkpoint recovery, timeout ownership and DataPlane. A separate real Serve replacement/recovery test passed. Actor SHA/source hashes and zero generation/job counters were verified; owned services and ports were cleaned.
  • Native async RL defaults have CPU coverage; training, TP>1 and the deferred repairs were not exercised by the PPU run. No new PPU run was performed for this test/documentation-only cleanup.
  • The prior head's citest failure was solely existing GitHub documentation links returning HTTP 503 (1785 passed, 1 failed, 115 skipped). Rerunning the unchanged head passed. Current-head citest and lint passed. citest-npu remains queued; its result is still unverified.

Benchmark

Qwen3-1.7B, 128 tokens/request, PPU ZW810, DP=2 / TP=1, vLLM 0.20.1+PPU:

Concurrency Tinker tok/s Twinkle tok/s
1 239 221
8 639 913
32 2918 2884

The issue's single RTX 4090 measurements (24/27/26 tok/s, vLLM 0.20.2) use a different environment. Absolute throughput is not directly comparable; these workload-specific results show scaling with concurrent requests.

@Yunnglin
Yunnglin marked this pull request as draft October 13, 2026 00:39
@Yunnglin Yunnglin changed the title fix(server): batch concurrent Tinker sample requests in vLLM fix(server): concurrent Tinker and Twinkle sampling with safe cleanup Oct 13, 2026
@Yunnglin
Yunnglin force-pushed the fix/297-concurrent-tinker-sample branch from 858be8c to c84e633 Compare October 13, 2026 13:11
@Yunnglin Yunnglin changed the title fix(server): concurrent Tinker and Twinkle sampling with safe cleanup fix(server): enable concurrent Tinker and Twinkle sampling Oct 13, 2026
@Yunnglin
Yunnglin marked this pull request as ready for review October 14, 2026 09:12
Copilot AI balanced review requested due to automatic review settings October 14, 2026 09:12

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

A cancellation race can execute an already-cancelled task, and an E2E actor lookup uses the obsolete class name.

2 open findings
What changed in this PR

Enables concurrent Tinker and Twinkle sampling while coordinating sampler mutations and checkpoint loads.

Changes:

  • Adds concurrent compute admission, lifecycle tracking, and sampler read/write coordination.
  • Adds token-only sampling, safer checkpoint loading, and server-specific vLLM concurrency.
  • Expands unit and E2E concurrency, timeout, cancellation, and recovery coverage.
File Description
tests/​twinkle_agentic/​test_vllm_sampler_tq_generation.py Updates generation timeout tests.
tests/​server/​utils/​test_task_queue_mixin.py Tests concurrent task lifecycle and capacity.
tests/​server/​sampler/​test_twinkle_async_rows.py Adapts DataPlane tests to concurrent submission.
tests/​server/​sampler/​test_tinker_handlers.py Tests token-only concurrent Tinker sampling.
tests/​server/​sampler/​test_stream_guarantees.py Moves timeout enforcement to the caller.
tests/​server/​sampler/​test_sampling_concurrency.py Covers locking, isolation, timeout, and recovery.
tests/​server/​sampler/​test_resolve_sampler_weights.py Updates checkpoint validation coverage.
tests/​server/​sampler/​test_mock_sampler.py Verifies server-specific actor concurrency.
tests/​server/​integration/​test_sampler_concurrency_e2e.py Adds real concurrent sampling E2E coverage.
tests/​sampler/​test_vllm_sampling.py Tests token-only sampling and checkpoint safety.
src/​twinkle/​server/​task_queue/​worker.py Adds detached concurrent compute execution.
src/​twinkle/​server/​task_queue/​types.py Centralizes backend timeout exceptions.
src/​twinkle/​server/​task_queue/​mixin.py Exposes concurrent scheduling APIs.
src/​twinkle/​server/​task_queue/​config.py Adds concurrent-task capacity configuration.
src/​twinkle/​server/​sampler/​weights.py Coordinates checkpoint loading and generation.
src/​twinkle/​server/​sampler/​twinkle_handlers.py Routes Twinkle sampling concurrently.
src/​twinkle/​server/​sampler/​tinker_handlers.py Routes Tinker sampling concurrently.
src/​twinkle/​server/​sampler/​concurrency.py Implements sampler reader/writer coordination.
src/​twinkle/​server/​sampler/​backends/​vllm_async.py Adds the server-specific concurrent sampler.
src/​twinkle/​server/​sampler/​app.py Initializes concurrency and template state.
src/​twinkle/​sampler/​vllm_sampler/​vllm_sampler.py Adds token-only mode and safer batch cleanup.
src/​twinkle/​exceptions.py Defines partial weight-load failure signaling.
src/​twinkle_agentic/​async_rl/​vllm_sampler_tq.py Reuses sibling-cleanup gathering.

🧠 Review effort: Balanced


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

Comment thread src/twinkle/server/task_queue/worker.py
Comment thread tests/server/integration/test_sampler_concurrency_e2e.py Outdated
A client cancel can win the race against the running status write. The terminal record is kept, but the task still started and held detached capacity. Re-read the record after the write and return before starting work.

Also default the E2E sampler actor lookup to ServerVLLMSampler.
@Yunnglin
Yunnglin merged commit 9953ea1 into modelscope:main Oct 14, 2026
2 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.

[Tinker compat] Concurrent sample requests are serialized, so vLLM never batches them (~25 tok/s at any concurrency)

4 participants