Hold the background warmup ladder while a request is in flight - #300
Open
Blakeolson21 wants to merge 3 commits into
Open
Hold the background warmup ladder while a request is in flight#300Blakeolson21 wants to merge 3 commits into
Blakeolson21 wants to merge 3 commits into
Conversation
…one completes _BackgroundWarmup._foreground_quiet_for_s decided whether a warm rung may run from state.last_request_at, which is stamped when a request COMPLETES. A request that has arrived and is still generating therefore leaves it untouched, and on a daemon that has not completed a request yet it is still 0.0, which the function read as "no request has ever landed" and reported as infinite quiet. The rung was admitted and ran a full background prefill against live traffic. The window this opens is the worst possible one. A UI that restarts the engine on a configuration change is typed into immediately afterwards, so the first request of a serve is both the one that has not completed yet and the one most likely to be warmed over. Under the turbo profile's eight-rung ladder (512..32768) a single rung is tens of seconds of prefill on a 27B, and the per-chunk foreground-yield abort only bounds the damage once the rung is already running. Treat model work that is in flight or queued as zero quiet at admission time, and only then fall back to the completion stamp. The check is made at step admission, before the warming generation begins, so it reads real foreground work rather than the warming request's own counter, which is why it can use has_foreground() where the _ForegroundYield shim deliberately cannot. The scheduler-queue half is checked through the same _foreground_model_work_pending helper the yield shim uses, so queued but not yet executing foreground work also holds the ladder. Three tests: a request in flight holds the plan, queued foreground work holds the plan, and a genuinely idle fresh daemon still warms immediately so the guard does not cost the case it exists for. The first two fail before this change.
…eferral test setup
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.
Branch:
q6-lane/warmup-inflight-foreground-yield(3 commits, basemain@ 90d8c4b)Files:
mtplx/server/openai.py,tests/test_background_warmup.pyWhat is wrong
_BackgroundWarmup._foreground_quiet_for_sdecides whether a warm rung may beadmitted, and it reads only
state.last_request_at. That field is stamped when arequest completes (five assignment sites, all on completion or cancellation
paths). A request that has arrived and is still generating therefore leaves it
untouched, and on a daemon that has not completed a request yet it is still
0.0, which the function reads as "no request has ever landed" and reports asinfinite quiet:
The comment says "landed". The field means "finished". Everything follows from
that gap.
Why this window is the worst one
The failure is worst exactly where it is most visible. A control UI that
restarts the engine on a configuration change is typed into immediately
afterwards, so the very first request of a serve is both the one that has not
completed yet and the one most likely to be warmed over. Under the turbo
profile's eight-rung ladder (
512,1024,2048,2560,4096,8192,16384,32768) asingle rung is tens of seconds of background prefill on a 27B, so the operator's
first request shares the GPU with a rung that the 90 second idle grace was
supposed to have held back.
Observed on a 27B (Qwen3.8-27B, 6-bit, M3 Max) from the server's own request
log, real non-warmup rows recorded while the ladder was walking:
The per-chunk
_ForegroundYieldabort still works and still bounds the damageonce a rung is running. It cannot help with a rung that should never have been
admitted.
The change
Model work that is in flight or queued holds the ladder as its own admission
condition, whatever the grace is. Only when nothing is running does the
completion stamp decide.
Four details worth reviewing:
_foreground_busy) runs at step admission, in_run_step_inner, before the warming generation begins. At that momenthas_foreground()counts only real requests, which is why this code can useit where the
_ForegroundYieldshim documents that it deliberately cannot(the shim runs inside the warming generation, which has incremented the
counter itself via
_run_generation_dispatched)._foreground_model_work_pendinghelperthe yield shim uses, so foreground work that is queued but not yet executing
also holds the ladder.
MTPLX_WARMUP_IDLE_GRACE_S=0is how an operator says "do not wait betweenturns", and expressing busy as zero quiet would collapse the hold there
(
0.0 < 0.0is False) and hand a warm rung a request that is stillgenerating.
request_observability["warmup"]filter_dashboard_record_completionalready applies, so a warming generation no longer writes
last_request_ator
requests_completed. Before this, a rung's own completion made the nextrung read quiet as roughly zero and park for the full 90 second grace, and
/healthreportedseconds_since_last_requestas if a user had just beenserved on an untouched daemon.
last_request_atmeans "a user requestcompleted"; warmup was writing it. The counter moves with the clock so the
two
/healthfields cannot disagree.last_request_started_atis deliberately not used as the anchor even though_smart_fan_activity_probepairs it withlast_request_at: warming generationsgo through
begin_foregroundtoo, so that field is set by the warmup itself andusing it would make every rung after the first defer forever.
Verification
Four added to
tests/test_background_warmup.py:test_background_warmup_defers_while_a_request_is_in_flight(fails before)test_background_warmup_holds_a_live_request_even_at_zero_grace(failsbefore)
test_background_warmup_defers_while_foreground_is_queued(fails before)test_background_warmup_still_warms_a_genuinely_idle_fresh_daemon(passesbefore and after, so the guard is shown not to cost the case it exists for)
make_stategains theforeground_activecounter and thehas_foregroundaccessor a real
ServerStatecarries, and the shared_deferral_probehelper replaces a verbatim copy of the same setup in the existing
foreground-recent test.
The warmup-stamp half of the change has no direct test: every test in this
file fakes
_run_generation(the real one needs a model runtime), so noharness can observe the stamp. The closest pinned evidence is the existing
test_dashboard_record_completion_skips_warmup_rows, which asserts theidentical warmup filter on the sibling call in the same function.
Residual, stated plainly
A request is only counted from
begin_foreground, so the 100 to 200 ms ofhandler Python between arrival and dispatch (tokenize, chat-template render) is
still a window in which a rung can be admitted. Closing that needs an arrival
stamp that warming cannot set, which is a larger change; the per-chunk yield
already bounds that case to one warming chunk, which is what it was designed
for. This change closes the unbounded case.
If a code path ever leaked
begin_foregroundwithout its matchingend_foreground, warming would stop rather than misfire. Every site pairs themin a
finally, so this is a degradation mode, not a new failure mode.