Skip to content

cua_s1: retain bounded vision cache across screenshot grids - #127

Open
Levius-Fubuki wants to merge 12 commits into
InferMatrixLab:mainfrom
Levius-Fubuki:codex/cua-vision-multigrid-cache
Open

Levius-Fubuki wants to merge 12 commits into
InferMatrixLab:mainfrom
Levius-Fubuki:codex/cua-vision-multigrid-cache

Conversation

@Levius-Fubuki

@Levius-Fubuki Levius-Fubuki commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

Current-head A800 follow-up — 2026-10-22

Exact unchanged source: 298e6094f13cc02659a7f9325d21b5d075e17480. Detailed results and limits · raw evidence and portable audit.

Fresh A800 ABI8 regressions passed 5 CPU and 7 GPU Rust executions: A/B/A, entry/byte LRU, oversized/transient integrity, cached capture fallback, and zero-entry queued-copy failure with vision Graph off and on. All four drain/release markers passed. Initial build-space and stale-Cargo/zero-test attempts are retained and excluded; final recompilation and 620 source hashes were verified. Live isDraft=false; existing contributor declarations and earlier review remain recorded.

All five published assets were downloaded and verified by bytes and checksums. The downloaded offline audit and independent review passed, including 2,488 separately obtained source-file hashes across the four heads. These current-head checks supersede earlier device-unverified statements only for the listed paths; historical results retain their original source and ABI scope. Large reproduction inputs are explicitly omitted, while measurement logs are retained. No benchmark samples or performance, broad quality or other-hardware claim are added. Existing human declarations and maintainer approval remain separate. No PR merge was performed.


Current-main cache integration — 2026-10-22

Current head: 298e6094f13cc02659a7f9325d21b5d075e17480. Normally merged main 32bdd2aade0e0f3f0452d04ca5022fcaaa4783c9, preserving the existing PR commits. #117 is now merged; this PR is retargeted to main. The shared vision model conflict is resolved with both implementations retained: Cua's bounded count/byte LRU and uncached resident-output ownership, and main's generic single-image/ordered-multi-image FIFO and JEMM reference arithmetic. ABI remains 8.

The integrated Scratch owns main's Buffers. One shared 18-buffer layout determines the bounded policy's allocations and byte total; ordinary and adapted-Cua allocation sizes are checked against the fake backend's actual requested-byte counter. No opaque cuDNN workspace is guessed: the explicit bounded-cache constructor rejects reference numerics and multi-image calls before work. Existing JEMM consumers use the generic constructor and retain their reference/multi-image behavior. The recipes and backend guide document this boundary.

Cache zero/oversize cases retain device output until the consuming streams drain and the live borrow ends. Eviction synchronizes first; host download/trace failures and queued-copy failures preserve drain-before-release behavior. Main's norm, linear, enqueue, graph-capture and graph-clear function bodies are byte-identical after integration.

Personally executed CPU validation

cargo fmt --all --check
cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings
cargo test --workspace --locked --features omni-laya/serve
cargo build --workspace --release --locked --features omni-laya/serve
python -m unittest discover -s tests/benchmarks -p 'test_*.py' -v
python -m unittest discover -s tests/decider -p 'test_*.py' -v
python src/backends/cuda/check_contract.py --repo-root .
python -m mkdocs build --strict
git diff --check

macOS arm64 results: 261 workspace tests passed / 0 failed / 56 ignored, including the compile-fail live-borrow lifetime check. Strict Clippy, formatting, workspace release build, strict docs and effective-diff whitespace checks passed. The targeted vision suite passed four CPU cases (ten device cases ignored); fake LoRA dispatch and cache environment tests also passed. Extended host-boundary coverage exercises requested bytes, LRU, zero/oversized retention, trace/download failure cleanup and reference-policy rejection while retaining the existing batch GEMM/attention/FIFO assertions. Python benchmark/Decider/CUDA-checker/backend suites passed 58/16/68/8 cases; Cua Python contract tests passed 15 with one asset-dependent skip; smoke validates four requests. CUDA text contract: zero errors, one existing Laya ABI declaration warning.

Independent review of the complete 14-file effective diff found no actionable finding. Reviewed and committed diff SHA256: 812211098bab0289d8305e064faeffaa0d5966f293399b1ccbcc5b1b6e79fe95. Inherited main EOF whitespace diagnostics from an old-head comparison were preserved instead of changing unrelated guidance; the effective PR diff against main is whitespace-clean.

No new CUDA compilation, GPU/checkpoint parity, capture/replay, performance or real device-memory qualification was performed. The 56 ignored tests remain unverified on this integrated head. Historical device results below retain their original source and scope. Exact-head hosted CI 39707331377 and Docs 39707331389 passed: Rust, benchmarks, CUDA contract and documentation build; PR documentation deployment was intentionally skipped. Existing contributor answers and declarations are retained; this is agent-executed maintenance and not a new human attestation or maintainer approval. Historical #117 HOLD/base statements below are superseded by this heading for the current merge state.


