Repository navigation
perf(training): let a disjoint deployment opt into sharing prefixes - #208
Closed
BobbyZhouZijian wants to merge 2 commits into
Closed
BobbyZhouZijian wants to merge 2 commits into
BobbyZhouZijian wants to merge 2 commits into
Conversation
Reef disabled SGLang's radix cache for every layout. The reason is real: a shared cache entry carries no runtime-load-ID identity, so reusing one across a publication would let a request generate under new weights from a prefix the previous weights encoded. The rollout's runtime-load spans cannot express that, because the contamination is in the prefix's encoding rather than in a suffix of the sequence — the training data would claim a provenance it does not have. A colocated deployment already makes that impossible. Its training step releases the KV cache before every publication, and SGLang's own release_memory_occupation flushes the tree before pausing that allocation, so no entry outlives the weights that built it. Reef's scheduler plugin is what lets that path run at all while requests sit retracted. Enable cross-request prefix reuse there, and keep it off for disjoint engines, which deliberately carry their KV across the update and have nothing that invalidates a shared entry. One colocated engine also holds several scenarios' adapters, and the same prefix has different KV under each. SGLang folds the adapter into the entry's key; a build that does not is left with reuse off rather than trusted to keep the scenarios apart. The config validator now rejects a re-enabling override only for disjoint groups: a colocated group may choose either setting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merge Oncall: @BobbyZhouZijian The merge oncall will route reviews and keep this pull request moving toward a clear decision. |
Closes #207. A disjoint engine keeps its KV across a weight update on purpose, so an entry built by one version stays matchable by a request running under the next, and Reef had to give up cross-request prefix reuse to prevent it. Clearing the cache at every publication is enough — but it is only possible once no request holds KV, which is what a retract pause does and what an in_place pause deliberately does not. Sharing prefixes and retracting are therefore one decision, not two: derive the pause mode and the radix cache from the same predicate so a deployment cannot end up sharing entries it cannot clear. --disjoint-prefix-sharing takes that trade: re-prefill whatever was in flight at each publication, in exchange for reuse by every request in between. Default off. Colocated deployments are unchanged — they retract already, and SGLang's release path already flushes the tree with their KV allocation; the explicit flush here is what extends the same guarantee to a disjoint engine.
BobbyZhouZijian
force-pushed
the
perf/disjoint-prefix-sharing
branch
from
September 4, 2026 02:12
fd0d50a to
4771bb6
Compare
Contributor
Author
|
Folded into #206: both halves turned out to be the same invariant — prefix reuse is safe exactly where the cache is cleared at publication, which is only possible once no request holds KV. Keeping them in one pull request rather than stacking a disjoint opt-in on a colocate enable. Branch left in place; #206 carries the combined commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #207. Stacked on #206 — review that one first; this branch contains its
two commits.
Motivation
A disjoint engine pauses
in_placeand deliberately preserves each in-flightrequest's KV across a weight update. That is what makes it disjoint, and it is
also why it could not share prefixes: an entry built under one version stays
matchable by a request running under the next, and the resulting rollout would
claim a provenance it does not have.
#207 proposed giving entries a version. Implementing it surfaced the part that
matters: a version on the entry is only meaningful while no request carries KV
across the boundary.
in_placeguarantees the opposite.Changes
configure_sglang_runtimederives the pause mode and the radix cache from the same predicate, so a
deployment cannot end up sharing entries it cannot scope.
--disjoint-prefix-sharing(default off) takes the trade for a disjointdeployment: publication retracts in-flight requests instead of preserving
them, and prefix reuse turns on.
pause_generation_for_updateflushes everyengine after a retracting pause, which is the point every publication path
already goes through. Nothing the previous weights built survives into the
next ones — the same guarantee a colocated engine gets for free, since
SGLang's release path flushes the tree with the KV allocation.
Colocated deployments are unchanged: they retract already, because their KV
has to leave the GPU regardless.
Why the flush has to follow a retracting pause
SGLang refuses to flush while requests are pending, and the refusal is not
visible over HTTP: the 200 response's own body says so — "(When there are
running or waiting requests, the operation will not be performed.)" — while
the scheduler only logs
"Cache not flushed because there are pending requests". A flush issued against an engine that kept its in-flight KV wouldtherefore be a no-op that reads as a success, which is the worst possible
outcome here.
A retracting pause is what makes it real: it releases every in-flight
request's KV allocation, and Reef's own
install_colocated_retract_offloadrelaxes
is_fully_idleso the flush can proceed with the retracted queue inplace. That is the same premise colocate has been relying on for its
release-path flush.
Flushing is also the right instrument only once nothing is mid-flight — it
would otherwise discard entries belonging to requests that legitimately hold
them.
Compatibility and operational impact
explicitly.
re-prefilled rather than resumed from its own KV. Worth it when requests
share long prefixes and publications are not constant; not worth it when
publications are frequent relative to traffic. That balance is the
scheduling question in [Bug] Wake the colocated actor once per training job instead of four times #202 and the discussion behind it.
deployment that preserves in-flight KV.
install_colocated_retract_offloadbecomes live for an opted-in disjointengine. It is dormant unless generation is paused in retract mode and
already excludes health checks, but it is no longer colocate-only in
practice.
Verification
Run on macOS/CPU, where
slime, Megatron, and CUDA are not installed:pytest tests/slime_backend(excluding the two modules importingslimeatmodule scope) → 100 passed, 2 skipped, 26 failed, against 99 passed and 25
failed on this branch's parent (perf(training): share prefix caches wherever they are cleared at publication #206). The new pass is the disjoint opt-in
wiring in
test_sglang_engine.py; the new failure istest_retracting_pause_clears_the_shared_cache_before_a_publication, whichlives in
test_rollout_recovery.pywhere every manager test needs the realslimepackage and fails the same way here. That test has therefore notbeen executed — it will first run in CI.
pytest tests/reef_service --ignore=tests/reef_service/test_slime_bridge.py→ 1411 passed, 12 failed, 4 skipped (all
slime, same onmain).pre-commit run --all-files→ all hooks pass.python -m mypy→ 4 errors from local torch stubs, unchanged frommain.Not verified, and worth confirming on a live disjoint run: that the flush
actually takes effect there. HTTP 200 does not prove it, so the guarantee
rests on the retracted engine passing
is_fully_idle— Reef's own premise ininstall_colocated_retract_offload, which colocate depends on today, butwhich has never been exercised on a disjoint engine. A scheduler log line
(
"Cache not flushed because there are pending requests") is the thing towatch for on the first opted-in run.
AI assistance
Claude Code (Opus 5) traced the invalidation and pause paths through Reef,
slime, and SGLang, wrote this change and its tests, and ran the checks above.
Checklist