Repository navigation
Add pluggable ResidencyPolicy seam for QMoE expert bank (#82) - #1784
Merged
Merged
Conversation
Introduce a real, non-owning ResidencyPolicy trait + validated ResidencyPlan type that consumes the per-expert region candidates added in #1779, while keeping today's whole-bank-resident behavior byte- and handle-identical by default. - New types in onnx-runtime-ep-api::weight: ResidencyDecision, ResidencyDegradationReason, ResidencyPolicy (trait), ResidencyPolicyInput, WholeBankResidentPolicy (the only shipped policy), plan_residency() (single plan-creation entry point), and validate_decision() (fail-closed validation: any invalid PerExpertCandidate proposal -- out-of-range/duplicate expert index, or proposed over a non-pageable catalog -- degrades to WholeBankResident with an explicit reason, never panics). - ResidencyPlan is pure data (BTreeMap-free HashMap<ValueId, ResidencyDecision> + policy name); it has no allocator/stream/pointer fields and cannot allocate, copy, or synchronize by construction. - Wired one call site in Executor::build_with_cuda_requirement (build.rs): plan_default_residency() runs right after expert_region_candidates, using WholeBankResidentPolicy, and the result is recorded via a new "session.residency_plan" trace span (policy name, resident/degraded/total counts) using existing trace_span/Args infra -- no new counter mechanism. - No production dispatch path reads PerExpertCandidate decisions yet; a test-only AlwaysSplit policy proves the trait boundary is genuinely substitutable (not an inert marker) without shipping LRU/static-hot/ q*/prefetch-execution behavior. - Promoted onnx-runtime-loader from a dev-dependency to a regular dependency of onnx-runtime-ep-api so weight.rs can reference WeightRegionCatalog/NonPageableReason/Pageability outside tests; no cycle (loader does not depend back on ep-api). - Tests (new, same commit): default-policy equivalence for pageable and non-pageable catalogs, alternate-policy substitutability, three invalid-decision rejection cases (out-of-range index, duplicate index, per-expert-over-non-pageable), and deterministic plan ordering, both at the weight.rs unit level and the Executor level (proving weight_handles/binder output is unchanged). - No changes to weight_paging.rs, kernel ABI, prefetch execution, or any static-hot/LRU/q*/elastic-resize logic -- this slice only introduces and proves the seam per Roy's staged plan in .squad/decisions/inbox/roy-residency-policy-gated-on-82.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
- Coverage 80.64% 80.28% -0.37%
==========================================
Files 411 411
Lines 199652 199922 +270
Branches 199652 199922 +270
==========================================
- Hits 161019 160501 -518
- Misses 33175 33962 +787
- Partials 5458 5459 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
**`main` is red and no PR can merge.** Since `fbccba582` four required
jobs fail — `Rust quality` ("Check the native backend compiles"), `CUDA
compile` ("Clippy engine native-CUDA integration"), and both `CLI ORT`
lanes — with one error between them:
```
error: this function has too many arguments (8/7)
--> crates/onnx-genai-engine/src/engine/placement.rs:115:1
```
`push_qmoe_pair`, added in #1784, takes 8 arguments and
`clippy::too_many_arguments` is denied. It fires on the engine lib, so
every job that clippies a crate depending on it fails.
## The fix
Not a threshold bump. The 8 arguments were two clusters that were
already conceptually one thing each:
- The three input indices become **`QmoeExpertSlots`**, with
`FC1`/`FC2`/`FC3` constants. The call sites read `2, 3, 11` / `5, 6, 12`
/ `8, 9, 13` today — positional `usize` triples with nothing to stop a
scales index landing in the zero-points slot — and now name the expert
tensor they mean.
- `bits` and `block_size` become **`QmoeQuant`**, the per-node
quantisation pair that was identical across all three calls.
Eight arguments to five.
**No behaviour change.** Same slot indices, same order, same control
flow; the destructure at the top of the body keeps the rest of the
function byte-identical.
## Validation
- `cargo clippy --locked --all-targets -p onnx-genai-engine --features
native-backend -- -D warnings` — the exact failing CI command — clean
- the same without `--features native-backend` — clean
- `cargo fmt --all -- --check` — clean
- `cargo test -p onnx-genai-engine --lib placement` — passing
Reproduced the failure on `origin/main` at `ac9c7dd06` before fixing,
and confirmed the same command is clean after.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 23, 2026
…paging authorities (#82) (#1789) Part of #82 (DeepSeek-V4/GLM-5.2 QMoE vertical slice). 4th slice of the pluggable residency-policy line: #1779 (region candidates) → #1784 (policy seam) → #1787 (migrate existing decision logic into `CudaHotSetResidencyPolicy`) → this PR (elastic resize). ## Goal Introduce FreeToken-style elastic expert-residency **resizing** as a policy-issued resize *intent*, executed transactionally by the existing Resource Governor + PMM/VMM/weight-paging authorities. The policy never allocates/frees/copies/relocates a virtual address — it only asks; existing infrastructure executes. ## New contract (`onnx-runtime-ep-api::weight`) - `ResidencyResizeRequest { direction: Grow | Shrink, target_bytes, priority }` - `ResizeSafePoint { capturing, pending_deferred_releases, admission_in_flight, multi_device }` with `is_safe()` / `blocking_reason()` - `ResizeRejection` (`NotSafePoint` / `WouldExposeColdExpert` / `ExecutionFailed` / `NoOp`) - `ResidencyResizePlan::{Accepted(request), Rejected { request, reason }}` produced by the pure `plan_resize(request, safe_point)` — no allocation, copy, or VA touched during planning. - `ResidencyResizeOutcome` (before/after bytes, `accepted_bytes`, `rollback_count`, `safe_point`, `is_success()`). ## Executor side (`onnx-runtime-ep-cuda::weight_paging`) - `CudaWeightResidency::resize_safe_point(device_count)` builds the snapshot from **existing** signals only: `CudaRuntime::is_capturing()`, the provider deferred-release queue's pending count, and the existing in-flight-fill quarantine. No new state. - `CudaWeightResidency::execute_resize(plan, device_count)` is the *only* place a plan becomes bytes actually moved: - **Grow** reuses the existing `MemoryLease::grow` path — the same one `admit_committed_span`'s over-budget handling already uses. - **Shrink** reuses the existing `ReclaimableMappedHolder::reclaim_mapped` (governor-driven LRU eviction + deferred-release-queue-idle wait) when a mapped allowance is installed, or an equivalent plain-lease evict-then-shrink through the same `remove_page` helper otherwise (so process-global resident-byte/eviction telemetry stays in lockstep, not duplicated). - Re-validates the safe point immediately before committing (planning and execution can be separated in time). - Refuses to shrink an *ungoverned* budget (no lease/allowance) — mirrors `set_ungoverned_budget`'s existing refusal to shrink once nothing governs the budget. ## Safe-point invariant A resize may only commit when: no CUDA graph slot in use is capturing/replaying; the provider's deferred-release queue has nothing pending; no page admission is currently in flight; and the call is **not** running under multi-device/TP. No existing barrier/authority in this codebase coordinates a resize across devices, so multi-device fails closed with an explicit reason rather than inventing distributed synchronization (confirmed via search — no TP/multi-device coordination primitive exists yet). ## What can actually be reclaimed this slice Honest scope: only the existing dense/MatMul weight-paging cache's ordinary LRU-evictable, non-pinned resident bytes — the same population `reclaim_mapped` already touches. **No QMoE per-expert reclaim is enabled.** Whole-bank QMoE residency is unchanged; slice 2/3's "no cold expert may reach QMoE" invariant is preserved because nothing here changes per-expert eviction. Per-expert-aware eviction (requiring a routed-residency guarantee) is the next dependency gate. ## Default behavior Unchanged: `budget()`/`adopt_governed_budget()`/`set_ungoverned_budget()` are untouched, and every existing `weight_paging` test still passes without this seam ever being invoked. ## Tests - `crates/onnx-runtime-ep-api/src/weight.rs` — `plan_resize` acceptance/no-op/capture/pending-release/in-flight/multi-device rejection, deterministic blocking-reason priority, outcome success reporting. - `crates/onnx-runtime-ep-cuda/src/weight_paging.rs` — ungoverned cache refuses grow/shrink leaving budget untouched; governed grow moves exactly the requested bytes / fails over-capacity with budget unchanged; zero-byte resize is a rejected no-op; multi-device fails closed; repeated grow/shrink oscillation keeps accounting consistent; rejected-plan outcome preserves the original request's direction. ## Local validation (A100) - `cargo fmt` / `cargo clippy -D warnings` on both touched crates — clean. - `cargo test -p onnx-runtime-ep-cuda --features cuda --lib` — 524 passed, 0 failed, 31 ignored (GPU-only helpers not applicable), including a single-threaded rerun to rule out cross-test device contention. - `cargo test -p onnx-runtime-ep-api --lib` — 79 passed (+ new resize tests, 88 total counting weight.rs additions). - `cargo test -p onnx-runtime-session --lib --features cuda` — 205 passed. ## Independent review A code-review agent inspected the diff before this commit and found two substantive issues, both fixed prior to committing: 1. The plain-lease shrink fallback updated only per-instance accounting and skipped the process-global resident-byte/eviction gauges every other eviction call site updates — now routed through the existing `remove_page` helper so telemetry stays consistent. 2. A plan-time-rejected outcome always reported `direction: Grow` regardless of the original request — `ResidencyResizePlan::Rejected` now carries the original request so telemetry reports the correct direction/bytes even on rejection. ## Explicitly out of scope q* CPU/GPU split, predictor-based prefetch, a new allocator/cache, and enabling per-expert QMoE eviction/reclaim (next dependency gate, once a routed-residency guarantee exists). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Working as Batty (CUDA & Performance Engineer).
Goal
Next #82 slice per Roy's architecture verdict: introduce the smallest real, pluggable expert-residency policy boundary that consumes the per-expert region candidates from #1779, while default behavior stays exactly whole-bank-resident. This is a policy-boundary refactor, not a residency behavior change.
Exact new contract (
crates/onnx-runtime-ep-api/src/weight.rs)ResidencyDecision—WholeBankResident { reason: Option<ResidencyDegradationReason> }|PerExpertCandidate { experts: Vec<usize> }.ResidencyDegradationReason—NonPageableCatalog(NonPageableReason)|PolicyDeclinedSplit|RejectedByValidation(String).ResidencyPlan— pure data:HashMap<ValueId, ResidencyDecision>+policy_name. No allocator/stream/pointer fields; cannot allocate/copy/synchronize by construction.policy_name(),decision(),ordered_values()(deterministic ascending order),len()/is_empty(),resident_count()/degraded_count()(disjoint).ResidencyPolicyInput<'a>—boundary,catalog: &WeightRegionCatalog,budget_bytes: Option<u64>.ResidencyPolicytrait —name(),decide(&self, &ResidencyPolicyInput) -> ResidencyDecision. Immutable borrow only in, owned value out — no way to mutate external state through the signature (the "non-owning" proof).WholeBankResidentPolicy— the only shipped policy; always resident, surfacing the catalog's ownNonPageableReason(if any) rather than inventing one.plan_residency(candidates, policy, budget_bytes) -> ResidencyPlan— the single plan-creation call site.validate_decision(catalog, decision)— fail-closed: any invalidPerExpertCandidate(out-of-range index, duplicate index, proposed over a non-pageable catalog, or an index with no real region) degrades that single entry toWholeBankResident { reason: Some(RejectedByValidation(..)) }. Never panics, never fails the whole plan.Call sites (requirement #2)
Executor::build_with_cuda_requirement(build.rs) callsplan_default_residency()immediately after computingexpert_region_candidates, usingWholeBankResidentPolicy. Result stored asExecutor.residency_plan(new field,state.rs), read-only under#[cfg(test)]today.plan_residency()itself validates every decision before it's accepted into the plan (see above) — this is "existing infrastructure" in the sense that no new dispatch-time consumer was introduced; no production path acts onPerExpertCandidateyet, matching the explicit scope (no kernel ABI changes, no CUDA/weight_paging.rstouch)."session.residency_plan"trace span recordspolicy,resident,degraded,total, using the existingtrace_span/Argsinfra — no duplicate counter/state.Non-owning proof
ResidencyPlan/ResidencyPolicy/ResidencyPolicyInputhold no allocator, stream, or pointer/handle fields — only value-level data (ValueId,usizeindices,&'static str, catalog references). The trait'sdecide()takes&selfand an immutable input reference and returns an owned enum — it has no path to allocate, copy, or synchronize.Substitutability proof
A test-only
AlwaysSplitPolicy/AlwaysSplitpolicy (in bothweight.rsandexecutor/tests.rs) proposesPerExpertCandidatefor every pageable catalog and is accepted byplan_residency()/wired throughplan_residency_with()(a test/measurement-only seam alongside the productionplan_default_residency()), proving the trait boundary is real, not an inert marker — without shipping LRU/static-hot/q*/prefetch-execution behavior.Tests (same commit)
weight.rs: default-policy equivalence for pageable and non-pageable catalogs, alternate-policy substitutability, three invalid-decision rejections (out-of-range index, duplicate index, per-expert-over-non-pageable), deterministic ordering.executor/tests.rs: default plan matches whole-bank-resident with unchangedweight_handlesoutput; non-pageable reason surfaced without changing handles; seam substitutability at the executor's own candidate wiring, with the defaultExecutor.residency_planunaffected by the substitution.Local evidence
cargo test -p onnx-runtime-ep-api --lib: 71 passed.cargo test -p onnx-runtime-session --lib: 201 passed (+3 new vs. pre-slice 198).cargo test -p onnx-genai-engine --lib: 445 passed, 1 ignored (unaffected, run as a broader regression check).cargo clippy -p onnx-runtime-ep-api -p onnx-runtime-session --lib --tests: clean.cargo fmton touched crates: clean.weight_paging.rs/kernel-ABI files touched in this diff — a CUDA/VMM comparison run is not applicable to this slice (verified viagit diff --stat, onlyonnx-runtime-ep-api/onnx-runtime-sessionfiles changed); 5x A100 confirmed available for future slices that do touch CUDA paths.Reviewer verdict
Independent code-review agent found one substantive issue:
resident_count()'s doc comment said "no degradation reason" but the implementation counted allWholeBankResidententries including degraded ones, soresident_count()+degraded_count()weren't disjoint as documented (misleading telemetry). Fixed:resident_count()now filters onreason: Noneto match its documented semantics and stay disjoint fromdegraded_count(); corresponding tests updated. Re-ran all targeted tests after the fix — all green.Explicitly out of scope (per Roy's staged plan,
.squad/decisions/inbox/roy-residency-policy-gated-on-82.md)No static-hot algorithm, LRU mutation,
plan_double_buffer/prefetch execution wiring, q*, elastic resize, or kernel ABI changes.weight_paging.rs'sCudaWeightResidency/eviction_for_boundaryremain untouched and the single non-pluggable authority for allocation/accounting/VA/sync.Next policy-implementation slice
Per Roy: refactor
eviction_for_boundary/static-pin-threshold logic inweight_paging.rsinto a realResidencyPolicyimplementation (byte-identical behavior first), then wire the already-implementedplan_double_buffer(executor/prefetch.rs) through the plan for actual prefetch execution.Part of #82 — this is one slice of a staged plan; issue should remain open for the follow-ups above.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com