Repository navigation
feat(memory): extract foundational memory API (Phase 1) - #1252
Closed
justinchuby wants to merge 1 commit into
Closed
justinchuby wants to merge 1 commit into
justinchuby wants to merge 1 commit into
Conversation
Move only dependency-free memory mechanism primitives into a new onnx-runtime-memory-api crate while preserving the existing governor re-exports, allocator signatures, and runtime behavior. Keep roles, errors, allocator dispatch, capacity accounting, policy, and lifecycle in onnx-runtime-memory-governor. Part of #1186 Phase 1. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Independent Phase 1 review: approved with no blocking or high-confidence findings. Verified the full diff is limited to the six stated type moves plus compatibility/workspace/publish/docs wiring; Residual gate: hosted CUDA compilation remains queued. The re-exported structs/fields are unchanged, so no source-level CUDA compatibility issue was found; the hosted CUDA lanes should still complete before the stack is merged. |
74 of 100 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
+ Coverage 79.95% 80.62% +0.67%
==========================================
Files 360 360
Lines 158518 155718 -2800
Branches 158518 155718 -2800
==========================================
- Hits 126741 125551 -1190
+ Misses 27160 25560 -1600
+ Partials 4617 4607 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
This was referenced Aug 19, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
> **A100 safety update (head `13c37a7f`):** expected shared-prefix admission failures are now all-or-private: K/V commit is transactional, a failed V share rolls K back before enqueue, and only a successful rollback permits private-KV fallback. Fatal kernel faults remain process-fatal; the new cross-process VMM probe proves a worker fault does not affect an actively executing peer, while the current server is still in-process and does not yet consume physical shared-prefix metadata. See the [final verified conclusion](#1579 (comment)). Collapses phases 1–7 of the #1186 memory architecture rework onto current `main` as a single merge. The stack forked 270 commits ago (176 of them touching `crates/`), so rebasing it layer by layer would mean solving the same 17 conflicts fourteen times, with no reviewer ever looking at the intermediate states. Closes the stack: #1252 #1263 #1279 #1283 #1301 #1341 #1349 #1426 #1440 #1448 #1454 #1462 #1465 #1468 #1533. **A review guide is in the first comment.** It is the part worth reading — this diff is 103 files, but only three decisions in it are ones a compiler cannot check. ## What was already reviewed, and what wasn't Every phase was reviewed and approved by a session that was not its author, under the rejection-lockout rule (a rejected author never writes the next revision). | Phase | PR | Verdict | |---|---|---| | 1–5 | #1252 → #1426 | approved | | 6 | #1440, #1448, #1454 | **rejected** ×3 | | 6 | #1462 | approved | | 7 | #1465 | **rejected** | | 7 | #1468 | approved | | 7 test fixes | #1533 | approved, A100-verified | The four rejected rounds were not replaced — the later rounds are stacked **on top of** them. So the tree here is the approved state, but the history contains the rejected commits. **#1440 / #1448 / #1454 / #1465 should not be reviewed individually**; they close automatically. **Not reviewed anywhere:** the conflict resolutions themselves. That is what this PR is for. ## Verification Run here, on this merged tree: - `cargo check --workspace --all-targets` — clean - `cargo check -p onnx-genai-engine --features cuda,native-backend --all-targets` — clean. Worth calling out: the default feature set does **not** compile the `cfg(cuda)` code, which is where the riskiest edits are. Checking only the default set would have missed a real break (see the guide). - `cargo test` over 7 crates — **1105 passed, 2 failed, 88 ignored** - `cargo clippy --workspace --all-targets` - `cargo fmt` **The 2 failures and the 1 clippy error are pre-existing and proven so, not merge damage:** - `platform_capacity::{disk_capacity_is_measured_for_the_working_directory, an_explicit_byte_limit_is_honored_without_a_device_query}` — `platform_capacity.rs` is byte-identical to `main` (`git diff origin/main HEAD -- ` that file is empty). The cause is a macOS-only FFI layout bug: `fsblkcnt_t` is 4 bytes on macOS (confirmed: `sizeof(struct statvfs)=64`, `sizeof(fsblkcnt_t)=4`) while the Rust struct declares `f_blocks`/`f_bfree`/`f_bavail` as `u64`. CI is Linux, where it is 8 bytes. Left alone: it is a real bug but not this PR's. - `optimizer.rs` clippy `approx_constant` — present on both parents; the merged file is byte-identical to `main`. - Three files had `rustfmt` drift already present on `main` (`matmul_nbits.rs`, `normalization.rs`, `optimizer.rs`). Reverted rather than swept in, to keep the diff readable. **Final A100 revalidation (head `13c37a7f`):** full CUDA-memory GPU suite passed; CUDA EP default-parallel lib suite passed (**488 passed / 17 ignored**); real fp16 GQA covered three requests × two interleaved decode steps with two shared peers and one transactional private fallback, byte-identical to independent GPU and CPU references. A device-started/event-gated worker remained healthy through a peer process `CUDA_ERROR_ILLEGAL_ADDRESS`, owner exit, replacement worker, and process restart. Governor tests, targeted Miri, CI Clippy, fmt, and CUDA honesty also passed. ## Still open after this merges - `memory-plugin-provider-wiring` — the back half of Phase 6 criterion 8 (provider/context pinning), which fell in the gap between phases. **#1186 must not be closed until it lands.** This merge makes it more tractable, not less: see decision 3 in the guide. - `memory-deferred-invariant-asserts` — two surviving mutants found by #1462's reviewer, adjudicated NON-BLOCKING and not defects. - #1533's CUDA honesty guard warns on a legitimately portable anchor. Should be moved out of the CUDA test binary or allow-listed rather than left warning. --------- Signed-off-by: Justin Chu <justinchuby@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Copilot-Session: 46c5d75b-8146-489c-b82f-08ee29c27ce4 Copilot-Session: 39ff6824-d35f-4d3b-8f5f-043a7119a100 Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
This was referenced Aug 22, 2026
Closed
Owner
Author
|
Closing: superseded by #1579, which collapsed phases 1–7 into a single merge — landed as This PR was approved by a session that was not its author, and its work is in Containment verified, not assumed: Every one of the 15 stack heads (#1252 → #1533) is a literal ancestor of the merged head, and I separately confirmed the merged head's content reached Why this didn't close itself: #1579's body says #1186 stays open: #1579 explicitly gates it on |
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
1 participant
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.
Summary
onnx-runtime-memory-apias the lowest memory mechanism vocabulary layer.Tier,DeviceKey,AllocationCommitRange,MappedAllocation,SharedDevicePrefix, andSharedPrefixCommitInfo.onnx-runtime-memory-governorroot andallocator-module re-exports, so downstream imports do not need a rename sweep.Part of #1186, Phase 1 only. This is the safe re-land after #1247; it does not start Phase 2.
Boundary retained in the governor
The following remain in
onnx-runtime-memory-governorbecause they are accounting/governance concerns or are currently coupled to them:DeviceAllocatorandHostAllocator: existing signatures use governor-ownedMappedPhysicalCapacityTokenandMemoryError.MemoryRoleandMemoryError: reservation purpose, budget refusal, and governed-capacity outcomes.MemoryAuthorityId,HolderId, ledgers, mapped allowances/tokens, growth authorities/grants, leases, pressure responders, and governor traits.LargeAllocCacheand prefix-shareability analysis, which are built over the current governor-owned allocator/admission model.Dependency direction is one-way: dependency-free
onnx-runtime-memory-api<-onnx-runtime-memory-governor<- existing consumers. The API crate has no EP, session, engine, or governor dependency and introduces no cycle.Behavior/signature preservation
DeviceAllocatortrait text is byte-identical to currentmain.GovernedAllocatorsource files are unchanged frommain.with_memory(Arc<dyn DeviceAllocator>), allocation/release behavior, eager/VMM selection, shared-prefix handling, mapped refunds, synchronization, governance policy, and runtime defaults are unchanged.Validation
cargo fmt -p onnx-runtime-memory-api -p onnx-runtime-memory-governor -- --checkcargo metadata --locked --no-deps --format-version 1cargo package --locked -p onnx-runtime-memory-api --allow-dirtycargo check --locked --all-targets -p onnx-runtime-memory-api -p onnx-runtime-memory-governor -p onnx-runtime-cuda-memory -p onnx-runtime-ep-api -p onnx-runtime-ep-cuda -p onnx-genai-ort-D warnings: all targets for memory-api, memory-governor, CUDA memory, and EP API; library target for ORTRUSTDOCFLAGS="-D warnings" cargo doc --locked --no-deps -p onnx-runtime-memory-apigit diff origin/main...HEAD --checkPre-existing failures/warnings on current main
cargo fmt --all -- --checkreports formatting incrates/onnx-genai-server/src/routes/completions.rs; this branch does not modify that file. The changed Rust packages pass targeted fmt.-D warningsreaches existing CPU EP and CUDA EP lints; the changed crates and EP API pass with warnings denied, ORT lib passes with warnings denied, and CUDA EP lib passes with its existingunnecessary_unwrapwarning.