Current-head GPU verification — 2026-10-15

Exact tested head: 03b111ce3f97dd97c57cc6a9023de2aa7ee56be6.

Current-head GPU validation completed: 22 passing test executions across 12 groups (includes repeated modes and a separately traced ABA probe). The source-specific Cargo target produced the expected two host-capture failure cases. Full 4B host/resident feature/hidden/logit/probability/response parity passed with vision Graph 0/1; capacity/byte LRU eviction, invalid input, zero retention, transient oversize, cached capture fallback and resident transient lifetime passed.

The retained A→B→A trace has exactly 2 captures / 1 replay. The production-entry queued-copy failure records drained and released before error return. Direct v2 oracles and ordinary/resident Graph tests also passed with their original gates. These are synthetic fixture/device correctness checks; complete 27B checkpoint and screenshot HTTP performance remain unqualified.

Integration dependency remains #117 → #127. #117 is explicitly held open and unmerged, so this result does not authorize integrating the stack.

Environment: A800 80GB/sm80, CUDA 13.0.88, Torch 2.14.0+cu130, Transformers 5.17.0, ABI 8. Four source archives and every archived source file were verified; no production-source edits or numerical-gate reductions were used. Report, raw responses/logs, protocols and SHA256 inventory are public. The downloaded archive and all 208 files were verified locally before platform shutdown; a fresh SSH probe was unavailable afterward. Independent local raw-evidence review is recorded with the release.

Initial dependency/path fixture failures and the disqualified stale shared-Cargo-target #127 attempt remain in the archive. The valid #127 and #143 runs use source-specific target directories. No throughput, latency, peak-memory, Hopper, full 27B checkpoint, FLA-enabled parity or real GUI task-quality claim is added.

Historical records below keep their named sources and environments. Pending-GPU statements in those earlier records are superseded only within the scope verified above. Existing contributor declarations and maintainer verdicts are preserved. No PR was merged.

CPU maintenance and merge handoff — 2026-10-15

Current head: 03b111ce3f97dd97c57cc6a9023de2aa7ee56be6.
Actual target: codex/qwen-vision-27b at ddd7ab5f06b161f47d5b155abc0f9723a82ca85a; the PR target was not changed.

Normally merged the documentation-only #117 maintenance. The previous #117/#135 source conflicts and ABI-8 reconciliation were already resolved; this pass changes only the shared CUDA contract documentation. Count/byte LRU, uncached resident-output ownership, queued-copy error release and original cache regressions remain unchanged. This PR still targets #117, which remains held unmerged.

Remaining device handoff: run current-source cache hit/eviction/zero-retention/oversize cases, eager/Graph changed-image replay and queued-copy error/stream-drain release checks on a GPU. Prior A/B/A evidence remains pinned to its recorded older source.

Personally ran strict MkDocs and git diff --check on this head. Verified that every changed file since the previous published head is Markdown; all non-Markdown bytes are unchanged. Rust/device suites were not repeated for this documentation-only delta.

This pass follows the contributor's 2026-10-15 instruction to maintain these existing PRs, complete CPU-only fixes and merge preparation, and keep #117 unmerged. Existing contributor declarations are preserved; this update adds no new human attestation or maintainer approval. No PR was merged and no GPU/model/performance run was performed. Source-pinned historical results remain historical. Current-head hosted workflows: CI run 38667533193, Docs run 38667536214. Read their live results separately from the personally executed checks above; these CPU workflows do not execute the skipped GPU tests.

Historical records below

Earlier maintenance/review/test sections retain their named revisions and environments. Their old ABI or build commands are historical reproduction instructions, not commands for deploying this current head. Use the current branch recipes for builds; the CPU checks above do not qualify skipped device tests.

Historical maintenance — 2026-10-15

Current head: c592ec8f52a0e7c39005c91fdb28d7cae8c517d9.

Resolved all four #117/#135 integration conflicts and refreshed onto the maintained #117 at fb74c54 through normal merges. Cua retains the explicit count/byte LRU, environment warnings and original eviction regressions; generic consumers retain the compatibility FIFO. Uncached device output remains model-owned until both vision and language streams drain. Added an explicit release API with a compile-fail live-borrow check and a production queued-copy failure regression that checks transient release before error return. The C++ fault wrapper passed syntax checking. Current source also includes #117's v2 operator regressions and colocated notices. No unrelated cumulative branch features were imported.

Personally run: format, strict workspace Clippy, workspace tests (203 passed, 0 failed, 52 ignored), release build, benchmark tests, CUDA checker/backend tests, source contract check, smoke validation and strict MkDocs all passed. Device-dependent tests were compiled and skipped on this macOS host. No new GPU, numerical-parity or performance pass is claimed.

