Skip to content

jemm: add native decision worker and pinned weight export - #118

Open
Levius-Fubuki wants to merge 18 commits into
codex/qwen-vision-27bfrom
codex/jemm-native
Open

Levius-Fubuki wants to merge 18 commits into
codex/qwen-vision-27bfrom
codex/jemm-native

Conversation

@Levius-Fubuki

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

Copy link
Copy Markdown
Collaborator

CPU maintenance and merge handoff — 2026-10-15

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

Normally merged the documentation-only #117 maintenance, preserving published history. The complete delta from this PR's previous head is src/backends/cuda/contract.md only; no model, device, test, build or manifest bytes changed. Review/landing order remains #117 → #118 → #122 → #123, with #143 branching from #118. #117 is held unmerged.

Current JEMM builds require ABI 8; earlier full-model A800 evidence belongs to ABI-6 source. Full current-source 27B text/1–4-image HTTP/reference parity and combined multimodal-language Graph qualification remain unverified. Rebuild all consumers with the library before a device handoff.

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 38667505304, Docs run 38667508947. 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: 3be46fa270cb74c33d4db4ffd761405d0522018a.

Merged #117 at fb74c54 through a normal merge, preserving the JEMM v2 unmerged LoRA/reference arithmetic, component projections and image-local multi-image attention. Resident outputs now use the same Buffers-owned storage and capture boundary as the shared executor. Both base and library now require ABI 8 and must be rebuilt together; the prior A800 evidence remains a historical ABI-6 qualification, not a fresh ABI-8 pass. The CPU fixtures initially failed on stale ABI 6/device-query stubs; updating only the fake-library interface restored the original LoRA and vision batching assertions without changing tolerances. Full local checks passed after that repair.

Personally run: format, strict workspace Clippy, workspace tests (240 passed, 0 failed, 47 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.

Applicability: the primary change adds a native JEMM worker; this template-only update adds no source bug fix. Historical multi-image numerical failures, corrections and their raw diagnostics are retained in the v2 evidence archive. The current operator checks and historical full-checkpoint checks retain their separate provenance below.

Purpose

Add a native JEMM worker for ordered text and 1–4-image decisions with pinned checkpoint export and whole-request serial admission. The v2 export preserves BF16 base weights and separate FP32 LoRA pairs. Original component GEMM shapes and aggregate vision projection rows are retained, with image-local attention.

JEMM opts into additive reference CUDA functions for normalization, GDN, attention, rotary and cuDNN patch convolution. Legacy Qwen entry points and default GEMM policy remain unchanged. Attention buffers and shape-owned cuDNN plans allocate before graph capture.

Scope and review map

System1-Omni Version / Commit: ed9f9d6eb566d226e2441c5e9d84187abe1e4d63.

Incremental baseline: shared vision #117 1dc77639; integrated main: 3e9cc33a61ddf110d97512fb6f296cf540377c61. The maintenance merge retains LoRA stream ownership and both text/multimodal Graph caches. Scratch growth and LoRA retirement drain before destroying captures and referenced buffers. The backend inventory includes JEMM and its optional reference sources.

This remains a large, coupled implementation: export precision, projection shapes and device arithmetic jointly determine parity. Suggested review order:

  1. Export/artifact contract, pinned inventory, and reusable fixtures.
  2. Unmerged adapter, component projections, and LoRA resource ownership.
  3. Aggregate vision rows, image-local attention and ordered image reconstruction.
  4. Optional reference CUDA API, preallocated workspaces, cuDNN plan ownership and executor selection.
  5. Processing/response reconstruction, calibration, and numerical regressions.

Historical Test Plan

System1-Omni Version / Commit: ed9f9d6eb566d226e2441c5e9d84187abe1e4d63.

Reproduction commands from the repository root; recorded local feature switches and device prerequisites remain scoped to the original results above. This PR-body update runs no additional source or device checks.

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 -m mkdocs build --strict

Use the pinned JEMM recipe for export and a fresh cuDNN-enabled ABI 6 library. The maintained fixed-seed operator runner is:

python3 tests/jemm/reference_kernels.py "$CUA_S1_CUDA_LIB" --report jemm-reference-kernels.json

It requires the recorded Torch/FLA environment. Full 27B text/1–4-image and combined v2 multimodal-language Graph qualification needs sufficient GPU memory; the RTX4090 operator checks do not substitute for it.

Historical Test Result

Local maintenance checks passed formatting, strict workspace/all-target Clippy, locked workspace tests, release build and strict MkDocs. The final workspace log records 231 unit/integration tests plus one compile doctest passed, with 37 device/checkpoint tests ignored.

Fresh RTX 4090 checks rebuilt the cuDNN/FLA reference path and passed eleven fixed-seed reference-kernel comparisons exactly. These checks validate the tested operator inputs; no 27B checkpoint was loaded, and they do not qualify full JEMM decisions, combined v2 multimodal-language Graph replay or performance on this head.

Demo / evidence

The archived A800 v2 campaign passed the frozen 16-request / 25-question text and 1–4-image corpus, with maximum probability drift 0.0166172279642417 < 0.02. Its actual HTTP pass used text-language and vision Graphs, with multimodal language eager. Two- and four-image outputs agreed with the reference to host floating-point noise; the maximum was three-image Choice.

That campaign used one exclusively reserved A800 80 GB PCIe, BF16 base/activations, FP32 LoRA, concurrency 1 and pinned reference/runtime hashes. The original probability/Score/winner-margin gates remain 0.02 / 0.1 / 0.05. The archive contains source/library/binary hashes, raw native and HTTP responses, operator checks, build logs and failed diagnostics. These are historical results for their recorded source and configuration, not a new full-checkpoint pass at ed9f9d6e.

Reproduction and original scope: pinned recipe, maintained corpus/oracle.

Limits and review

No new speedup claim is made. Current-head full-checkpoint parity/HTTP, broader labelled accuracy, throughput, repeated latency distributions, peak-memory comparisons, 27B checkpoint qualification and other accelerators remain unverified. Hosted CPU checks do not execute ignored device tests; prior hosted passes belong to their older SHAs.

Source-tree artifacts are reusable scripts and the synthetic frozen corpus/oracle. Historical failed evidence is retained. No real customer data or model credentials are included in the synthetic fixtures.

Contributor responsibility and maintainer review remain separate from this assistance.

Exact-head hosted verification — 2026-10-13

These are upstream workflow-dispatch results for the exact source head. Ignored GPU/checkpoint tests did not execute; maintenance status contexts link the actual runs.

Video: N/A for this nonvisual API, validator, shared executor or documentation change. Source-pinned tests and the linked validation records provide the relevant evidence; existing hardware and unverified-measurement limits remain unchanged.

Self-review

The contributor explicitly confirmed completion of self-review and authorized this checklist/template update on 2026-10-13 in the maintenance conversation. This records the human confirmation; it does not turn unverified measurements into passes or 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.

@Levius-Fubuki Levius-Fubuki changed the title jemm: add native text/image decisions with pinned export and A800 validation jemm: add native decision worker and pinned weight export Oct 8, 2026
@Levius-Fubuki Levius-Fubuki changed the title jemm: add native decision worker and pinned weight export jemm: add native text/image decisions with pinned export and A800 validation Oct 8, 2026
@Levius-Fubuki Levius-Fubuki reopened this Oct 8, 2026
@Levius-Fubuki
Levius-Fubuki force-pushed the codex/qwen-vision-27b branch from 907ffbb to 6480b6b Compare October 8, 2026 03:30
@Levius-Fubuki Levius-Fubuki changed the title jemm: add native text/image decisions with pinned export and A800 validation jemm: add native decision worker and pinned weight export Oct 8, 2026
@Levius-Fubuki
Levius-Fubuki marked this pull request as ready for review October 8, 2026 07:41
Copilot AI balanced review requested due to automatic review settings October 8, 2026 07:41

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.

@MrDongsls

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the implementation quality is high overall. The findings below are ordered by severity; "official" always means the pinned reference ypcypc/JEMM@6822fe0 (serve.py / contract.py read directly at that revision).

  1. Question-count cap 64 vs official 8: src/models/jemm/native/src/contract.rs:8 accepts 9–64-question requests with 200 + answers, while official contract.py (MAX_BUNDLE_QUESTIONS = 8) rejects them with 422 "UNSUPPORTED: bundle size". Every other UNSUPPORTED message matches the official text verbatim — this is the only one that doesn't. If the wider cap is intentional, please document why; otherwise align to 8.

  2. /health status literal "ready" vs official "READY": server.rs:100 returns {"status":"ready"}; official returns {"status":"READY"}. The frontend passes the health body through unchanged, so probes checking status == "READY" will mark this worker unhealthy. One-line fix.

  3. Error envelope shape: this PR returns {"error": <message>}, official returns {"error": <exception class>, "detail": <message[:200]>}. The 415 Content-Type gate (server.rs:85) is also outside the official status-code set {200, 404, 422, 500} (valid JSON without a Content-Type header: official 200, here 415). Since jemm: load and validate checksum-covered calibration #122 rewrites the envelope, please merge in order or note the divergence so clients don't have to adapt twice.

  4. Warmup never exercises the vision path: the warmup payload (server.rs:16) is text-only, so the new 27B v2 VisionModel::forward / forward_multimodal first runs on the first user image request. Combined with retire-on-any-execute-error (executor.rs:279: state.take() + ready=false), a defect that only shows up on a real image forward makes the worker permanently 503 from the first image request — while /health reported ready until then. Suggestion: add one minimal synthetic image to the warmup set.

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

Reviewed at head ef1fd634eb (stacked on #117; 16 files, +4168/−1). A disciplined, faithful native JEMM port — I verified the contract against upstream ypcypc/JEMM@6822fe0f (32 candidates A–Z+012345, choice/score/noul, first-argmax tie-break, confidence = max probability, expected_value = Σ level·p, noul reads "yes", per-modality temperature, 16 MB body) with no foreign model rules imposed. Export integrity is unusually strong: pinned file SHA-256s + full tensor inventory, FP32-delta-into-cloned-BF16 safe merge, manifest with export_sha256 over every artifact, all re-verified before device init; the worker requires real warmup before readiness. Authored count 1,641 / total 4,169 matches my independent computation; artifact hygiene clean; CI runs verified SUCCESS at this exact head (workflow dispatch — the stacked base keeps the PR rollup empty).

One P2 blocks my approval — it is honestly self-disclosed, which is why it is a P2 and not a P1:

  • src/models/jemm/native/src/executor.rs:159-183 — multi-image native-vs-reference parity exceeds the model's own predeclared 0.02 gate at 2 and 4 images (max drift 0.0421; 3 images pass; the single-image corpus passes at 0.0015). Preprocessing is bit-exact (tokens/positions/grids/pixels) and the #118/#122 binaries return bit-identical answers, isolating the deviation to this executor/merge numerics. Reference parity is this worker's core claim and multi-image (up to 4) is a supported input class: root-cause the discrepancy or record an explicit, documented limitation (in the eventual src/models/jemm/README.md) before merge.

Also needed before merge: jemm has no in-repo documentation or repository tests anywhere in the open PR set (CONTRIBUTING's adding-a-model requirements: model README, recipe, supported-models/mkdocs/README entries, tests). File the linked follow-up PR — or include them here.

Everything else was verified clean: all validation maps to 422 at this head (jemm's own convention), the pyrepr short-escape gap only affects control characters inside container-shaped instruction text (noted upstream of any parity claim), and no run output is committed.

Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108) and its repository map at trusted base 4a79980d8a75; remote head rechecked before posting; GPU campaigns author-reported, not re-run (no GPU on this host).

@Levius-Fubuki

Copy link
Copy Markdown
Collaborator Author

@hsliuustc0106 already fixed 👀

@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 4760abf44a after the branch rebase (prior review: COMMENTED at ef1fd634eb, 2026-10-08). Series identity: git range-diff of the previously reviewed head vs the new head shows every commit patch-identical and none added or dropped — a rebase, not new work. Provenance: review skill at main@4a79980d (SHA-256 58be3bc6…dde108); remote head re-pinned immediately before posting.

Prior finding status — still open (no content change):

  • P2: multi-image native-vs-reference parity exceeds the predeclared 0.02 gate at 2 and 4 images (max drift 0.0421) — root-cause or a documented limitation is still required before merge.
  • CONTRIBUTING adding-a-model gap: no in-repo jemm documentation/repository tests yet (follow-up PR still needed).

No CI has reported on the rebased head yet; the workflow-dispatch CI run verified at the prior head covered this identical series. Unverified (unchanged): GPU behavior and parity numbers author-reported; no GPU on this host.

Verdict: COMMENTED (P2 open).

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

PR #118 — incremental re-review #4 (jemm: add native decision worker and pinned weight export)

Reviewed at head 06ea197326d53416e90df282249a3c92eb947d8a (remote head re-pinned via gh pr view immediately before reporting; PR state OPEN, non-draft). Prior reviews: 2026-10-08 COMMENTED at ef1fd634eb; 2026-10-10 re-review at 4760abf44a (P2 open).

Incremental scope

The new series is 8 commits on top of the previously reviewed head; 4760abf is a direct ancestor (96659ef/9d4436e both parent it), so the prior reviewed content is unchanged and the authoritative delta is the tree diff 4760abf..06ea197: 37 files, +5241/−161. Authored code per CONTRIBUTING's definition (excluding 194 lines of docs and 544 lines of static parity fixtures): 4,664 lines across 31 files; total series delta 5,402 lines. The PR body reports PR-base counts 6,786 authored / 10,188 total, 59 files with a split rationale ("export precision, projection shapes and device arithmetic jointly determine JEMM parity"), a numbered component map with review order, per-area validation with commands/results/unverified items, and a self-review section — the large-code-change preparation requirements (>3,000 authored lines) are met.

Commits: 9d4436e (separate FP32 language LoRA), 96659ef (v2 exports preserve original weights + FP32 adapter), f74a7f9 (merge), a3c7a78 (original base projection shapes for unmerged models), db400fb (all-image projection shapes in 27B vision), 38c0206 (merge), 3a2ff03 (Jemm vision projections over all request images), 06ea197 (unmerged multi-image execution aligned with reference numerics). I diffed both merge commits against each parent: both are clean unions — each shows only the other parent's files; neither introduces conflict-resolution content of its own.

Provenance: canonical .agents/skills/system1-omni-review/SKILL.md (SHA-256 58be3bc6…dde108, verified against bytes read), its repository map, docs/architecture.md, CONTRIBUTING.md. I read from pinned refs (refs/remotes/p3old/118, refs/remotes/p3new/118) and an extracted copy under /tmp; the repository was not modified.

P2 resolution assessment — resolved to the standard of this host

The prior P2: multi-image native-vs-reference parity exceeded the predeclared 0.02 gate at 2 and 4 images (max drift 0.0421; single image 0.0015; 3 images passed), with preprocessing bit-exact, isolating the deviation to executor/merge numerics. I required root-cause or a documented limitation.

The series takes the root-cause path, and the mechanism matches the failure signature. Verified in code:

  1. Merge rounding removed (v2 exports). recipe/jemm/export.py deletes safe_merge entirely; the 17 language shards are published byte-for-byte (atomic_original, hardlink-else-copy) and checksum-compared to the pinned base inventory, and the raw FP32 adapter_model.safetensors is preserved the same way. src/models/jemm/native/src/artifacts.rs::lora_contract enforces jemm-native/2, unmerged_fp32, rank 16 / 496 pairs / scale 2, absence of lora_merge, and adapter SHA-256 equal to the pinned inventory — before any file or device access (covered by new tests). This removes the load-time BF16 rounding of W0 + B·A that the reference (FP32 LoRA applied at runtime) does not have.
  2. Runtime FP32 LoRA (src/models/qwen3_5/native/src/lora.rs). Strict parser (prefix/target allow-list, exact shapes vs config, rank ≤128, offset/size bounds, per-element finiteness, A/B pairing, header ≤8 MiB, whole-file size bound, safetensors re-validation). At runtime: BF16→FP32 input lift, per-prompt FP32 A and B GEMMs (each prompt keeps its own M, including inside packed projections), and a single BF16 rounding at the add into the BF16 projection output (cs1_vision_lora_add), with copy2d gather/scatter for offset components inside fused outputs.
  3. Reference projection shapes (model.rs::gemm_group → lora::project_components). Fused qkv/in_proj_qkv/gate_up GEMMs are decomposed into the original per-component GEMMs with original M/N/K and contiguous input/weight/output strides ("A strided GEMM output itself can choose a different cuBLAS plan"), then copied into the fused scratch. This restores the reference's unfused GEMM plan selection.
  4. Reference arithmetic opt-in (enable_reference_numerics + additive cs1_reference_* ABI-6 group). Resolved by symbol presence with a cs1_reference_available() capability check (requires a cuDNN 9 build; missing-capability error names the fix). Reference rms_norm/add_rms_norm (FP32 reduction order, BF16 residual-add boundary), reference GDN prefill (chunked), reference language attention (fixed-order split combine), reference attn_prep, a separate GEMM handle whose reduction-scheme mask is the full CUBLASLT_REDUCTION_SCHEME_MASK (legacy language excludes INPLACE; vision NONE) — i.e. the framework's heuristic — and vision reference norm/rotary (FP32 angles, device trig)/attention/cuDNN Conv3D patch plans. Guards: only the unmerged 27B models (language hidden 5120/head_dim 256 with LoRA present; vision hidden 1152 unadapted), before first forward, scratch/graphs empty; prefix caching is explicitly rejected on this path and documented. All legacy call sites use map_or fallbacks — Cua-S1/Open-Jev/JEV-VL behavior is unchanged.
  5. All-image vision projections (vision/model.rs::forward_images). 2–4 unadapted 27B images are concatenated into aggregate projection rows (patch conv + linears) matching the reference's joint projection shapes, with image-local attention per image (BatchGeometry, per-image patch counts, ≤65536 aggregate patch bound, separate 4-entry scratch FIFO keyed by ordered grids, graph capture/replay supported). Single image delegates to the unchanged forward; empty input does nothing. This explains the old failure pattern precisely: with one image, per-image and aggregate GEMM shapes coincide (hence the 0.0015 single-image pass); at 2/4 images the per-image loop changed cuBLASLt plan selection (the non-monotonic 3-image pass is consistent with heuristic plan choice). The retained diagnostic records in the evidence archive attribute the large divergences to vision MLP linear_fc2 — a projection GEMM — consistent with this.

Multi-image coverage in the pinned corpus: yes. tests/jemm/data/parity/corpus.jsonl (16 requests) now includes multi-image-2/3/4 (each exercising choice, noul and score) alongside two single-image and eleven text cases; tests/jemm/data/parity/reference.json is the 16-entry official-reference oracle with 1:1 id alignment (I verified both, plus the corpus SHA-256 82c05de9cb21b7763864dde6e80cc6bed588219d4d53da2d8157c0ec27a3663c claimed in the evidence doc). tests/jemm/parity.py is sequential, retry-free, checks identity/order/exact usage, and enforces the unchanged predeclared gates (0.02 probability/confidence/noul, 0.1 score EV, winner agreement at ≥0.05 reference margin; low-margin cases retained) and records corpus/reference/runner hashes. I exercised compare() locally: an exact replay passes and a +0.05 perturbation trips the probability gate.

Author-reported result (not independently runnable here): one A800 80 GB pass over the full corpus, max probability drift 0.0166172279642417 < 0.02, both eager and through the real HTTP worker with graphs enabled; 11 fixed-seed operator checks (Torch/FLA oracle, graph replay equal to eager). Per the prior review's standard ("author-reported GPU numbers are evidence gaps unless disclosed with archives"), the archive is disclosed: fork release jemm-reference-20261011 contains the raw evidence tgz + SHA-256 + report.md + predeclared protocols, with failed diagnostic candidates retained and the old failing v1 evidence explicitly kept and marked "not evidence for the new v2 path". I downloaded report.md from the release; its headline drift matches the in-repo claims. Residual caveat: this is author-run evidence on one GPU/framework combination, and the in-repo docs say so; nothing on this host can re-run it.

Both src/models/jemm/README.md and recipe/jemm/README.md document the root cause, the v1→v2 semantics, the legacy-export rejection, the cuDNN requirement, and the validation scope.

CONTRIBUTING adding-a-model gap — correction, plus one new residual

Correction to my prior reviews: at both 4760abf (old head) and the new head, the previously-missing registration already exists — root README.md (line 169), docs/supported-models.md (line 20), mkdocs.yml nav (line 72), recipe/README.md (line 20), src/models/jemm/README.md, recipe/jemm/README.md, and repository tests under tests/jemm/. My 2026-10-10 restatement of "no in-repo jemm documentation or repository tests" did not match the tree at 4760abf and is withdrawn. That portion of the gap is closed (and was, before this series).

New residual introduced by this series (P3-1 below): the two registration surfaces that point at the old v1 story were not updated and now contradict the docs this PR rewrites.

New findings

  • P3 — stale/contradicted registration docs and a dead anchor, introduced by this series.
    • README.md:169: "text/single-image parity on A800; known multi-image limitation" — the target section was retitled by this series (old "## Validation and known multi-image limitation" → "## Validation"), so the anchor is dead, and the claim is now false (the v2 path validates 1–4 images within gates).
    • docs/supported-models.md:20: "text/single-image corpus; two-/four-image parity limitation" and "Pinned merged export" — both now false (1–4-image campaign; v2 is unmerged; the worker rejects premerged exports). It also omits the new cuDNN-9/reference-symbols requirement.
    • Consequence: broken link and a registration row that contradicts the model README/export contract the same PR establishes. mkdocs build --strict does not catch it (anchors into unpublished src/ pages are not validated). Fix is two lines; should land in this PR.

Verified-acceptable, not findings: the breaking jemm-native/1 rejection is deliberate, loudly surfaced at load before file/device access, tested (tests/jemm/artifacts.rs, tests/jemm/tokenization.rs), and documented in three places; there are no released consumers of v1. The fake .c interposers (tests/qwen3_5/lora_fake.c, vision_batch_fake.c) compile at test time with the host cc, and the tests parse cuda.rs's api! block and stub every unimplemented cs1_* symbol to return an error, so an unexpected dispatch fails rather than fake-passing; real kernel tests still require the real library and remain #[ignore]-gated. tests/jemm/test_export.py is not wired into CI (needs torch/safetensors), consistent with repo precedent (only tests/benchmarks Python runs in CI) and documented in the recipe's checks section.

Evidence (commands run on this host)

  • git rev-parse both pinned refs (match the brief); git diff --stat/--numstat p3old..p3new; per-commit --stat for all 8 commits; git diff <merge>^1 <merge> and <merge>^2 <merge> for f74a7f9 and 38c0206 (clean unions).
  • Read from the pinned head: lora.rs, model.rs diff, vision/model.rs/geometry.rs diff, cuda/reference.rs, cuda.rs diff, executor.rs, artifacts.rs diff, processing.rs diff, export.py diff, ops.h/build.sh/gemm.cu diffs, backend README, both jemm READMEs, evidence doc, all new test files and fakes; structural skim of reference_gdn.cu, reference_norm.cu, reference_vision_attention.cu, and the entry points of the remaining kernels.
  • Python (/Users/hsliu/miniconda3/bin/python): corpus/reference id alignment (16/16, multi-image 2/3/4 present); corpus SHA-256 matches the evidence doc's pin; parity.py::compare exercised (exact replay passes; +0.05 perturbation trips the probability gate; low-margin winner logic confirmed in code).
  • mkdocs build --strict on the extracted head: pass (INFO only; the new evidence page sits outside nav, matching the existing docs/benchmarks/ precedent).
  • gh pr checks 118 --repo ThinkFlowLab/system1-omni: 3/3 pass — fork rust (fmt, strict Clippy, locked tests, release build), docs (strict), benchmarks, all annotated "Exact commit"; I verified run 38071297926's head_sha equals 06ea197326d….
  • gh release view/download on the fork: evidence release exists (tgz + sha256 + report.md + final-pr-status.json); report.md headline drift 0.0166172279642417 matches the in-repo claims.
  • Author-reported (PR body, corroborated by archive): Rust workspace 173 passed / 0 failed / 24 ignored; exporter tests 6 passed; legacy CUDA kernel regressions 11 passed; sm_80 builds with and without cuDNN.

Material unverified checks

  • Kernel numerics: the reference CUDA kernels compile into the CI build but their arithmetic is not verifiable on this host; correctness rests on the author's 11 operator checks against the pinned Torch/FLA oracle (with graph-replay-equals-eager assertions) plus the parity campaign.
  • The GPU parity campaign itself is author-reported; I read the release's report.md but did not download/re-derive the full evidence tgz, and no GPU re-run is possible here.
  • tests/jemm/test_export.py locally: this host's torch install is broken; not run. Covered by the author's environment (6 passed) but not by CI.
  • Rust suite locally: no Rust toolchain by host policy; covered by the fork CI run at the exact head (green), with ignored counts as reported.
  • Registration staleness (P3-1) means the root README/supported-models validation claims should not be quoted until fixed.

Verdict

The blocking P2 from 2026-10-08/2026-10-10 is resolved: root cause identified and addressed at three coherent levels (unmerged FP32 adapter, reference-shaped projections/reference kernels, aggregate multi-image vision projections), multi-image cases are now in the pinned corpus under the unchanged predeclared gates, and the author-supplied evidence is archived and internally consistent. One new P3 (stale root-README/supported-models registration + dead anchor) should be fixed in this PR before merge. No other new findings in the delta; both merge commits are clean.

@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 91cc5c2574: one new commit since my review at 06ea197326 — "docs(jemm): align registration with v2 multi-image validation" (2 files, +2/−2).

This resolves the single P3 from that review, verified in the diff: README.md's JEMM row now claims "text and 1–4-image frozen-corpus parity on A800" linking to the existing ## Validation section (anchor resolves); docs/supported-models.md drops the false two-/four-image limitation, fixes the same anchor, and now states the actual v2 contract ("Pinned jemm-native/2 export with BF16 base and unmerged FP32 LoRA … cuDNN 9 and additive reference CUDA symbols"), including the cuDNN-9 requirement my P3 flagged as omitted. I ran mkdocs build --strict on the full tree at this head: passes.

No actionable findings remain. Recap of the review trail at this head: the 2026-10-08 blocking P2 (multi-image parity drift 0.0421 > the predeclared 0.02 gate) was resolved by the 06ea197 series — root-caused (export-time BF16 merge of FP32 LoRA + fused-projection GEMM plan drift at multi-image shapes), fixed at three coherent levels (v2 unmerged exports with byte-preserved original shards and raw FP32 adapter; runtime FP32 LoRA with a single BF16 rounding at the add; reference-shaped component GEMMs plus the additive ABI-6 cs1_reference_* kernel group and all-image batched vision projections), with multi-image 2/3/4 cases now in the pinned 16-case corpus under the unchanged gates — see my full incremental review of that series (posted at 06ea197) for the evidence and the withdrawn CONTRIBUTING claim.

APPROVED. Unverified checks: the reference CUDA kernels' arithmetic is not verifiable on this host — correctness rests on the author's 11 fixed-seed operator checks against the pinned Torch/FLA oracle (with graph-replay-equals-eager assertions) plus the parity campaign; the A800 campaign itself is author-run (max probability drift 0.0166172279642417 < 0.02), disclosed as a public fork-release evidence archive whose report I checked against the in-repo claims, but the full tgz was not re-derived and no GPU re-run is possible here; tests/jemm/test_export.py is not in CI (torch dependency, repo precedent) and could not run locally; the Rust suite is covered by the exact-head fork CI run at 91cc5c2 (rust/docs/benchmarks all pass). Merge in stack order (#117 → #118 → #122 → #123).

Provenance: review skill at main@7cbce19 (SHA-256 58be3bc6…dde108); remote head re-pinned immediately before posting.

@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 after the rebase wave (prior review: APPROVED). Wave-verification note: this pass reviewed the whole 10-branch rebase wave systematically — every range-diff shows the own-commit series patch-identical; all post-pin merge resolutions were checked file-by-file against both parents (source/test/config/docs) with zero dropped lines; approved-content markers verified present in every tree (jemm v2 export, decider fixture READMEs, serving-matrix docs, vision FIFO regressions). Fork/upstream CI states as noted per PR.

All commits are patch-identical to the approved series (range-diff, named refs, cat-file verified); the stack merges' resolutions were checked file-by-file with zero dropped content, and the approved markers (the v2-series content) remain in the tree. Exact-head upstream maintenance workflows (ci + docs) pass at this head, posted by the author; earlier exact-head fork CI also green.

APPROVED (verdict carries — content-identical rebase). Transition note: the body predates the post-#138 template — add the four human contributor answers and the completed self-review checklist when next touched. Standing unverified checks unchanged (GPU evidence author-reported; no Rust toolchain on this host).

@MrDongsls

Copy link
Copy Markdown
Collaborator

Re-checked PR #118 at head 5bdd625b against the previously approved 3be46fa. The two-commit increment changes only src/backends/cuda/contract.md; it clarifies that the CPU cuda-contract checks are wired into CI and removes the stale ABI-4 example in favor of the current manifest/header contract. No source, test, manifest, or runtime bytes changed in this increment.

I did not find a new actionable issue, and the previously reviewed multi-image parity and registration-documentation findings remain resolved. Existing GPU qualification limitations are explicitly disclosed and are not reopened here.

The previous approval remains valid for this documentation-only update.

@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 5bdd625b4c after the rebase wave onto main@e66ff34 (prior review: APPROVED). 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.

This branch inherits the verified 3be46fa2 merge plus trivial merges; the jemm v2 markers (export.py without safe_merge, jemm-native/2, untied head, recipe docs) are present in the tree, and the FP32-GDN/resident-feature content from the shared line is unioned, not dropped. Exact-head maintenance workflows (ci + docs) pass, annotated "no GPU qualification" as usual.

APPROVED (verdict carries — content-identical rebase). Transition note unchanged: post-#138 human answers and checklist to be added when next touched. Standing unverified: GPU evidence author-reported; no Rust toolchain on this host. Stack order #117 → #118 → #122 → #123 still applies.

Copy link
Copy Markdown
Collaborator Author

Validate the current integrated JEMM source before extending qualification

The earlier full-model numerical evidence belongs to an ABI6 source, while this branch includes the ABI8 shared-vision integration. Please retain the historical results under their original source identity and qualify the current combination separately.

Preserve the coupled contract: original BF16 language weights, separate FP32 adapters, component GEMM dimensions, image-local attention and ordered reconstruction.

Required coverage includes the cuDNN9-enabled build, reference operators, selected label head, and the complete 16-request/25-question corpus with 1–4 images. Compare against both the frozen oracle and freshly captured official-reference responses on the declared environment.

Keep the existing numerical gates and include growth/reuse/recovery sequences. The dependency on #117 and its HOLD remains applicable.


Reviewed head: 5bdd625b4c27efb02f43a1f71150e1a7d7c0f071.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants