Skip to content

sampling: sample through pools and delete the queue sampler - #339

Closed
ShubyM wants to merge 4 commits into
openrl-dev:mainfrom
ShubyM:feat/sampling-pools-switch
Closed

ShubyM wants to merge 4 commits into
openrl-dev:mainfrom
ShubyM:feat/sampling-pools-switch

Conversation

@ShubyM

@ShubyM ShubyM commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #337; review the last two commits.

Sampling now goes through pools, and the in-process queue sampler (server.vllm_sampler) is deleted. SAMPLING_BACKEND is torch or vllm, and vllm means pools. There is no flag to keep both paths alive; the TPU branch will catch up with main.

What a pool is

  • vllm serve replicas, one Workload each, plus one CPU pod running the dispatcher, Envoy and the llm-d EPP.
  • A replica is Ready only once it holds the pool's weight version, so the router never sends to stale weights.
  • Each LoRA save is frozen into sampler_lora/<sha256(ref)>, and vLLM loads it by name.
  • Gone with the queue sampler: the Redis sampling list, the sampler_ready handshake and the shutdown sentinel.

The second commit holds the review fixes

  • Sampler status. A sampler reads ready only once its pod is Ready and its pool's dispatcher has a pod.
  • Clear errors and retries. The server refuses to start without REDIS_URL, accepting a sample retries NFS lookups for up to 2 s, and a fresh install no longer hits the RBAC subject bug.
  • One vLLM compile cache per process. Two pods could otherwise corrupt the shared one.
  • One tmp dir. The probe, the dispatcher and the worker pods all fall back to /tmp/open-rl, locally and on the cluster.
  • Local mode. Its dispatcher restarts along with its vLLM, and the LoRA snapshot folder exists before a LoRA pool starts.
  • vLLM's 0.90 memory fraction is back. Pending prompts are held compactly in the dispatcher.

Known limitations. Every pool sampler has its own GPU, so an FFT job takes two until #342 lands. Under sparse-delta sync, an FFT replica that restarts or falls behind can't catch up yet; that needs periodic full checkpoints in a follow-up.

Tested with unit tests (303). LoRA and FFT runs on GKE used this pools code before the flag was removed; the removal touches only routing and the deleted queue sampler.

@ShubyM
ShubyM force-pushed the feat/sampling-pools-switch branch 3 times, most recently from 188ac31 to 87f10aa Compare October 16, 2026 22:38
@ShubyM
ShubyM marked this pull request as ready for review October 16, 2026 23:05
@droot

droot commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Reviewed the last commit. This is a big switch and the mechanism holds together: readiness gated on the weight version, EPP only picks ready pods, dispatcher double checks the version on the answer. 22 runs on spot is good evidence. Ran the unit suite locally against a redis, 147 pass. It also closes three of my questions on #336 (the @hex suffix, freeze_adapter wiring, pool names), thanks.

Two things I would want settled before this merges, then smaller items.

1. FFT jobs no longer share a GPU between trainer and sampler

exclusive = not meta.shares_gpu() or role == "sampler" means every FFT job now needs trainer GPU(s) plus sampler GPU(s) full time. The description says this is "not carried over", which reads as permanent. That changes the GPU count for every FFT job and the single-GPU FFT story we have been demoing. The time-slicer and snapshot agent stay in the tree but nothing uses them on the sampler side anymore.