Current-head hosted workflows: ci.yml, docs.yml. Linked hosted results are separate from personally executed local checks. Earlier evidence sections retain their named original revisions and scope; they are not new qualification claims.

No PR merge was performed. #117 remains on its explicit merge hold.

Required human contributor answers

These answers reuse the contributor's explicit 2026-10-12 maintenance instruction:
keep the existing PRs mergeable and CI passing; eliminate conflicts and review
issues; serve maintainers who need clear, verified changes that are easy to merge;
repair, validate and push to the existing PRs. The source is the contributor's
local maintenance conversation on that date; private conversation details are
not published.

  1. What outcome are you trying to achieve?

    Keep the existing PRs mergeable and CI passing.

  2. What is the highest-leverage variable for that outcome?

    Eliminate merge conflicts and resolve review issues.

  3. How will this work move that variable rather than create busywork?

    Repair the identified conflicts and review issues, validate the affected
    integration, and push the verified changes to the existing PR branches.

  4. Who is the customer or user, and what do they actually need?

    Repository maintainers, who need clear, verified changes that are easy to merge.

After answering the four questions: is a PR needed at all to achieve this outcome?

The contributor explicitly directed updates to the existing PRs. Maintain these
PRs; no new PR is needed for the maintenance work.

Observed integration-fix evidence: normal integration of #117 initially produced 39 compiler errors from cache tests referring to the previous constructor and scratch layout. The cache adaptation and root test migration resolve these errors; current-head Rust CI passes. See the integration changes and exact-head CI. Raw failure and corrected-run logs are preserved in the local maintenance evidence archive. The original cache feature addresses repeated A/B/A captures, rather than a model-output correctness bug.

Purpose

Retain exact screenshot [T,H,W] scratch/Graph pairs in a bounded LRU so A/B/A can replay A's retained buffers. Upload current pixels on every request. Cua reads CUA_S1_VISION_CACHE_ENTRIES (default 1, cap 16, 0 disables retention) and CUA_S1_VISION_CACHE_BYTES (default 256 MiB). Evict before admitting a retained miss; oversized/disabled requests use synchronized transient eager scratch without evicting resident entries. Budget excludes weights, GEMM plans/workspace, driver/Graph metadata and temporary oversized allocations.

Historical Test Plan

System1-Omni Version / Commit: 86ad72f729fabb0499bc973348e65394ad43478f.
Target: #117 branch codex/qwen-vision-27b, currently 18094f16b342d29c5442838983f8a8dd60783f2f. Standalone library ABI: 6.

cargo fmt --all --check
cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings
cargo test --workspace --locked --features omni-laya/serve
cargo build --workspace --release --locked --features omni-laya/serve
python3 src/backends/cuda/check_contract.py --repo-root .
python3 -m mkdocs build --strict

For device reproduction, rebuild the ABI6 CUDA library and export the pinned CUA_S1_BASE, CUA_S1_VISION_ADAPTER, and CUA_S1_CUDA_LIB artifacts. Follow the source-pinned vision/cache/fault-wrapper recipe. Ordinary vision, cache, trace, and injected-capture checks use separate filters/environments. CPU/hosted checks compile ignored device tests but do not execute them.

Historical Test Result

Shared vision integration — 2026-10-13

Head 86ad72f729fabb0499bc973348e65394ad43478f includes main 3e9cc33a61ddf110d97512fb6f296cf540377c61 and #117 1dc77639. Target #117's codex/qwen-vision-27b branch. Against the current #117 target 18094f16b342d29c5442838983f8a8dd60783f2f, the complete merge-base delta is eight files (+619/-67 total; 619 changed code/test lines). The original target 1dc77639 and current target have identical complete trees. Review #117 shared executor/configuration first, then this cache policy, storage accounting, Cua wrapper and root regressions. Shared VisionModel owns policy and allocation; Cua opts into bounded caching through an explicit constructor. Generic from_tensors/load keep #117's four-grid FIFO default. The estimator uses the same dimension-aware 18-buffer layout as allocation: 4B LoRA/base and 27B base CPU byte oracles are covered.

Existing parser warnings, default/cap/zero behavior and root test organization remain intact. Independent source review found no remaining actionable issue. Fresh readiness recheck at unchanged head on 2026-10-13: formatting, strict workspace/all-target Clippy, 194 CPU tests including compile doctest (0 failed, 40 ignored), locked release build and strict MkDocs pass. CUDA contract check passes with 0 errors and 1 existing warning. Rechecking the earlier A/B/A raw log confirms two captures and one replay; this is a log check, not new GPU execution.

GPU qualification from the earlier 2026-10-13 maintenance run — RTX4090

Exact shared-head source and fresh ABI6 sm89 library, pinned 4B base+vision adapter. Two normal vision Graph cases, three cache cases, an explicit A/B/A trace probe and two capture-failure cases pass. Trace confirms two captures and one retained replay with feature parity. Coverage includes capacity/LRU eviction, retained-byte admission, oversized transient use, invalid input and recovery. Injected capture failure keeps eager output and disables replay.

