Skip to content

cua-s1: keep vision features GPU-resident for language execution - #135

Merged
MrDongsls merged 5 commits into
InferMatrixLab:codex/qwen-vision-27bfrom
Levius-Fubuki:codex/cua-execution-pipeline-20261011
Oct 14, 2026
Merged

MrDongsls merged 5 commits into
InferMatrixLab:codex/qwen-vision-27bfrom
Levius-Fubuki:codex/cua-execution-pipeline-20261011

Conversation

@Levius-Fubuki

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

Copy link
Copy Markdown
Collaborator

Purpose

Keep Cua vision-merger BF16 features on GPU across language questions with opt-in CUA_S1_DEVICE_FEATURES=1. Default remains the host path; invalid explicit values fail before loading. Vision Graph is independent; resident-feature language remains eager. Checked borrowed device views prevent scratch reuse while consumers use its output. Producer and consumer streams drain before release, including queued-copy failure.

Shared vision integration — 2026-10-13

Head 8207cdf02a7cf9aacef12c20f357e9a6ca55b97a includes main 3e9cc33a61ddf110d97512fb6f296cf540377c61 and repaired #117 1dc77639. Target #117's codex/qwen-vision-27b branch: the actual increment is resident output and its contract/tests/docs, not a second vision implementation. Shared VisionModel owns execution; Cua delegates. Generic output width follows config, shared exact-grid FIFO policy is retained, and no #127 LRU policy is included here.

ABI 8 header, runtime getter, Rust loader, manifest and consumers agree. Optional v2 vision symbols and current Decider consumer remain registered. README, model support matrix, MkDocs navigation and the new recipe/cua_s1/native_screenshot.md describe the actual entry points and limits.

Fresh qualification — RTX4090, 2026-10-13

Exact shared-head source; CUDA 13.0.88 / Rust 1.98 / pinned 4B base+adapter/export; no threshold relaxation. Formatting, strict workspace/all-target Clippy, 199 CPU tests including compile doctest (43 ignored), locked release build, strict MkDocs and CUDA contract checks pass. Independent merge review found no remaining actionable source issue.

Thirteen device test invocations pass: finite scan; three host/device feature, language-growth and nonfinite-recovery cases with vision Graph both off and on; two normal vision Graph regressions; resident replay; host/device capture-failure fallback; and injected failure after a real queued D2D copy. The last case confirms drain before release and successful recovery. This is fresh functional evidence, not a new screenshot latency campaign, 27B qualification or physical-device-fault claim.

Historical screenshot campaign

Raw evidence archive · Release / report / SHA256SUMS

Archive SHA-256: c0578fddf4b923fa40e70db437fe1c57ac3c91dec4012811f4b1d76457c99325 (GitHub asset digest and local file hashes verified).

Tested commit: aa5cf36123c0e11b6a10aa44b2f3dc64f31b4974. RTX4090 24GB,
driver 580.105.08 / CUDA 13.0.88, Rust 1.98.0, SM89, BF16; identical release
worker/library and checksum-verified pinned base/adapter export in every mode.
Source, binary/library/export hashes and package versions are retained.

25 GPU tests passed, including all 12 shared kernel/checkpoint/prefix cases,
finite rejection, 3 host/device matching cases with vision Graph off and the same
3 with it on, replay/geometry, capture failure, and queued-copy drain/recovery.
Complete stdout and explicit reproduction commands are in the archive.

60/60 measured configurations completed: five rounds per host/device ×
vision-Graph off/on × concurrency 1/4/16. Eight real 1280×720 browser screenshots
from Python Documentation, Wikipedia and MDN; every configuration has 32 requests
mixing one/four questions. 1,920 requests / 4,800 questions, zero failures.

960/960 host/device HTTP body-byte pairs match (480 per vision-Graph setting).
Host Graph off/on: 480/480; device Graph off/on: 480/480. Status and content
type also match. No differing request or field was observed. An independent local
verifier recomputed every measured percentile, throughput, sampled memory maximum
and byte comparison directly from the retrieved raw records.

“Graph” means CUA_S1_VISION_GRAPH; CUA_S1_GRAPH=0 throughout, so multimodal
language execution remains eager. Startup, first real image/capture, four excluded
feasibility runs and uncounted warmups are preserved separately. Feasibility logs
confirm real-image capture/replay at [1,44,80]; measured workers logged no capture
failure/fallback. Mode/concurrency order and the fixed budget were frozen before
execution; five values and ranges per configuration remain in summary.json.

That campaign belongs to its recorded pre-shared source/binary. Its screenshot/latency records remain historical and do not establish maintained-head performance. Current device tests above validate the new shared integration. Full 27B JEMM and combined v2 multimodal-language Graph remain unverified on this 24GB server.

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.