I am fine if this is the direction, but it should be a deliberate call with a short note on what replaces it (sleep/wake on the pool replica? the trainer taking the sampler's GPU between steps?) and what happens to the fft-timeslice deploy and e2e scenarios. Right now it is a one-line aside in a PR that also deletes the old path.

2. Sparse delta still has no way back, and retention now makes it worse

Same point as on #337: under weight_sync_strategy=delta the trainer only writes deltas, and the dispatcher refuses a delta whose parent the replica does not hold. This PR adds drop_old_checkpoints, which deletes everything but the newest 3 (plus pinned). So a replica that restarts, or falls more than 3 saves behind, has no chain to replay even in principle. A spot preemption, which is exactly what you tested on, will hit this.

I think the fix is on the trainer side: write a full checkpoint every K saves or whenever a replica reports a version outside the kept window, and have the dispatcher load full when the parent is missing. Could be a follow-up, but then delta mode should be marked unsafe for pools until it lands.

3. Sampler status is now "ready" while vLLM is still loading

openrl_status drops the sampler_ready announce, so a pool sampler reports ready as soon as its pod is Running, which is minutes before it can serve. We just added the starting/ready distinction in #328 for exactly this. The readiness probe you added is the right signal; can the Workload status (or a pod read) carry the Ready condition so the model view uses it?

Smaller

  • ensure_dispatcher can block up to 180s in to_thread while an old pool's pod terminates, and create_sampling_session waits on it. Fine for now, but worth a log line so a slow session create is explainable.
  • VLLM_CACHE_ROOT and TRITON_CACHE_DIR on the shared NFS volume with several replicas compiling at once: does vLLM lock the cache dir, or can two replicas race on the same artifact? I hit this question on my cold-start branch too, so happy to compare notes.
  • Local mode without REDIS_URL: the dispatcher dies on os.environ["REDIS_URL"] and accept_pool_sample fails on store.redis. A clear "pools need REDIS_URL" error at startup would save someone an hour.
  • The NetworkPolicy blocks everything but the pool pod on 8000. That also blocks any metrics scrape of vLLM. Not a problem today, just noting it.
  • Do we have step-time numbers from the 22 runs against the queue sampler on the same model? The Envoy + EPP hop and the per-answer /weight_info call are on the critical path for every request, so a before/after on fft-gsm8k-rl would be good to have in the PR.

@ShubyM
ShubyM force-pushed the feat/sampling-pools-switch branch from 87f10aa to 44bc7ab Compare October 19, 2026 16:19
@ShubyM ShubyM changed the title sampling: sample through pools and delete the queue sampler sampling: add sampling pools behind SAMPLING_BACKEND=vllm-pools Oct 19, 2026
ShubyM and others added 2 commits October 19, 2026 18:54
The dispatcher sends each pending request through the pool router, Envoy
in front of the llm-d EPP, and commits the first result. A result counts
only if the replica that answered still holds the expected weights and has
not paused to load others since the request was sent. When the pool moves
to a new revision, each replica loads the checkpoint or sparse delta,
resets its prefix cache and resumes. The dispatcher sleeps nowhere: it
waits on the stream for new requests, and wakes at least every second to
retry a failed send. On one machine, OPEN_RL_SAMPLER_ENDPOINTS lists the
replicas and no router is needed. FFT sparse deltas now record the save
they apply on top of, so a replica on any other version loads a
checkpoint instead.

A pool's only dispatcher reads each stream entry once and drops it when it
commits. A Redis or Kubernetes error in a dispatch pass is logged and
retried rather than ending the dispatcher, and commits wait for one of 64
Redis connections, since every in-flight send commits at once when replicas
come up. A request pending longer than OPEN_RL_SAMPLE_DEADLINE_SECONDS
(default one hour, measured from its stream id) fails with the pool and
revision in the error, so a pool that can never serve a revision does not
leave its clients waiting. The replica listing has a 10 s Kubernetes API
timeout.
- Recover replicas a dead dispatcher left paused or inside a weight
  update. A stop or kill during a load left the replica paused while it
  reported the new version, and a kill between start_weight_update and
  finish_weight_update left vLLM refusing every later update. The
  dispatcher resumes a replica left paused, closes an open update with a
  bare finish_weight_update and marks the replica unknown until a full
  load, and on shutdown gives loads in flight five seconds.
- Simplify the dispatcher: inline the deadline check into dispatch, drop
  the OPEN_RL_SAMPLE_DEADLINE_SECONDS knob and a concurrency default
  nothing used, chain raise_for_status, commit inside send's branches
  and flatten load_weights.
- Point an FFT delta at the save its optimizer step started from. The
  parent was the last sampler save, but a delta holds only the last
  optimizer step, so a save the pool never sampled broke the chain for
  every replica, and two optimizer steps between saves silently served
  wrong weights. The trainer lists every save a delta applies on as its
  bases, and writes a full checkpoint when no save holds the weights its
  step started from.
- Free a dispatch slot when its record fails at the deadline. A generate
  had no timeout and the deadline commit left the send running, so a
  replica that never answered kept its slots and starved healthy ones.
  The deadline commit cancels the send, and each send is bounded by the
  time its record has left.
- Keep a replica paused when its load is cut off mid-update. The
  cancelled load's finally resumed the replica inside its open update,
  so it served under its old version with weights that may be partly the
  new revision. Such a load now leaves the replica paused, and on first
  sight a dispatcher closes an open update on every replica, paused or
  not.
- Keep at most 32 idle connections in the dispatcher's HTTP client. It
  kept every connection a burst opened, hundreds of them, and httpx
  scans its whole pool whenever a request starts or ends, so a burst
  held the dispatcher's one loop at a full core. Its 5 s weight checks
  then timed out about 21 s after the send, and finished samples were
  generated again. Sends still have no connection limit.
- Probe a replica for an open weight update only on first sight and
  after one of the dispatcher's own loads breaks off. Each load ended by
  dropping the check, so the pass after every load sent a bare
  finish_weight_update, which vLLM refuses with a logged 500 traceback
  when no update is open.
@ShubyM
ShubyM force-pushed the feat/sampling-pools-switch branch from 44bc7ab to e6ab43c Compare October 19, 2026 18:54
ShubyM and others added 2 commits October 19, 2026 19:57
Every sampler is now a sampling pool replica. On Kubernetes a pool is
vllm serve Workloads plus one CPU Deployment running the dispatcher, Envoy
and the llm-d EPP, with a NetworkPolicy and a readiness probe tied to the
expected weight version. On one machine the local worker manager starts
one vllm serve and a dispatcher that sends to it directly. asample and
create_sampling_session go through the pool accept path, LoRA saves are
frozen under their sampler reference, and checkpoints that pending
requests name are kept. Pool names are derived valid Kubernetes names, so
OPEN_RL_SAMPLER_POOLS is gone. The queue sampler, its Redis sampling queue
and the sampler_ready handshake are deleted.

A save under a frozen sampler name whose adapter is byte-identical to the
snapshot changes nothing, as when a script evaluates twice with no training
between; different weights under a used name still fail, since vLLM caches
adapters by name.

The dispatcher runs on the API-server image when OPEN_RL_DISPATCHER_IMAGE
names it, which the open-rl-images ConfigMap does; its pull from the 7 GB
worker image was the last thing a cold pool waited on. vLLM replicas keep
their torch.compile and Triton caches under OPEN_RL_TMP_DIR/cache on the
shared volume, so a new replica does not compile again.
- Call a pool sampler ready only once its pod is Ready. Its Workload
  reads Running from container start, through vLLM's load and through a
  crash loop, so the model view called it ready minutes before it could
  serve.
- Bind the worker-launcher Role to open-rl-sa, the account the API server
  runs as. open-rl-api-server is created nowhere, so a fresh install could
  not create Workloads, Deployments or NetworkPolicies.
- Refuse to start pool sampling without REDIS_URL, in every launcher
  mode. Before, the dispatcher died on the missing variable and asample
  failed on the in-memory store.
- Wait out NFS lookup caching before refusing a sample. The API server's
  NFS client could still hold a failed lookup of weights another node had
  just saved, so accept_pool_sample now lists the directory above the file
  and looks again for up to two seconds before the 400.
- Share only the Triton cache between vLLM replicas. vLLM writes its AOT
  compile artifact through a temp file named by PID alone, and replicas in
  different pods often run the same PID, so two replicas compiling at once
  could leave a mixed artifact.
- Drop pool manifest, router and RBAC settings that restate llm-d and
  Envoy defaults or that no code uses, and inline single-use helpers.
- Keep local pool files under OPEN_RL_TMP_DIR, default /tmp/open-rl. With
  the documented local setup the dispatcher put its revision file under
  /mnt/shared, died with PermissionError and left every local sample
  waiting, and vLLM got the same unwritable Triton cache.
- Fall back to the same /tmp/open-rl on the cluster. The readiness
  probes path took that default, but a pools dispatcher and every
  worker pod still fell back to /mnt/shared/open-rl, so an API server
  without OPEN_RL_TMP_DIR rendered a probe for a revision file no
  dispatcher wrote, and no replica ever turned Ready. The probe, the
  dispatcher and the pods now read one tmp_root(). The in-tree manifests
  all set OPEN_RL_TMP_DIR, so they never hit this.
- Create the LoRA snapshot directory before a LoRA pool starts. vllm
  serve exits when VLLM_LORA_RESOLVER_CACHE_DIR is missing, and only
  freeze_adapter created it, so a pool started before the first LoRA save
  crash-looped and base-model-only use never recovered.
- Pass trunk's 0.90 GPU memory fraction to every replica again, with the
  env still winning. Replicas ran at vLLM's 0.92, the setting tied to FFT
  delta applies running out of memory on an L4. The device-scaled batch
  limits are deleted instead, since the API server renders the args
  without knowing the device and vLLM 0.30 sizes them itself.
- Keep a local pool's dispatcher running beside its vLLM. A dead local
  dispatcher was never restarted, so its samples waited with nothing to
  fail them, and a vLLM restart left the old dispatcher reading the
  stream beside the new one.
- Hold pending prompts as array('I'). A prompt parsed into a list costs
  about 36 bytes a token, so the 2Gi dispatcher ran out near 40M pending
  prompt tokens and crash-looped re-reading the stream. A pending prompt
  now costs about 4 bytes a token once read.
- Call a pool sampler ready only while its dispatcher has a pod. The
  readiness probe compares vLLM's version with the revision file, which
  outlives the dispatcher, so a sampler read ready with its dispatcher
  dead while every sample waited.
@ShubyM
ShubyM force-pushed the feat/sampling-pools-switch branch from e6ab43c to 54aa9d3 Compare October 19, 2026 20:51
@ShubyM ShubyM changed the title sampling: add sampling pools behind SAMPLING_BACKEND=vllm-pools sampling: sample through pools and delete the queue sampler Oct 19, 2026
@ShubyM ShubyM closed this Oct 21, 2026
@ShubyM

ShubyM commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of new design

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.

2 participants