This establishes functional parity on those fixed inputs. No new HTTP screenshot latency/throughput campaign, generic 27B numerical qualification, physical-device-fault result or standalone performance gain is claimed. #135 resident output is a separate sibling increment; combined lifecycle qualification is tracked independently.

Demo / evidence

Separate cumulative ABI 8 qualification — 2026-10-13

Combined integration branch, final head 18e1a5a6f68b9d11d3e11c7ea7b939af87272f53, integrates the twelve source-qualified maintenance heads through normal merges. The later #117 18094f16 ancestry-only update is not in that branch; its complete tree is identical to the included 1dc77639. This is separate review material, with no new PR and no main merge. It includes a substantive cross-feature fix: model-owned transient vision output survives its borrow, then retires only after both producer and consumer streams drain. A production-entry injected-copy failure confirms the matching output allocation is released before error return and subsequent requests recover.

262 Rust CPU tests (59 ignored), 163 Python tests, strict Clippy/format, release, docs and ABI contracts pass. Fresh cuDNN ABI8 GPU qualification passes 17 Rust device invocations, all eleven fixed-seed JEMM reference operators with exact Graph replay, and five Decider controls (five requests/20 rows each); maximum probability drift 0.012, Score drift 0.01. Fixed/shared logits and complete responses are exact; actual Graph replay and shared-prefix savings are observed. Runtime validation source is b909398d; the final head changes only active recipe ABI wording, with source/test/build identity verified. No new performance or full27B/Hopper claim is added.

Exact-head cumulative CI and Docs passed. Raw protocols, responses, failures and hashes are retained in the local maintenance evidence archive.

Readiness and integration constraints

Latest maintainer review inspected this exact head and reported zero actionable findings; its COMMENTED verdict reflects the draft/template state and is not an approval. Exact-head CI and Docs passed.

#135 has merged into #117. This PR now integrates #117's resident-feature path, preserves the count/byte LRU and its existing regressions, and reconciles the ABI 8 documentation. Oversized or retention-disabled device output remains owned by the model until producer and consumer streams drain; a production-entry queued-copy failure regression checks release before error return. #117 itself remains open and unmerged under the contributor's explicit hold.

Video: N/A for this executor/cache change. Source-pinned regression tests and the recorded A/B/A trace provide functional evidence. Current-head HTTP screenshot latency, throughput and peak device memory were not measured; no speedup or full 27B/Hopper qualification is claimed. No new GPU execution is attributed to this readiness update.

Self-review

The contributor explicitly confirmed completion of self-review in the 2026-10-13 maintenance conversation and has now authorized marking #127 ready after the self-review check. Agent-assisted review checked the complete actual-target diff, architecture, root test wiring, artifact hygiene and evidence scope. This records that contributor confirmation and does not replace maintainer review.

  • I have reviewed the full diff and addressed the issues I found.
  • I have checked that the change follows the project's architecture and stays focused on the stated purpose.
  • I have run the checks appropriate to this change and reported commands, results, and anything I could not verify above.
  • I have checked that the PR description, documentation, and any accuracy or performance claims match the implementation and available evidence.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review — PR #127 "cua_s1: retain bounded vision cache across screenshot grids"

Reviewer: independent code review (ZCode agent), 2026-10-10. Not posted to GitHub; no repo modifications.

1. Verdict

1 actionable finding (P3: 1). No P0/P1/P2 findings. The cache delta is well-bounded, its key is correct, lifetimes are handled carefully, and the model contract is unchanged. The main risks are evidence gaps that cannot be closed on this host (no GPU, no Rust toolchain): all device execution and numerical-parity evidence is author-reported historical, and the current-head integration has no fresh GPU validation — which the PR itself discloses.

2. Findings

P3 — Silent fallback on unparsable cache-limit environment variables

  • File: src/models/cua_s1/native/src/vision/model.rs (head lines ~548–556, CacheLimits::from_env)
  • Trigger: an operator sets CUA_S1_VISION_CACHE_ENTRIES=4 with a stray space/non-numeric value ("four", "4 ", "-1"). std::env::var(name).ok().and_then(|v| v.parse::<usize>().ok()).unwrap_or(default) silently substitutes the default (entries 1, bytes 256 MiB).
  • Consequence: the worker starts with single-entry retention instead of the intended multi-grid cache — exactly the scenario the PR optimizes — with no log line to explain why the expected cache hits never appear. Verified in source:
let value = |name: &str, default| {
    std::env::var(name)
        .ok()
        .and_then(|v| v.parse::<usize>().ok())
        .unwrap_or(default)
};
  • Suggested fix: log effective limits once at load (cache_trace already prints them) and/or warn when an env var is present but unparsable. Minor; does not block.