The contributor also explicitly requested the current-head real-screenshot GPU
comparison and attachment of the raw results on 2026-10-12. The completed
campaign, provenance and remaining limitations are recorded above.

Contributor self-review

The existing human answers above are retained. Agent-assisted source review, regression checks and device qualification do not substitute for contributor confirmation or maintainer approval. Existing checklist state remains unchanged:

  • 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.

Cumulative ABI 8 qualification — 2026-10-13

Combined integration branch, final head 18e1a5a6f68b9d11d3e11c7ea7b939af87272f53, integrates all twelve maintained PR heads through normal merges. 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.

Copilot AI balanced review requested due to automatic review settings October 11, 2026 03:23

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.

@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.

Review draft — PR #135 "cua-s1: keep vision features GPU-resident for language execution"

Reviewer: independent local review (no posting, no repo modification).
Head pinned: 98d0465de57d4d67c5a49f00cc5fb95526df5360 (ref refs/remotes/p6new/135, fetched 2026-10-11).
Base: origin/main = 7cbce19f117f23214c8acd8d874c2ab5f10c683e (refetched; unchanged from prior check). Merge base of head vs base = 7cbce19 — the PR is a clean rebase onto current main; the diff below is exactly the PR's content.

Verdict

No P0/P1 findings. 1 × P2, 2 × P3, plus notes. By inspection the residency path is bit-exact against the host path by construction, and the buffer-lifetime design (borrow-typed device view) makes the dangerous failure modes unrepresentable in safe code. The material gaps are evidence and coverage, not correctness: the new path ships with zero committed tests (the author's parity/failure-injection tests ran from an uncommitted validation overlay), a new serving env flag is undocumented in-repo, and new diagnostic API surface has no in-repo consumer.

This is not an approval and not a statement that validation is complete; see "Material unverified checks".

Changed-file inventory and size

8 files, +415/−35 (450 total changed lines), from git diff --numstat origin/main...refs/remotes/p6new/135:

File +/- Content
src/backends/cuda/qwen3_5/elementwise.cu 16/0 bf16_finite_kernel + cs1_bf16_finite
src/backends/cuda/qwen3_5/ops.h 4/1 ABI 6→8, cs1_get_device, cs1_bf16_finite decls
src/backends/cuda/qwen3_5/runtime.cu 2/0 cs1_get_device
src/models/cua_s1/native/src/vision/model.rs 53/13 forward_device, run/enqueue_image refactor, capture_current
src/models/cua_s1/native/src/vision_engine.rs 96/13 CUA_S1_DEVICE_FEATURES, raw_score_device, score_device, device branch in execute_prepared
src/models/qwen3_5/native/src/cuda.rs 122/2 DeviceBf16Matrix, DeviceFeatureLayout, current_device, DeviceBuffer::device, ABI 8
src/models/qwen3_5/native/src/inputs.rs 67/5 DeviceMultimodalInput, validate split into metadata/positions
src/models/qwen3_5/native/src/model.rs 55/1 forward_multimodal_device

Large-code-change rule (CONTRIBUTING): authored code = 450 lines, total diff = 450 lines — no docs, generated output, lockfiles, or fixtures in the diff, so the counts coincide. Well under 3,000; split rationale/component-map requirements not triggered. Artifact hygiene over the full inventory: no artifacts added or changed (no data, dumps, caches, or binaries) — clean.

Findings

P2 — No committed tests for the new residency path; validation exists only in an uncommitted overlay

  • Where: whole PR; contrast tests/cua_s1/vision_graph.rs (head) which covers only the host forward/forward_with_trace path.
  • Trigger → consequence: any future regression in forward_device (vision/model.rs:240), forward_multimodal_device (model.rs:855), DeviceMultimodalInput::validate (inputs.rs:99), validate_finite (cuda.rs:286), the bf16_finite_kernel (elementwise.cu:89), or the CUA_S1_DEVICE_FEATURES gate lands with nothing in the repo to catch it — CI (which cannot run GPU tests anyway) and the local GPU test suite both stay green. The PR body states: "Feature-specific tests and failure-injection wrappers were run from a separate, uncommitted validation overlay; they are not added to this PR." Those tests are therefore not reviewable and not reproducible by any other contributor; the review skill's checklist for graph/cache changes ("test changed inputs, reuse, growth, eviction and failure recovery") is satisfied only by author-reported runs of invisible code.
  • Evidence (personally verified at head): git grep -n "forward_device|score_device|forward_multimodal_device|DeviceMultimodalInput|CUA_S1_DEVICE_FEATURES|DeviceBf16Matrix" refs/remotes/p6new/135 (excluding the five changed source files) returns zero hits — no test, example, or binary consumes any of the new API. The diff touches no file under tests/ and no Cargo.toml [[test]] entry.
  • Note that a meaningful subset is testable weight-free and would run in CI: device_features_enabled (vision_engine.rs:259), DeviceFeatureLayout::new exact-extent/overflow checks (cuda.rs:244), and DeviceMultimodalInput::validate metadata rejection — pure Rust logic. The GPU-path tests could follow the existing vision_graph.rs pattern (ignored by default, run by contributors with GPUs).
  • The PR body says test support was "excluded from the commit as requested"; even so, at merge time the repository carries no protection for the changed behavior. Recommend committing at least the weight-free unit tests and a vision_graph.rs-style device parity/recovery test (ignored by default) before or with merge.

P3 — New serving env flag CUA_S1_DEVICE_FEATURES is undocumented in-repo

  • Where: vision_engine.rs:37-43 (read at load), vision_engine.rs:259-265 (device_features_enabled).
  • Trigger → consequence: an operator reading recipe/cua_s1/ (which documents every sibling flag: CUA_S1_GRAPH at recipe/cua_s1/native.md:26, CUA_S1_VISION_GRAPH and CUA_S1_GRAPH_TRACE at recipe/cua_s1/native_vision_graph.md) has no way to discover the opt-in residency mode, its default-off behavior, or its failure values ("must be unset, 0, or 1"). git grep CUA_S1_DEVICE_FEATURES refs/remotes/p6new/135 -- '*.md' returns nothing.
  • Evidence: grep above; PR body documents the flag, but PR prose is not the documentation site. CONTRIBUTING requires documenting model contract/lifetimes "alongside the existing API", and the repo's convention is one recipe section per serving flag. The body says documentation was "excluded from the commit as requested" — flag as a follow-up that should not be lost.

P3 — Dead diagnostic surface with no in-repo consumer or exercise

  • Where: VisionEngine::score_device (vision_engine.rs:161-175) and DeviceBf16Matrix::download (cuda.rs:312-328, "Diagnostic readback; serving does not download the full feature matrix").
  • Trigger → consequence: both are pub with zero callers anywhere at head (grep verified; omni-cua-s1-vision binary and all tests use only the host readout). download() contains the PR's only unchecked FFI call (unsafe { (api().cs1_stream_destroy)(stream) }; — return value dropped) and a non-trivial stream create/destroy lifecycle that nothing in the repo ever runs. Unexercised unsafe-adjacent code ships as untested surface; if it is meant for the uncommitted parity overlay (P2), it should land together with that consumer or with an ignored GPU test; otherwise it is unused API introduced by this PR (the self-review checklist asks to remove unused code introduced by the PR).

Notes (not findings)

  • Numerical parity is bit-exact by construction (verified by code reading). Host path: vision merger BF16 → read() download (cuda.rs cs1_download = cudaMemcpyAsync + cudaStreamSynchronize, runtime.cu:35-39) → Vec<bf16> → overwrite_image_rows re-serializes to_le_bytes() and uploads (model.rs:931-955) — a bit-preserving round trip. Device path: cs1_copy_dd = async cudaMemcpyDeviceToDevice of the same bytes into the same destination rows (s.res + index * row_bytes, same run-coalescing loop as overwrite_image_rows, compare model.rs:880-897 with model.rs:937-955). Downstream ops are the same code on both paths: upload_positions, embed_tokens, run(s, &[t], true) (same custom_positions=true as host forward_multimodal), last_hidden, and the shared letter_logits (f64 accumulate → f32, unchanged). The only divergence is the finite check: host bf16::is_finite() vs the GPU exponent mask (x & 0x7f80) == 0x7f80 (elementwise.cu:89-95) — equivalent for ±Inf/all-NaN, same error text "non-finite image embedding". Author-reported byte-identical responses (576 comparisons) are consistent with this analysis.
  • Lifetime/ownership design is sound. forward_device returns DeviceBf16Matrix<'_> tied to &mut VisionModel (vision/model.rs:240), so scratch retirement/geometry rebuild (vision/model.rs:317-322) cannot run while a view exists; no raw-pointer constructor is exposed (cuda.rs:384-391 doc). DeviceFeatureLayout::new requires rows*columns*2 == allocation bytes with checked muls (cuda.rs:244-256); the producer allocation is exact (out: DeviceBuffer::new(n / 4 * 2560 * 2), vision/model.rs:530) so the view is created only against an exact fit. Language-side scratch growth (prepare_scratch realloc) cannot invalidate the view — features are external to language scratch, and each forward_multimodal_device re-derives s after growth.
  • Stream ordering is verified on every exit. forward_device synchronizes the vision stream before publishing the view (vision/model.rs:243); forward_multimodal_device drains the language stream on success and error (let synced = self.synchronize(); synced?; result, model.rs:901-903) before the borrow can end — this also covers the queued D2D copies; execute_prepared additionally drains both streams on all paths (vision_engine.rs:245-248). cs1_copy_dd is stream-ordered async (runtime.cu:41-44), correctly documented; validate_finite reads its flag back through the synchronizing cs1_download plus an explicit synchronize (cuda.rs:286-308).
  • Graph interaction unchanged. The device path is eager-only on the language side (PR states multimodal execution remains eager; forward_multimodal_device calls run directly, never touches self.graphs). Vision-side capture is preserved: capture_current runs after the completion boundary in both paths (vision/model.rs:243-246, 262-266), capture failure still falls back to eager and disables replay (vision/model.rs:346-353), and the refactor of run into enqueue_image preserves replay/capture semantics exactly (replay branch returns (n, false); capture flag callback.is_none() && graph_enabled, identical to main's conditions). Error-path draining inside run/forward_device is new but strictly additive (main drained only in execute_prepared).
  • Sizes/overflow: all feature extent math goes through checked_mul (cuda.rs:250-252, inputs.rs:102-105); bf16_finite_kernel uses a grid-stride loop with grid capped at 65535 and guards n == 0 (elementwise.cu:155-161). Source row offsets index * row_bytes are bounded by the validated indices < t ≤ max_positions and the exact-extent layout check.
  • CPU/fallback behavior: default (CUA_S1_DEVICE_FEATURES unset or 0) keeps the host path with identical code; invalid values fail closed before checkpoint loading (vision_engine.rs:37-43), including non-Unicode. The ABI load check is exact-match (abi == ABI_VERSION, cuda.rs:163-166), so a stale libqwen3_5_cuda.so fails at load with a "rebuild it" message rather than silently missing the new symbols. The rebuild requirement (even with the feature disabled) is correctly stated in the PR body.
  • ABI 6→8 skip is explained and verified: open PR #98 (gemm-fixed-algorithm, twu3202, ready) bumps the same two constants 6→7 (gh api pulls/98/files shows CS1_ABI_VERSION 6→7 in ops.h and ABI_VERSION: u32 = 6→7 in cuda.rs). #135 takes 8. Both PRs modify the same lines of src/backends/cuda/qwen3_5/ops.h and src/models/qwen3_5/native/src/cuda.rs — whoever merges second must renumber/rebase; the lockstep Rust/library agreement at each head is verified by inspection (ops.h:16 = 8, cuda.rs:12 = 8), and runtime agreement requires the rebuilt library (see unverified checks).
  • Degenerate zero-placeholder input diverges fail-closed: an empty feature matrix would be rejected by DeviceFeatureLayout::new (rows > 0) where the host path could theoretically accept an empty embedding slice; unreachable in practice because prepare_prompt/image_positions (multimodal.rs:49-52, 82-107) error on missing placeholders before any forward. Recording only, since it is a strict failure (error) not silent divergence.
  • Stacking/overlap with sibling PRs: #103/#106 (multimodal language CUDA Graph, same author, open) will need to handle the device variant or keep it eager deliberately; #127 (draft, "retain bounded vision cache across screenshot grids", same author) will tension with the borrow-based per-request view — a cross-grid cache holding device features cannot use the DeviceBf16Matrix<'_> borrow pattern and will need an owned-buffer design with its own lifetime review. #101 (draft, "ci: compile every declared CUDA backend") addresses the CI gap below.
  • Performance claims are author-reported and appropriately hedged (0.6–2.2% p50 reductions, concurrency 1, 2 rounds/mode, synthetic PNGs; round order host→device→device→host; an overlapping-compilation campaign explicitly excluded). Raw per-run evidence is not committed or linked; per the committed-artifact-hygiene rule, raw measurements/failures should live in a durable artifact or PR/CI evidence with the exact revision linked. The per-question validate_finite cost (4-byte cudaMalloc/free + full-matrix scan + sync per question) is disclosed by the author as remaining overhead; serving correctness is unaffected.

Skill load record

  • Skill: system1-omni-review, repository ThinkFlowLab/system1-omni, path /Users/hsliu/thinkflowlab/system1-omni/.agents/skills/system1-omni-review/SKILL.md (repository-tracked, published at working tree commit ca5f69b5c104ff2ed0c64ed2f195b49bec92e32c, local working tree clean for this path).
  • SHA-256 of bytes read: 58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108 — matches the requested value (shasum -a 256, personally run).
  • References actually read: docs/architecture.md (head of working tree), .agents/skills/system1-omni-review/references/repository-contracts.md, CONTRIBUTING.md, .agents/skills/self-review/SKILL.md §Committed artifact hygiene, src/runtime/README.md not read (no admission/dispatch change in this PR — runtime code untouched).

Commands run and results

Source inspected (personally, at refs/remotes/p6new/135 unless noted):

  • git fetch origin 98d0465...:refs/remotes/p6new/135 — new ref created; git rev-parse confirms head; git fetch origin main → 7cbce19 (unchanged); merge-base = 7cbce19.
  • git diff origin/main...refs/remotes/p6new/135 (685-line diff read in full) + --numstat (counts above).
  • Full head files read: qwen3_5/native/src/model.rs (forward_multimodal family, run, prepare_scratch, overwrite_image_rows), qwen3_5/native/src/cuda.rs (api! table, Stream repr(transparent), upload/download/copy_dd semantics, Drop), cua_s1/native/src/vision/model.rs (Scratch alloc sizes, read/synchronize/capture), cua_s1/native/src/vision_engine.rs (entire), backends/cuda/qwen3_5/runtime.cu copy/sync implementations, multimodal.rs placeholder construction, main's vision/model.rs run for before/after comparison.
  • Greps at head: new-symbol usage (zero external consumers), CUA_S1_DEVICE_FEATURES in *.md (zero), CUA_S1_GRAPH/CUA_S1_VISION_GRAPH docs (present), forward_multimodal_capture (present at main — from merged #108), CI workflow CUDA compilation (absent).
  • shasum -a 256 on the skill file — match.

Author-reported (not independently verifiable, no GPU/Rust toolchain on this host): 139 passed / 25 ignored workspace tests on macOS+Linux; RTX 4090/CUDA 13 kernel regressions incl. the new finite scan; 3+3 device/host parity tests with vision graph off/on (byte-identical features/hidden/logits/responses, scratch growth, injected capture failure, injected post-copy error recovery); 768-request HTTP campaign with 576 byte-identical comparisons; latency table above. Reproduction commands for the workspace checks are given in the PR body (including the SM 89 CUDA library build).

Commands NOT run (host constraints): all cargo commands (no Rust toolchain — forbidden on this host), any GPU/CUDA execution, mkdocs build --strict and the benchmark Python tests (no docs or Python/benchmark files changed in this PR; not applicable).

CI observed at this exact head

gh pr checks 135 and gh api commits/98d0465.../check-runs — all four check runs have head_sha == 98d0465de57d4d67c5a49f00cc5fb95526df5360 (exact-head verified):

  • rust — success (1m42s)
  • build — success (13s)
  • benchmarks — success (10s)
  • deploy — skipped (as on main)
    No "Exact-head fork workflow" annotations required: the runs are on the upstream repo at the exact fork head SHA. Note: per repository-contracts.md and ci.yml (no nvcc/build.sh step), CI neither compiles the changed CUDA sources nor runs GPU tests — the passing rust job validates neither the new ABI symbols' existence in a rebuilt library nor kernel behavior.

Material unverified checks

  1. GPU behavior of the entire residency path — no GPU on this host; every device-side claim (finite scan kernel, D2D residency parity, graph capture/replay interaction with the view, error draining under real queued work, scratch growth/recovery) rests on author-reported runs of an uncommitted overlay. Nothing GPU-dependent in this PR is verifiable from the repository.
  2. Rebuilt-library ABI 8 lockstep at runtime — cargo-based checks and the author's build cannot be reproduced here; a stale library fails cleanly at load (verified code path), but successful load with ABI 8 was not observed.
  3. CUDA compilation of elementwise.cu/runtime.cu changes — CI has no CUDA build step; compilation evidence is author-reported only (build.sh, SM 89).
  4. Performance table — author-reported; raw measurements not committed or linked; denominators, raw responses and manifest hashes not independently inspectable.
  5. Byte-identical HTTP responses vs main — author-reported (576 comparisons); not reproduced (no toolchain/weights).
  6. cargo fmt/clippy/test/build at this head — observed via CI (success), not personally run (no toolchain; ignored-test count inside the CI job not inspected).

@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 f548d01c94: one new commit since my review at 98d0465de — "test(cua-s1): cover resident feature validation and recovery" (15 files, +635/−3).

All three findings from my review are addressed, verified at the new head:

  • P2 (zero committed tests) — fixed: forward_device is now consumed by three committed test files and score_device by two; the weight-free validation tests (exact-extent DeviceFeatureLayout checks, overflow boundaries, device mismatch, env-flag re-probe asserting the exact must be unset, 0, or 1 error text) are host-runnable, and the GPU parity/recovery tests (device_features_gpu.rs, device_copy_failure_gpu.rs with the fail_device_copy.cpp LD_PRELOAD interposer per repo precedent) are correctly #[ignore]-gated with named CUDA + pinned-weight requirements and serial execution.
  • P3 (undocumented flag) — fixed: recipe/cua_s1/native_vision_graph.md now documents CUA_S1_DEVICE_FEATURES (opt-in, lifetime contract) and the backend README documents the ABI-8 requirement and the new cs1_get_device/cs1_bf16_finite entry points.
  • P3 (dead diagnostic surface) — addressed: score_device is now test-consumed (the DeviceBf16Matrix::download path is covered transitively; the unchecked stream-destroy in OwnedStream::drop remains, acceptable in a Drop impl).

I ran mkdocs build --strict on the full tree at this head: passes. Fork CI at this exact head: rust, build, benchmarks all pass.

No actionable findings remain. APPROVED. Unverified checks: the committed GPU tests' execution remains author-run (this host has no GPU — CI compiles but does not run the ignored tests); all GPU parity/failure-recovery behavior, the rebuilt ABI-8 library's runtime lockstep, and the perf table remain author-reported; the Rust suite is covered by the exact-head fork CI run. Provenance: review skill at main@7cbce19 (SHA-256 58be3bc6…dde108); remote head re-pinned immediately before posting.

Copy link
Copy Markdown
Contributor

Thanks for this. #135 can land on its correctness evidence; the A/B is only needed before we cite any speed numbers. When you have time, could you share one on the current head f548d01c (or the merged SHA if it lands first)?

  • Same build and weights, CUA_S1_DEVICE_FEATURES=0 vs =1, each with CUDA Graph on and off
  • Real screenshots, concurrency 1/4/16, at least 5 rounds per mode after an uncounted warmup
  • p50/p95 latency, throughput, peak GPU memory, and the byte-identical response count; for any response that differs, which request and how
  • The skipped GPU tests on that head (12 kernel tests plus device-vs-host matching, Graph on and off), with output pasted
  • Your setup: GPU, driver and CUDA version, weights, and which screenshot set you used

Please attach or link the raw results on the PR.

@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 aa5cf36123: the branch merged current main (through #138) and reconciles the backend ABI metadata in the merge resolution — reviewed directly: the resolved qwen3_5.backend.json declares abi_version: 8, consistent with ops.h's CS1_ABI_VERSION 8 and the README's ABI-8 requirement at this head (main's side carried ABI 6; the branch's resident-feature library supersedes it, and the models list from main is preserved). Both of my approved commits are patch-identical (git range-diff against f548d01c94); no new content. Fork CI at this exact head: rust/build/benchmarks/cuda-contract all pass.

The approval at f548d01c94 carries unchanged: zero actionable findings. Standing unverified checks (GPU test execution and parity author-run; ABI-8 runtime lockstep; perf table) carry with it.

One transition note, not a finding: main's PR template now (post-#138) requires the four human contributor answers and a completed self-review checklist before ready-for-review; this PR's body predates that and currently has neither the full answers nor checkboxes. Worth adding when the PR is next touched.

@Levius-Fubuki

Copy link
Copy Markdown
Collaborator Author

Current-head GPU A/B completed

Tested source: aa5cf36123c0e11b6a10aa44b2f3dc64f31b4974 (current PR head when frozen).
One RTX4090 24GB, driver 580.105.08, CUDA 13.0.88, SM89; Rust 1.98.0, BF16.
The same release worker/library and checksum-verified pinned base/adapter export
were used in all modes. Exact file hashes, versions and source inventory are in
environment.json, export-environment.json and build-and-weight-provenance.json.

Raw evidence archive · Release / report / SHA256SUMS

Archive SHA-256: c0578fddf4b923fa40e70db437fe1c57ac3c91dec4012811f4b1d76457c99325 (GitHub asset digest and local file hashes verified).

Correctness and raw evidence

  • 25 real GPU tests passed, 0 failed: 12 shared kernel/checkpoint/prefix tests;
    finite scan; 3 host/device feature/hidden/logit/probability/response cases with
    vision Graph off and the same 3 with it on; device/host vision replay and geometry;
    capture-failure recovery; real queued-D2D error, stream drain and recovery.
  • All 60 requested measured configurations completed: 5 rounds × 4 modes ×
    concurrency 1/4/16, with 32 requests and 80 questions per configuration.
    1,920 measured HTTP requests / 4,800 questions; zero failures.
  • 960/960 host-vs-device full body-byte comparisons match: 480 with vision
    Graph off, 480 with it on. Status and content type also match.
  • 480/480 host Graph off/on and 480/480 device Graph off/on comparisons match.
    Total across these four pair sets: 1,920/1,920 equal comparisons, zero differences.
    Request IDs, raw body bytes, hashes, timing and sampled memory remain in the archive.
  • The independent verifier recomputed every measured percentile, throughput,
    sampled memory maximum and comparison directly from the raw records.

Graph means vision Graph (CUA_S1_VISION_GRAPH); CUA_S1_GRAPH=0 throughout,
so multimodal language execution is eager. Uncounted startup, first real image,
four feasibility runs and per-configuration warmups are saved separately.
Feasibility traces confirm real-image vision capture/replay at grid [1,44,80];
measured worker logs contain no capture failure or fallback.
Mode/concurrency orders and the fixed run budget were frozen before execution.

Descriptive performance

Each entry is the median of five per-run statistics; all five values and ranges
are retained in summary.json. p50 is median request latency; p95 is nearest-rank
within each run. Requests mix 1 and 4 questions equally. Latency includes loopback
HTTP encode/send, full body receipt, JSON decode and committed wire validation.
Throughput is requests/s (question throughput is 2.5 times requests/s in this mix).
Peak memory is whole-device NVML used memory sampled at nominal 10ms;
it includes weights/workspaces/Graph runtime and is not an exact allocator peak.

Vision Graph HTTP concurrency Host p50 / p95 (ms) Device p50 / p95 (ms) Host / device req/s Host / device sampled peak (MiB)
off 1 266.746 / 376.464 263.910 / 363.597 3.748 / 3.809 10335.312 / 10335.312
off 4 911.636 / 951.133 878.728 / 912.858 4.350 / 4.512 10335.312 / 10335.312
off 16 3576.768 / 4092.263 3568.105 / 3971.791 4.301 / 4.424 10335.312 / 10335.312
on 1 264.344 / 376.596 260.952 / 366.026 3.789 / 3.851 10339.312 / 10339.312
on 4 911.675 / 948.395 881.576 / 918.873 4.355 / 4.503 10339.312 / 10339.312
on 16 3675.925 / 4102.126 3551.586 / 3982.748 4.299 / 4.427 10339.312 / 10339.312

This is a descriptive comparison of eight 1280×720 browser captures from Python
Documentation, Wikipedia and MDN, with top/scrolled views. It establishes
current-head output fidelity for this corpus. It does not establish GUI task
accuracy, statistical significance, production arrival/tail behaviour, other
screenshot geometries or other hardware. The GPU executor is serial; concurrency
includes CPU preparation and queueing and is not GPU batching.

The language export used the repository's original exporter with Torch
2.12.1+cu130 / torchvision 0.27.1+cu130, rather than the historical pinned Torch
reference environment. Both native controls use the identical exported files.
This campaign does not claim a fresh Torch-reference numerical/quality comparison.
Direct native failure-recovery tests passed; a separate HTTP fault-retirement
campaign was not run.

GPU command results

Test group Passed Failed
kernel-regressions 12 0
finite-device-scan 1 0
device-host-parity-graph-off 3 0
device-host-parity-graph-on 3 0
device-vision-graph-replay 1 0
host-vision-graph-replay 2 0
capture-failure-recovery 2 0
queued-copy-failure-drain-recovery 1 0

Full output is in evidence/gpu-tests/*.log; exact commands/controls are in
results.json, including the retained copy-drain.txt evidence. All code/source,
request manifests, screenshot attribution, raw HTTP/memory logs and reproduction
steps are in the downloadable evidence package. Runtime binaries, weights and
credentials are excluded.

Requested GPU stdout (full logs are in the archive)

12 shared kernel/checkpoint/prefix tests

running 12 tests
test device_copy_helpers_roundtrip ... ok
test flash_attention_matches_float64_reference ... attention t = 1, amplitude 8: largest relative difference 0.00e0
attention t = 63, amplitude 8: largest relative difference 3.46e-3
attention t = 65, amplitude 0.5: largest relative difference 4.16e-3
attention t = 139, amplitude 2: largest relative difference 4.85e-3
attention t = 700, amplitude 8: largest relative difference 3.38e-3
attention t = 2048, amplitude 0.5: largest relative difference 4.51e-3
ok
test fused_attention_gate_matches_separate_pass ... ok
test gated_delta_rule_matches_recurrent_reference ... gated delta t = 1, amplitude 1: largest difference 3.74e-4, largest |reference| 8.39e-2
gated delta t = 16, amplitude 1: largest difference 4.87e-4, largest |reference| 8.39e-2
gated delta t = 17, amplitude 1: largest difference 4.87e-4, largest |reference| 8.39e-2
gated delta t = 32, amplitude 1: largest difference 5.20e-4, largest |reference| 8.41e-2
gated delta t = 33, amplitude 1: largest difference 5.20e-4, largest |reference| 8.41e-2
gated delta t = 63, amplitude 1: largest difference 5.60e-4, largest |reference| 8.53e-2
gated delta t = 64, amplitude 1: largest difference 5.60e-4, largest |reference| 8.53e-2
gated delta t = 65, amplitude 1: largest difference 5.80e-4, largest |reference| 8.53e-2
gated delta t = 107, amplitude 1: largest difference 5.80e-4, largest |reference| 8.53e-2
gated delta t = 127, amplitude 1: largest difference 5.80e-4, largest |reference| 8.53e-2
gated delta t = 128, amplitude 1: largest difference 5.80e-4, largest |reference| 8.53e-2
gated delta t = 129, amplitude 1: largest difference 5.80e-4, largest |reference| 8.53e-2
gated delta t = 150, amplitude 1: largest difference 5.80e-4, largest |reference| 8.55e-2
gated delta t = 107, amplitude 1: largest difference 7.30e-4, largest |reference| 9.12e-2
gated delta t = 936, amplitude 1: largest difference 9.49e-4, largest |reference| 9.56e-2
gated delta t = 936, amplitude 1: largest difference 9.66e-4, largest |reference| 9.69e-2
gated delta t = 3399, amplitude 1: largest difference 1.15e-3, largest |reference| 9.60e-2
gated delta t = 65, amplitude 0.000000001: largest difference 2.26e-14, largest |reference| 4.05e-12
gated delta t = 65, amplitude 0.000000000000000001: largest difference 2.43e-32, largest |reference| 4.05e-30
gated delta t = 65, amplitude 0: largest difference 0.00e0, largest |reference| 0.00e0
ok
test gdn_conv_continuation_uses_cached_history ... ok
test gdn_empty_window_captures_state_copy_and_zero_initial_state ... ok
test gdn_prefill_two_phase_matches_one_shot_bit_for_bit ... gdn two-phase t = 107, split = 64: both phases bitwise equal to one-shot
gdn two-phase t = 936, split = 512: both phases bitwise equal to one-shot
gdn two-phase t = 3399, split = 1984: both phases bitwise equal to one-shot
gdn two-phase t = 65, split = 64: both phases bitwise equal to one-shot
ok
test graph_replay_reads_updated_inputs_after_failed_capture ... ok
test prefix_state_rejects_uncaptured_foreign_and_oversized_prompts ... ok
test residual_rms_norm_matches_rounded_reference ... ok
test silu_mul_matches_rounded_reference ... ok
test windowed_attention_matches_full_pass_bit_for_bit ... windowed attention t = 139, q_base = 64: rows >= q_base bitwise equal
windowed attention t = 712, q_base = 64: rows >= q_base bitwise equal
windowed attention t = 712, q_base = 128: rows >= q_base bitwise equal
windowed attention t = 972, q_base = 64: rows >= q_base bitwise equal
windowed attention t = 972, q_base = 896: rows >= q_base bitwise equal
windowed attention t = 2048, q_base = 64: rows >= q_base bitwise equal
windowed attention t = 2048, q_base = 1024: rows >= q_base bitwise equal
ok

test result: ok. 12 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 12.33s

Host/device matching with CUA_S1_VISION_GRAPH=0

running 3 tests
test vision_engine::device_feature_tests::device_features_host_parity_reuse_geometry_growth_and_recovery ... ok
test vision_engine::device_feature_tests::device_features_language_scratch_growth_and_short_prompt_recovery ... ok
test vision_engine::device_feature_tests::device_features_reject_nonfinite_and_recover ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 17 filtered out; finished in 64.42s

Host/device matching with CUA_S1_VISION_GRAPH=1

running 3 tests
test vision_engine::device_feature_tests::device_features_host_parity_reuse_geometry_growth_and_recovery ... ok
test vision_engine::device_feature_tests::device_features_language_scratch_growth_and_short_prompt_recovery ... ok
test vision_engine::device_feature_tests::device_features_reject_nonfinite_and_recover ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 17 filtered out; finished in 63.59s

The provided GPU instance has been shut down after artifact retrieval and independent verification; its provider console confirms the stopped state.

@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 8207cdf02a: the branch merged current main and the shared-vision-executor refactor; my approved content is patch-identical in range-diff, and the two substantive merge resolutions were reviewed file-by-file against both parents. Verified: the resident-feature implementation survives both folds intact (the cua_s1 vision wrapper over the shared executor, forward_device/score_device/CUA_S1_DEVICE_FEATURES parsing, and all four committed device-test files byte-identical to the approved state); ABI stays consistent at 8 across ops.h, the backend manifest and the README (the fold is a clean union of the branch's resident symbols with main's additive v2 endpoints); zero dropped lines in any resolution. The new native_screenshot.md documents the 1–8 question limit (verified in code) with honestly-scoped RTX 4090 evidence. mkdocs build --strict passes; fork CI rust/cuda-contract/benchmarks green at the exact head; MERGEABLE/CLEAN.

APPROVED (verdict carries). Transition note unchanged: post-#138 human answers and checklist to be added when next touched. Unverified: GPU execution and the RTX 4090 screenshot claims author-run; Rust suite covered by exact-head CI. Note for stacking: #127 rewrites the same cua_s1/shared vision files differently and will conflict when stacked after this lands.

@MrDongsls
MrDongsls merged commit 502ff3d into InferMatrixLab:codex/qwen-vision-27b Oct 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants