Fall back to a preset of the same weights format when no (model, weights) row matches: prefill-cost prediction goes from 3.1x to 1.15x on a registered artifact - #195
Conversation
…t row matches The compiled context-cost table is keyed by the pair (model_id, weights_id) and carries two rows: qwen3.6-27b/groupwise-int and qwen3.8-27b/nvfp4. The targets register five combinations, so three of them - qwen3.6-27b/nvfp4, qwen3.8-27b/groupwise-int and qwen3.6-35b-a3b/groupwise-int - find no row and fall through to generic_context_prefill_cost(). That generic profile is, coefficient for coefficient, the qwen3.6-27b/groupwise-int row. For an artifact whose weights are nvfp4 that is the wrong shape of cost: the same table already holds a measured nvfp4 profile whose chunk_ns is 14.7 ms against the integer profile's 40.8 ms. find_prefill_by_weights picks a row of the same weights_id when the exact pair misses, before falling through to the generic profile. Nothing else changes: the exact-pair lookup still wins, and an artifact whose weights format has no row at all still receives the conservative generic estimate. For the two groupwise-int gaps the new path selects the qwen3.6-27b/groupwise-int row, whose coefficients are byte-identical to the generic profile, so their behaviour is unchanged by construction. The one combination this actually corrects is qwen3.6-27b/nvfp4. Measured on that artifact with a stock server and --request-log-jsonl, two runs per arm with the arms alternated and both binaries built in the same run from a clean tree, means over 27 requests: predicted_now_ns falls from 240.6/240.4 ms to 87.8/87.8 ms against an actual prefill of 78.2/76.3 ms, i.e. from 3.08-3.15x over to 1.15x, and predicted_total_ns from 1251-1259 ms to 456 ms. Time to first token does not move - 69.4 vs 69.4, 83.3 vs 83.2, 122.2 vs 122.1 ms at shared prefixes of 150, 400 and 800 tokens. This corrects the prediction, not the throughput, and does not claim otherwise. Under load the decisions do not change either. Four repeated prefixes with varying tails, 96 requests, at --max-concurrency 1, 4 and 8 - the whole range the server accepts - two passes per arm with the arms alternated: the prediction changes by 2.73x and every pressure counter is identical, including checkpoints dropped, owners evicted, searches and completed captures. Base against base agrees exactly, so the noise floor on those counters is zero. The same holds with --device-state-slots raised to 8 and 16, where the planner stops exhausting its time budget and exhausts its candidate queue instead, so the agreement is not an artifact of searches being cut short. Matching on the weights format alone is a heuristic and its limit is worth stating: it assumes two models sharing a weights format have comparable per-token cost. That holds for the rows in the table today, since both groupwise-int entries are identical, but a dense and a sparse model of the same format would not be interchangeable. A future table with a measured MoE row would want the nearest profile chosen by weights format and architecture class together.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Checked on
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f84adafea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (const ContextPrefillPreset* by_weights = | ||
| find_prefill_by_weights(*machine, identity.weights_id)) { |
There was a problem hiding this comment.
Add behavioral coverage for the weights fallback
The passing suite does not exercise this new branch: test_context_cost.cpp covers an exact compiled match and a completely unknown identity, but not a known hardware/weights pair with an unmatched model. Consequently, the suite would still pass if this fallback were removed or returned the wrong preset, even though selecting the NVFP4 coefficients is the entire behavioral change and affects planner inputs. Add a resolution test for qwen3.6-27b/nvfp4 that verifies the selected coefficients and source while retaining exact-match precedence.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
The suite passed an exact match and a wholly unknown identity, so it would still pass with the fallback removed. Resolve a registered model that has no row of its own and assert it takes the coefficients of the same weights format, that an exact (model, weights) row still wins, and that an unregistered format still reaches the generic profile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correct — the suite would have passed with the fallback removed. Covered in Strength control, since a passing test proves nothing by itself: with and passes again when it is restored. The comparison is @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ae1335b26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(int_row.model.prefill.chunk_ns != nvfp4_row.model.prefill.chunk_ns && | ||
| resolve("qwen3.6-27b", "groupwise-int").model.prefill == int_row.model.prefill, | ||
| "an exact (model, weights) row no longer wins over the same-weights fallback"); |
There was a problem hiding this comment.
Test exact precedence with distinguishable same-weight rows
This assertion does not actually protect exact-match precedence: int_row is resolved from the only compiled groupwise-int row, so removing the exact (model, weights) lookup would still return that same row through find_prefill_by_weights and the assertion would pass. The fresh evidence in this revision is that both sides resolve qwen3.6-27b/groupwise-int, rather than two distinct models sharing a weights format with different costs. Use distinguishable same-weights rows so the test fails if the fallback is consulted before the exact match.
AGENTS.md reference: AGENTS.md:L78-L81
Useful? React with 👍 / 👎.
Reporting a borrowed profile as CompiledDefault hid two things: a reader of the resolution summary could not tell coefficients measured for this model from ones taken from a sibling, and the order of the two lookups was not observable from outside, so a test could not hold it. Both follow from naming the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Right, and thank you — that assertion was worthless. There is no second row of any format to build the test on, so Strength control, with the two lookups swapped in and with the fallback removed entirely it still fails on the two fallback assertions. Both pass The new source is worth having on its own: a reader of the resolution summary can now tell @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… pick list corrected - Neroued#222 parent + Neroued#195 series were missing from the audit)
Scope
One mechanism, four lines: when the compiled context-cost table has no row for an artifact's
(model_id, weights_id)pair, fall back to a row of the sameweights_idbefore falling through tothe generic profile.
This corrects the prediction, not the throughput. Time to first token does not move; the
evidence for that is below, and the report does not claim performance.
It improves one of the three unmatched combinations. The other two are unchanged by construction
Environment
sm_120a, driver 580.105.08, CUDA 13.1.115, gcc 13.3, Ubuntu 24.04, Release,-DCMAKE_CUDA_ARCHITECTURES=120a. Dedicated host, no other tenant.a140e7ae(origin/masterat the time of writing).context_cost.cppandcontext_cost_defaults.cppare untouched between that commit and the branch point.Qwen3.6-27BNVFP4 (model_idqwen3.6-27b,weights_idnvfp4).ninfer-servewith--request-log-jsonl. Both arms are built in the same run from aclean tree -
origin/masterand this one-file commit, nothing else applied - and each arm ismeasured twice with the arms alternated.
Observation
find_prefill(src/runtime/engine/context_cost.cpp:210) matches on both fields:return preset.model_id == model_id && preset.weights_id == weights_id;The compiled table (
context_cost_defaults.cpp) holds two rows:model_idweights_idchunk_nsqwen3.6-27bgroupwise-intqwen3.8-27bnvfp4The targets register five combinations (
src/targets/*/impl/package.cpp):qwen3.6-27b+groupwise-intqwen3.8-27b+nvfp4qwen3.6-27b+nvfp4qwen3.8-27b+groupwise-intqwen3.6-35b-a3b+groupwise-intAn unmatched pair keeps
generic_context_prefill_cost(), whose five coefficients are byte-identicalto the
qwen3.6-27b+groupwise-introw. So an artifact on NVFP4 weights is costed with theinteger-weight profile, which the table itself puts at 2.78x the measured NVFP4 one.
The comment calls the generic profile a conservative estimate for an unknown model, which is
reasonable. The model here is not unknown: the combination is registered and supported
(
src/targets/qwen3_6_27b/impl/package.cpp:92); it simply has no row.Measured consequence on the stock binary, means over 27 requests, two runs:
predicted_now_ns(this prefill step)predicted_total_nsThat 3.1x agrees with the 2.78x ratio between the two profiles' coefficients, which is what makes
the substituted profile the explanation rather than a coincidence.
The same shape appears on a second artifact:
qwen3.6-35b-a3bhas no row either, and its predictionreads 139.7 ms against 24.7 ms actual (5.7x). Different model, different machine, same mechanism.
Root cause
The lookup key is a pair; the table is populated on a diagonal. Two of the five registered
combinations are covered, and the fallback for the rest is not neutral - it is one of the two
measured profiles, the slower one.
Change
find_prefill_by_weightsreturns the first row whoseweights_idmatches, andresolve_context_machine_costconsults it only after the exact pair misses:Exact matches still win. An artifact whose weights format has no row at all still receives the
generic profile.
Correctness evidence
What changes, and what provably does not.
qwen3.6-27b+nvfp4qwen3.8-27b+nvfp4rowqwen3.8-27b+groupwise-intqwen3.6-27b+groupwise-introw - identical coefficientsqwen3.6-35b-a3b+groupwise-intThe two unchanged rows are not a measurement but a reading of the table: all five coefficients of
generic_context_prefill_cost()equal theqwen3.6-27b+groupwise-introw exactly, so the newpath substitutes the same numbers those combinations already receive. The change cannot make any
registered combination worse.
The one combination it corrects, same artifact, same probe, two runs per arm, arms alternated,
means over 27 requests each:
predicted_now_nsorigin/master, run 1origin/master, run 2predicted_total_nsfalls from 1251-1259 ms to 456 ms. The corrected arm reproduces to the firstdecimal across both runs, because the prediction is a function of the coefficients and the prompt,
not of the machine's state.
Time to first token does not move. Same runs, shared-prefix traffic, three prefix sizes:
69.4 vs 69.4 ms at 150 tokens, 83.3 vs 83.2 at 400, 122.2 vs 122.1 at 800. The materialization
planner's selection is empty on this traffic either way (
stop_reason: time_budget,last_selection.frontier_tokens: 0), so a corrected cost does not change what it picks. That isreported because it bounds the claim, and because the opposite would have been the natural thing to
assume.
ctest: 104 tests, 104 passed, 0 failed on36a3cb04. The submitted commitfb925985differsfrom it in the commit message only - both have tree
5a6f6c4b97ce7e6d4f385dd365d8e071f4c7bd3fandthe same
patch-id5bc6da3418d9, so the tested tree is the submitted tree. The suite is run onetest at a time -
build, run, delete - because the test executables are ~184 MB each and 104 of them do not fit
beside the model artifact on this host's 32 GB disk. Four of the 104 registered names are
variants of two parent executables (
ninfer_kv_cache_append_{k8v4,nvfp4}_test,ninfer_softmax_attention_{k8v4,nvfp4}_test) and are built through their parent target.Tradeoffs
The heuristic's limit, stated plainly. Matching on weights format alone assumes two models of
the same format have comparable per-token cost. That holds for today's table, where both
groupwise-intentries are identical. It would not hold between a dense model and a sparse one ofthe same format:
qwen3.6-35b-a3bis MoE with about three billion active parameters,qwen3.6-27bis dense. If a measured MoE row is ever added, the nearest profile should be chosenby weights format and architecture class together. This change does not create that situation,
but it does not prevent it either.
Three consumers. Under load, none of them changes its decisions.
ResolvedContextMachineCostreaches the materialization planner, the shared-capture planner, and
resource_manager.h, whereit supplies
rebuild_ns- the cost of rebuilding a dropped checkpoint, which feeds retention andeviction. That last one is the one to worry about: an over-predicted prefill makes checkpoint
rebuild look three times more expensive than it is. It was measured, and it does not move.
Four long shared prefixes with varying tails, 96 requests, at
--max-concurrency1, 4 and 8 -8 being the ceiling the server itself enforces (
--max-concurrency must be in [1,8]), so this isthe whole range the engine allows. Two passes per arm, arms alternated, both binaries built in the
same run. Counters are summed across the server's stats windows, because the
throughputeventreports per-interval deltas rather than running totals.
predicted_nowbase -> fixcheckpoints_droppedprivate_owners_evictedsearchescapturesThe prediction changes by 2.73x and not one pressure counter moves by one event. The only
disagreement anywhere is
private_owners_degraded3 against 4 in one of the four runs atconcurrency 4 - within that arm's own run-to-run spread, since the other three runs at that point,
including both base runs, all read 4. Base against base agrees exactly on every counter, so the
noise floor on these quantities is zero and the agreement is a statement rather than an inability
to tell them apart.
The counters do respond to something, and it is not the cost. Between concurrency 1 and 8
dropped checkpoints halve, captures fall fivefold, three counters that are zero at 1 become
non-zero at 8, and the degradation-unit distribution shifts from
{0:1, 1:1, 3:97}to{0:6, 1:7, 2:81, 3:5}. So the instrument is not stuck.Which part of concurrency does that, I tested and could not confirm. Setting
--device-state-slotsto 8 and then 16 at concurrency 1 - the same checkpoint capacity theengine gives itself at concurrency 8 - raises reserved memory by a gigabyte per step, and leaves
checkpoints_droppedandprivate_owners_evictedat exactly 194 and 97, unchanged from thedefault. I had predicted, in writing before the run, that this knob would move them; it does
not. What it does move is the planner's regime:
budget_exhaustedgoes from 95 requests to 0,stop_reasonfromtime_budgettoqueue_exhausted, and the enumerated candidate set from 558targets to 256. So checkpoint capacity changes how the planner searches without changing what
gets evicted, and whatever concurrency carries that does move eviction is something else.
That failed prediction strengthens the arms comparison rather than weakening it. Base and fix
agree exactly at every one of those slot settings too - including the regime where the search
exhausts its queue instead of its clock. The obvious objection to the original table, that the
price never had time to act because every search ran out of budget, is answered by measurement:
with
budget_exhausted = 0the answer is still identical.The planner is working, and that makes the agreement stronger. It evaluates 400-600 targets
per request (
targets_evaluated) and selects one, at 3 degradation units on 97 requests of 99.The cost model's own output does change with this patch -
predicted_future_loss_nsdrops from amaximum of 1.59 s to 0.58 s, the same ~2.7x - while the selected degradation distribution and
every pressure counter stay identical. So this is not a case of a planner that never runs; it is
a planner whose inputs move and whose choices do not.
The constant 3 is not a ceiling.
degradation_units(
src/targets/qwen3_6/impl/runtime/pressure_planner.h:129) is a tally of the consequences of adecision already taken - evicted continuation, plus state, KV and checkpoint changes - and no term
in it comes from the cost model. It decomposes exactly: at concurrency 1 the units sum to 292 over
99 requests, and the independently reported counters give 97 evictions plus 194 checkpoint drops
plus one state change, which is 292. Three units is "evict one continuation and drop its two
checkpoints", repeated because the traffic is uniform.
Why it does not move, read in the code rather than inferred.
rebuild_nsis genuinely readfrom this cost model (
resource_manager.h:1908,:1966, feedingbaseline_savingincontext_portfolio_value.h:69), so it is not a dead computation. But it prices an outcomerather than choosing it: the victim's
dispositionarrives already decided inassessment.owner_outcomes, and the planner only folds its consequences into a candidate cost(
materialization_planner.h:857). And the quantity that looks like the outcome of thatcomparison is not one:
degradation_unitsis a tally, as shown above. The planner'sown search budget cannot move either:
min(5 ms, incumbent.cost.total_ns / 20)binds at 5 mswhenever
total_ns >= 100 ms, and both arms are far above it (1875 ms and 686 ms).A second consequence, and it is about the clamp rather than about this change. The budget is
written as a proportion of the incumbent's cost with a ceiling:
total_ns / 20, capped at 5 ms.The proportional half only has an effect below
total_ns = 100 ms. With the prediction inflatedthreefold, the cap is saturated on every request measured here - 1875 ms - so the proportional
term never takes effect at all. Corrected, it is still saturated on this artifact (686 ms), but
the distance to the point where the proportion begins to act is three times shorter. Where a
correct preset puts the prediction under 100 ms, that term would take effect for the first
time. I have not measured that regime and this artifact cannot reach it; it is named because
whoever measures the three missing rows will reach it before anyone else, and should look.
A note on why nothing is reused here, since the logs invite the wrong conclusion. Across all
twelve runs
context_cache.selections.rootis 99 of 99 andreused_prompt_tokensis 0, ontraffic built from four repeated prefixes. That is expected, not a defect: this probe drives the
OpenAI chat endpoint with no cache markers, and
docs/serving.md:696states that block-levelcache_controlis what "creates explicit shared-prefix candidates". Reuse that needs no markeris turn closure, and it works - a separate multi-turn probe on the same stock binary reports
reused_prompt_tokens = 38andprivate_turn_closure = 4. So the zero above measures a probethat did not ask for caching, and it carries no claim about the engine.
What this does not establish. That cost never affects eviction. Only that on this traffic, at
this cache geometry, across the whole concurrency range the engine permits, a threefold change in
the predicted cost changes no decision.
No new coefficients. The change adds no measured numbers, so it carries none of this host's
hardware into the product. Filling the three missing rows properly is a separate job and belongs
on the maintainer's hardware; the tooling for it already exists in the tree
(
bench/context_cost/, targetninfer_context_cost_bench, emittingninfer_context_cost_calibration).Workspace, memory, kernels: untouched. This is host-side cost resolution at startup.
Reproduction
No patch is needed to see the defect - it reproduces on a stock build:
--context-cost-presets FILEdoes not mask this in normal operation:docs/serving.md:766describes it as an optional runtime registry whose default is "generic + compiled defaults", and
the repository ships no such file.
🤖 Generated with Claude Code