No other actionable findings. The specific review-focus risks were each checked and hold:

  • Cache key correctness across grids/screenshots: key is exact [T,H,W] equality against Scratch.grid (s.grid == image.image_grid_thw). All cached per-entry inputs (Geometry.indices/weights/cos/sin, verified in src/models/cua_s1/native/src/vision/geometry.rs) are derived only from the grid — bilinear position tables and rotary angles from row/col; no image content enters them. Pixel content is re-uploaded on every request, including graph replay (cuda::upload(scratch.pixels.at(0), pixels, ...) executes before the replay branch in run_scratch), so equal-grid different-screenshot requests recompute everything content-dependent. Equal patch counts with different H/W are distinct keys (tested).
  • Boundedness: cached: Vec<Scratch> is a proper LRU (push-back MRU, remove(0) LRU victim, promote-on-hit). Invariants verified by trace: total resident entries ≤ entries (default 1, .min(16), 0 disables) and retained_bytes() + incoming ≤ CUA_S1_VISION_CACHE_BYTES (default 256 MiB). The accounting constant 64,096 B/patch exactly equals the summed per-entry allocations (pixels 3072 + position tables 32 (i32×4 + f32×4) + RoPE 256 (f32×64×2... 128+128) + Work 32832 (16384+64+16384) + activations 27904 (x/norm/qkv/q/k/attn/delta 2048 each + mlp 8192 + out 1280)) — all 15 buffers plus Work counted; pinned by the CI-run test vision_cache_budget_policy (assert_eq!(patch_bytes, 64_096), bytes_for_grid([1,32,72]) == 147_677_184). No unbounded growth path exists.
  • Cache lifetime vs request/stream lifetimes: cache lives in the VisionModel instance (per loaded model, serial execution per the runtime contract); nothing persists across processes. Eviction/replacement occurs only after self.synchronize()?; transient scratch is dropped only after a draining synchronize that also captures enqueue/read/callback errors (result.and_then(|(values, _)| drained.map(|()| values))). No stale activation can reach a response: out is fully overwritten by merger.linear_fc2 before the exact-size read, and all intermediates are recomputed from freshly uploaded pixels.
  • Numerical invariants: the cache stores compute resources (buffers, grid-static tables, captured CUDA Graph), not results. Replay launches the identical recorded kernel sequence over identical addresses with fresh pixels; Geometry::new bounds (t==1, even, n≤4608, h,w≤512, checked_mul) are enforced on every insert, and hit paths can only match previously validated grids. Graph capture records but does not execute; the eager result of the capture request is returned. Bitwise replay-vs-eager parity is asserted on device by the ignored tests (bit-exact bf16 comparisons across changed pixels and cycling grids) — author-reported passing; unverifiable here (no GPU).
  • CUDA/Metal ownership: model-local only, as the PR claims. Scratch field order drops the graph executable before every referenced buffer; VisionModel field order retires scratch/cached (graphs + activations) before weights, GEMM workspace and stream, and Drop for VisionModel synchronizes first — so no graph/buffer is destroyed while work is queued, and graphs referencing shared weights/GEMM/stream are destroyed before those. Shared cuda::Graph (in src/models/qwen3_5/native/src/cuda.rs, pub used) always ends capture (including on record failure) and destroys exec on error/drop. Eviction destroys graphs post-synchronize. No new CUDA ABI symbols, no kernel changes in the delta — verified: the 5-file delta touches only Rust/Python/Markdown.
  • Model contract: serialization → prompt → token IDs → prefill → scoring → response untouched; the delta refactors the same op sequence (compared line-by-line in the diff: identical kernels, order and arguments) into enqueue/run_scratch and adds the cache around it. forward/forward_with_trace signatures and outputs unchanged.

3. Changed-file inventory and the >3000-line rule

Scope resolution (important): the locally pinned base origin/main@4a79980 is stale. Rechecked remote: live main is 7cbce19; the PR object reports base 0d25210 (cached). 0d25210 and the #108 head 6dd54b4 are both ancestors of the PR head 82a5fac and of live main, so the true merge base with live main is 6dd54b4.

Diff Files Lines Content
Pinned task diff 4a79980..82a5fac 111 +8,419 / −120 (8,539) cache delta + already-merged prerequisites: #96 jev_vl native model (+CUDA prefix kernels, qwen3_5 multimodal), #108 vision CUDA Graph
vs cached PR base 0d25210..82a5fac 9 +860 / −74 (934) cache delta + #108 (Cargo.toml [[example]], fail_graph_capture.cpp, vision_graph_bench.rs) — matches the PR description's counts exactly
True PR content 6dd54b4..82a5fac 5 +356 / −27 (383) src/models/cua_s1/native/src/vision/model.rs (+129/−13), tests/cua_s1/vision_graph.rs (+162), tests/cua_s1/vision_cache_trace_probe.py (+20), recipe/cua_s1/native_vision_graph.md (+44/−13), recipe/cua_s1/native.md (+1/−1)

Authored code in the true delta: 324 lines (model.rs 142, tests 162, probe 20); docs 59. The >3,000-line CONTRIBUTING rule is not triggered by the PR's own content. It appears triggered only against the stale local base because of merged prerequisite content, which the PR description discloses with a split rationale (depends-on #108, now merged), an incremental-diff link, per-area validation summaries, and honest scoping of historical GPU evidence. Contributor self-review checkboxes remain unticked (draft) — consistent with draft state. Main's commits after the merge base (#120 omnijev) touch no file overlapping the delta, so a clean merge is expected (GitHub mergeable_state was still unknown at query time; API flaky today).

4. Committed-artifact hygiene (full inventory)

  • True delta: 5 files, all source/test/recipe Markdown. No run output, no binaries, no generated data. Claim "Generated run output remains outside the source diff" verified.
  • Stale-base inventory (prerequisite content, already on live main): tests/jev_vl/data/replay/reference/*.json (59 files, +1,890) with sha256.json and restore_manifest.py are maintained pinned-reference fixtures, not one-off output; tests/jev_vl/data/replay/image.png (2.6 MB) is a replay input fixture; src/models/jev_vl/native/licenses/APACHE-2.0 and THIRD_PARTY_NOTICES.md are license text; Cargo.lock is the lockfile. tests/cua_s1/fail_graph_capture.cpp and tests/cua_s1/vision_graph_bench.rs are maintained test helpers (#108). No redundant artifacts identified. Note: this stale-base content is reviewed only at inventory/hygiene level here; it is #96/#108 scope, merged into main.

5. Commands run and results

Labeled: [inspected] = source read at pinned head 82a5fac; [author-reported] = claimed in PR body, not reproducible here; [observed CI] = GitHub check states; [run] = executed personally on this host.

  • [inspected] Skill load: .agents/skills/system1-omni-review/SKILL.md at main@4a79980, SHA-256 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 (matches pinned value). Also read: docs/architecture.md, references/repository-contracts.md, CONTRIBUTING.md, PR body via API.
  • [inspected] Head sources: vision/model.rs (full), vision/geometry.rs, shared qwen3_5/native/src/cuda.rs (Graph), tests/cua_s1/vision_graph.rs, probe, recipes, Cargo.toml, .github/workflows/ci.yml; diffs at all three bases.
  • [run] git merge-base, ancestry checks, numstat/shortstat for the three diff scopes — results as in section 3.
  • [run] gh pr view 108 → MERGED, head 6dd54b4 (the prerequisite is on main).
  • [run] gh api compare 6dd54b4...7cbce19 → main is 6 commits ahead (the #108 merge + #120 omnijev); file list has no overlap with the delta.
  • [run] Head tree extracted to /tmp/pr-review-2026-10-10/head via git archive (repo untouched):
    • python -m mkdocs build --strict → exit 0 (one pre-existing INFO: docs/benchmarks/cua-s1-cuda-graphs/README.md not in nav).
    • python -m unittest discover -s tests/benchmarks -p 'test_*.py' → 56 tests, OK (weight-free; covers the head tree's benchmark tooling, not the vision cache itself).
  • [author-reported] Fresh isolated-head fmt/clippy/tests (131 passed, 0 failed, 26 ignored)/release build/MkDocs/whitespace; historical GPU: RED 3 captures/0 replays vs GREEN 2/1 A/B/A, eager feature-bit parity, capture-failure injection; benchmark table (RTX 4090, mixed p50 36.6→32.4 ms, 128 captures→0/128 replays, observed max difference 0 vs gate 0.001) at experiment source 1560d23 — byte-identity of the three cache files to current head is asserted but not verifiable here (source not local).
  • Not runnable on this host (no Rust toolchain per constraints): cargo fmt/clippy/test/build, including the CI-runnable CPU test vision_cache_budget_policy in the delta.

6. CI observed (gh pr checks 127)

At the reporting-time head (runs cross-referenced to 82a5fac in the PR body):

  • rust — pass (1m34s). Job runs fmt --check, clippy -D warnings, frontend test, cargo test --workspace --locked, release build; all GPU tests are #[ignore]d, so this is CPU-only evidence and does not exercise the cache on device.
  • benchmarks — pass (14s).
  • build (docs) — pass (16s); deploy — skipping (intentional for PRs).

7. Material unverified checks

  1. cargo fmt --all --check, cargo clippy --workspace --locked --all-targets -- -D warnings, cargo test --workspace --locked, cargo build --workspace --release --locked — not run locally (no Rust toolchain); observed only via CI pass at head.
  2. All six #[ignore]d GPU regression tests in tests/cua_s1/vision_graph.rs (changed pixels/trace, geometry retirement, capture-failure wrapper, A/B/A probe, capacity/LRU/invalid-input, byte-eviction/transient-oversize) plus vision_cache_trace_probe.py and the fail_graph_capture.cpp wrapper test — require CUDA + pinned CUA_S1_BASE/CUA_S1_VISION_ADAPTER/CUA_S1_CUDA_LIB; no GPU on this host. Current-head GPU evidence is absent; the PR discloses this ("its device integration was not rerun on this macOS host"; historical evidence "does not prove this current shared-backend integration").
  3. Numerical parity (replay vs eager bitwise identity) and the performance claim (~11–12% mixed p50 improvement) are author-reported from a pre-head experiment source; not independently reproduced, and the PR correctly labels them descriptive, two-round, single-hardware observations.
  4. Live-main mergeability was unknown at query time (API flakiness); no file-level conflict was found by inspection.

Notes (non-actionable)

  • Default CUA_S1_VISION_CACHE_ENTRIES=1 retains only the active grid: A/B/A reuse requires ≥2 (4 for the measured four-grid cycle). Documented in the recipe; conservative default is a deliberate choice.
  • The byte budget excludes weights, GEMM workspace/plans, graph metadata and transient oversized allocations — documented in code, recipe and PR body as a retained-activation budget, not a whole-device limit.
  • Capture failure permanently disables vision Graph for the loaded model (reload to re-enable); replay launch failures propagate as inference errors without eager fallback — both documented, both inherited from #108 behavior.
  • Tests access private fields via the #[cfg(test)] + #[path] include of tests/cua_s1/vision_graph.rs — exactly the pattern CONTRIBUTING permits for private-item tests; vision_graph_bench.rs is registered as an [[example]] (prerequisite #108, merged).
  • Missing coverage (minor): the .min(16) cap and the 256 MiB default are not directly asserted (env-dependent); covered indirectly by explicit CacheLimits construction in tests.

Provenance

  • Repository: ThinkFlowLab/system1-omni (ID 1386454853). Base/head rechecked remotely during review: PR head 82a5facdb785170720d5bbf75df367b3bbdcebf1 (unchanged), state OPEN, draft; live main 7cbce19f117f23214c8acd8d874c2ab5f10c683e; true merge base 6dd54b493e0e8ada5230c2a2f84ac76de3dc3708.
  • Skill: .agents/skills/system1-omni-review/SKILL.md at main@4a79980, SHA-256 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108, plus references/repository-contracts.md, docs/architecture.md, CONTRIBUTING.md.
  • Host constraints honored: no Rust toolchain invocation, no GPU execution, no repo modifications; scratch work confined to /tmp/pr-review-2026-10-10/.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-review at new head 9dd8db132c: one new commit since my review ~1h ago at 82a5facdb7 — "fix(cua-s1): warn on invalid vision cache limits".

This resolves the single P3 from that review, verified in the diff: CacheLimits::from_env now warns on any unparseable value (invalid, negative, trailing-whitespace, or non-UTF-8 via var_os+to_str), naming the setting, the supplied value and the effective default, independent of CUA_S1_GRAPH_TRACE; the recipe doc documents the behavior. The added vision_cache_limits_from_env test is a genuinely thorough self-reprobe (unset/zero/valid/clamp-17→16 and three invalid classes per variable, with warning-count assertions). Zero actionable findings remain in the draft. Unverified: the new Rust test cannot run on this host (no toolchain) and no CI has reported on this head yet; GPU/trace evidence remains author-reported.

Verdict: COMMENTED — draft status, so no approval regardless; the P3 is resolved and this looks merge-ready once the draft is marked ready and CI reports.

@Levius-Fubuki
Levius-Fubuki changed the base branch from main to codex/qwen-vision-27b October 13, 2026 06:02
@Levius-Fubuki Levius-Fubuki mentioned this pull request Oct 13, 2026
2 of 4 tasks

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head 86ad72f729: the branch merged current main including #117's shared-vision refactor; all reviewed commits are patch-identical (range-diff, named refs, cat-file verified). The substantive merge resolution was reviewed file-by-file: the bounded vision cache survives the refactor intact (CacheLimits with invalid-value warnings, LRU + byte budget, default 1 / cap 16 / 256 MiB unchanged), its committed tests were re-homed, not dropped (budget-policy constants byte-identical: 64,096/patch, 288, 147,677,184; vision_cache_env.rs wired via #[path]), and the branch stays internally consistent at ABI 6. Fork CI rust/cuda-contract/benchmarks green at the exact head. Zero actionable findings; MERGEABLE/CLEAN.

Verdict: COMMENTED — still a draft. Stacking note: this branch and #135 rewrite the same cua_s1/shared vision files differently (cache adaptation vs resident-feature adaptation) and will conflict when stacked; if #135 lands first, an ABI-8 doc reconciliation is needed here. Template state (post-#138 answers/checklist) still to be completed before leaving draft.

@Levius-Fubuki
Levius-Fubuki marked this pull request as ready for review October 13, 2026 08:42
Copilot AI balanced review requested due to automatic review settings October 13, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Levius-Fubuki

Copy link
Copy Markdown
Collaborator Author

@hsliuustc0106 Follow-up to your latest review.

Self-review is complete, and all four self-review declarations are checked. The updated PR template now includes the required human contributor answers and PR-necessity decision, purpose, test plan/results, demo/evidence, and validation limits. PR #127 has been marked ready for review.

The source head remains 86ad72f729fabb0499bc973348e65394ad43478f. The readiness recheck passed formatting, strict Clippy, 194 CPU tests (0 failed, 40 ignored), release build, strict docs, and the CUDA contract check (0 errors, 1 existing warning). Earlier GPU evidence retains its original scope; no new GPU run is claimed for this readiness update.

The #135 stacking conflict and ABI reconciliation requirements remain documented. #127 still targets #117, and #117 remains open and unmerged under the contributor's explicit hold.

Please re-check the completed template/self-review and re-review #127 now that it is ready.

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First approval review at new head 03b111ce3f after the rebase wave (prior reviews of this draft: zero findings at 86ad72f729). Wave-verification note (this pass): the rebase wave was verified systematically — every own-commit series is patch-identical per range-diff; every post-pin merge resolution was diffed against both parents with zero dropped lines; approved-content markers verified present in every tree; the key new merge 3be46fa (shared-vision line × jemm v2 tip) carries #143\u2019s new test files intact and is ABI 8/8 consistent at every descendant head.

The substantive merge (a3157974, shared-vision-executor × this branch) was verified file-by-file: the bounded vision cache survives the refactor intact (CacheLimits with invalid-value warnings, LRU + byte budget, the budget-policy test), #135's committed device tests are carried intact (fail_device_copy.cpp strictly extended, device_transient_failure_gpu.rs present), and the resolution even ADDS a new integration test combining #135's failure wrapper with this branch's zero-entry cache behavior. The template gate is now complete (checklist 4/4, human answers present) and the PR has left draft. Fork maintenance workflows (ci + docs) green at this exact head; MERGEABLE/CLEAN.

No actionable findings remain. APPROVED. Unverified: GPU execution of the ignored cache/device tests author-run; Rust suite covered by exact-head maintenance CI. Stacking note: this branch and #135 rewrite the same cua_s1/shared vision files differently — merge #135 first, then reconcile here (the body discloses the ABI-8 doc reconciliation needed).

@Levius-Fubuki
Levius-Fubuki changed the base branch from codex/qwen-vision-27b to main October 22, 2026 01:43

@hsliuustc0106 hsliuustc0106 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-check at new head 298e6094f1 after the merge wave onto main@32bdd2a (prior review: zero findings at 03b111ce3f). All own commits patch-identical (git range-diff, named refs, cat-file verified); the merge resolutions carry the absorbed jemm/decider lineage, consistent with the pass-57 wave verification of this stack. Fork CI at this exact head: benchmarks/build/cuda-contract/rust green. Zero actionable findings; still a draft, so no approval. Template state (post-#138 answers/checklist) still to be completed before leaving draft.

Copy link
Copy Markdown
Collaborator Author

Current-head A800 cache/lifetime qualification for 298e6094f13cc02659a7f9325d21b5d075e17480; source unchanged.

Live GitHub state is isDraft=false, with existing contributor answers/checklist and the earlier approval review already recorded. The latest draft/template sentence is stale. No declaration or review status was changed by this follow-up. The current integrated consumer is ABI8.

Fresh serialized A800 regressions passed 5 CPU +7 GPU Rust executions: cache policy/env/shared-vision geometry, A/B/A trace (2 captures/1 replay), entry and byte LRU, invalid input, oversized resident/transient integrity, cached capture fallback, and production zero-entry queued-copy failure with vision Graph 0 and1. Both failure variants retained positive producer/consumer drain and transient-release markers (all four exact evidence files passed). No PR113 device result was substituted for this different cache source.

Two preparation failures are preserved and excluded: a debug compile exhausted the system overlay (debug artifacts were moved intact; release dependencies reused), then a stale shared Cargo artifact yielded zero selected tests. The positive-count gate stopped the attempt before GPU work. Refreshing only source metadata forced actual Qwen/Cua compilation from this127 source; all620 byte hashes and the detailed freshness receipt match. The final positive tests passed unchanged gates. The matching ABI8 library's20 build-input files match this head; genuine cached Cua inputs were rehashed.

GPU residency returned0. No source change, speedup/latency/quality claim, new human attestation or PR merge. This qualifies the bounded tests above, not every possible cache grid/hardware/model configuration.

Raw evidence, exact source/input hashes, commands/failures, portable offline audit and independent review. All five downloaded assets matched their sealed bytes/checksums; the downloaded audit passed, including2488 separately obtained source files across the four PRs. Full source/model/native payload inputs are omitted explicitly, while all measurement logs are retained. The offline audit checks recorded evidence, not independent authentication of GPU execution.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants