Repository navigation
Conversation
Extend the reference worker's contract with the two remaining Valen task types alongside choice: noul (fixed or custom true/false wording, answered as P(true) plus the probability pair and confidence) and score (2-10 ordered levels, per-level branches decoded as a cross-level softmax with the expected level, legend and concentration confidence, formulas copied from the pinned reference). Mixed-type requests share one state encoding; usage keeps the logical/compute split. Choice behavior is unchanged (the image/text parity blocks match the merged records byte for byte); noul and score parity against the pinned reference passes at 1e-6 on RTX 5060. 47 CPU tests; compare_reference gains --case noul|score|all with new example requests.
The cardinality check's message read like the satisfied condition; name the requirement so the 500-side error is actionable. (cherry picked from commit 3afa240)
The noul/score blocks were originally recorded from a working tree, and the rebase onto current upstream main (the reference slice landed as InferMatrixLab#115) orphaned the earlier verification SHAs. All four parity blocks and the frontend byte-identity were re-verified verbatim at 3afa240; the records now reference that tree. (cherry picked from commit 28072dc)
… the PR Align the noul/score documentation with the d3 recipe convention: the recipe records the procedure, commands and fixtures, while measured run output — the observed responses, the verbatim noul/score parity blocks and their provenance SHAs — lives in the pull request description. The model README drops its duplicate request examples and states the compiler record rules and answer fields in compact bullets. Only content this branch added or rewrote changes; the InferMatrixLab#115 slice's records stay as they were.
Address the review findings on compare_reference: dispatch the per-type comparison on the type requested in the wire body (rejecting answers that drift to another shape instead of silently skipping the noul/score checks), and include the confidence delta in the pass criteria with its own tolerance verdict line.
The four compare_reference blocks were re-run verbatim from the committed tool at 507c448; every block now carries the confidence tolerance verdict and the criteria paragraph states the requested-type and confidence gates.
The LoRA merge bf16(fp32(W) + 2*(B@A)) is verified bit-equal against the pinned peft 0.18.1 Linear.merge on synthetic tensors; the FP32->BF16 downcast mirrors the reference's own from_pretrained cast. Checkpoint keys decompose into LoRA pairs, vision-top blocks 20-23, merger and head with strict rejection of anything outside the pinned vision_top layout.
run_export writes model.safetensors (merged backbone in the base's own key layout, BF16 only), the fp32 head.safetensors, the copied base config and the valen_export.json contract through an incremental safetensors writer, tensor by tensor in sorted order, so host memory stays far below the 4.5 GiB artifact set. validate_export re-reads headers and digests and rejects tampered bytes, unlisted tensors, downcast-list drift and head-config drift. The tiny synthetic fixture proves determinism and path independence.
export() hashes checkpoint.pt and the base shard, checks them against training_metadata.json, valen_manifest.json and the checkpoint's own base_manifest (both record per-shard sha mappings), refuses any stage but the pinned vision_top layout, and records the revisions, repo and tool versions in the manifest sources. main() runs it as the one-command recipe. The env-gated regression (VALEN_CKPT/VALEN_BASE) exports the real artifacts twice and requires byte-identical output plus the pinned inventory; run on this host it passed in 3m33s with all four files identical across runs.
Add recipe/valen/verify_export_parity.py: it loads the original pinned stack (ValenExecutor: base + unmerged FP32 LoRA + checkpoint) and the exported stack (from_pretrained(weights/valen-export) backbone + DecisionHead rebuilt from head.safetensors, run through the pinned Valen class) serially, one subprocess each so the ~6.6 GiB VRAM footprint never overlaps, answers the recipe's existing text and image choice fixtures through the pinned Compiler, and gates on exact answer agreement plus probability deltas <= 1e-6. It also measures the merge drift of all 186 merged keys on the real tensors (research-001 method: unmerged PEFT semantics vs the exported BF16 GEMM, 256 bf16 probe rows, seed 0, CPU single thread), cross-checks every exported tensor bit-exactly against the merge math, fills the manifest's parity slot with the verdict and re-validates the artifact set. tests/valen/test_export_parity.py covers the pure contract CPU-only (bf16 ulp ruler, per-module drift against peft 0.18.1's Linear.forward, aggregation, case verdicts, parity record + slot filling) and wires the env-gated full-artifact regression (VALEN_CKPT/VALEN_BASE/VALEN_SRC + CUDA), asserting the recorded verdict's internal consistency rather than a desired outcome.
Add recipe/valen/export.md (usage, artifact inventory, manifest schema, merge decision, downcasts, head, consumption, pins/licenses, parity pointer) and recipe/valen/test_export_reference.py, which chains the tests/valen/test_export.py battery and the env-gated real-artifact determinism gate and validates an exported set through the tool's own validate_export. Link the export from recipe/README.md, src/models/valen/README.md and one line in reference.md.
Append the five-file export-parity block to recipe/valen/reference.md: the two serially loaded stacks (original ValenExecutor build vs the exported artifact set's documented consumption path), the verbatim gate output on the RTX 5060 host (text delta 7.06e-04, image delta 1.73e-02, exact answer agreement on both fixtures), the 186-module merge-drift aggregate (max one BF16 ulp per adapted module, ~53% bit-exact, worst relative drift 7.6e-3), the control checks (same-stack bit-identical re-runs, original stack matching the committed serving records, exported tensors bit-checked against the merge math), and the honest verdict: the declared 1e-6 tolerance fails, so the export fallback (separate FP32 LoRA export, unmerged serving) is triggered; the merged set keeps exact discrete answers and parity.status = failed in the manifest.
…iles The manifest cross-check read every artifact whole (read_bytes), pulling the ~4.5 GiB model.safetensors into host memory and contradicting the documented headers-and-digests-only contract. Sizes now come from stat() and digests from the module's streaming sha256_file, and the per-tensor safetensors header check reuses one handle per file instead of reopening per tensor.
A missing training_metadata.json silently skipped the checkpoint digest check right before a weights_only=False unpickle, and the base weights pins were conditional on both sides declaring a digest. Following the worker precedent (engine.py pins unconditionally), a missing training_metadata.json or an undeclared base digest is now an error; the doc's pin paragraph says so.
verify_sources_match_manifest checked the checkpoint sha and base revision strings but not the base weights digests the manifest records, although 392 tensors pass through the export byte by byte. The gate now re-hashes the base shard against sources.base.weights_sha256; tampered records or swapped shard bytes are rejected before any model loads.
- validate_export counts every directory entry, so a stray subdirectory no longer bypasses the exactly-four-files audit; - run_export cleans up its partial output on failure (removing a directory it created), so a failed run never blocks the retry; - classify_checkpoint_weights rejects vision blocks outside 20-23, not only sub-20 blocks, matching the pinned vision_top inventory; - drop the unused BACKBONE_PREFIX/VISUAL_PREFIX constants and the unused cuda mem_get_info free byte.
The fallback paragraph claimed the gate passes and the fallback is deliberately unimplemented, contradicting reference.md's recorded FAILED verdict (max probability delta 1.7e-2). It now states the failure and leaves the fallback decision to the spec owner. The suite count is updated to the measured 105 passed / 2 artifact-gated skips.
The recipe regression script introspects tests/valen/test_export.py and injects fixtures by parameter name, but only knew tmp_path; the new streaming/cleanup tests take monkeypatch and failed under the script while passing under pytest. The battery now hands each test a fresh pytest MonkeyPatch (undone after the test) for that parameter.
Review follow-ups on the export recipe, kept minimal: - run_export now stages the artifact set in a sibling directory and moves it into place with one rename, so a hard-killed run can no longer strand partial files under --out (a stale staging directory is cleared by the next run with the same target). - run_export rejects base_config_path=None instead of writing a set its own validate_export refuses (the allowed set includes config.json). - compare_case fails a case whose probability lists do not cover every candidate instead of silently truncating the delta scan. - export.md notes the artifact set deliberately excludes tokenizer and processor files (reconstructing the serving stack reads them from the pinned base) and records the new suite count (108 passed, 2 skipped).
VisionConfig now validates the shared Qwen3.5 vision layout with out_hidden_size configuration driven (must equal the text hidden size): 2560 keeps the Cua-S1 4B path byte-identical, 2048 accepts the Valen 2B. The forward, scratch and LoRA work buffers take their sizes from the validated config instead of the pinned 4B literals; merger fc1/fc2 use the HF intermediate/out dims. CPU tests cover both layouts, the text-hidden invariant and the still-pinned kernel dimensions.
ValenVisionCheckpoint loads the five-file export (valen-checkpoint-export/2) as the vision source: format, checkpoint/base provenance pins and every listed file's size+sha256 are verified before tensors are read (reusing the cua_s1 export-policy file check), and the manifest's visual tensor records must match the configuration-derived inventory with blocks 20-23 and the merger recorded as checkpoint-sourced replacements. VisionModel::load_valen uploads the same base layout with no adapter; the trait seam keeps the Cua-S1 base+adapter path untouched. CPU tests cover the pure manifest validation with synthetic corrupt manifests.
…e (N4) - omni-valen-native gains the execution layer: Engine (text/image paths converging at the multimodal language boundary), the CPU FP32 decision head over the export's head.safetensors, the reference-finisher with CPython-repr float formatting, and the /v1/systemone HTTP surface with the reference worker's exact gate order and error bodies over a stub inference seam. - shared qwen3_5 model: additive forward_positions / forward_multimodal_positions (multi-row final-norm readout), no ABI change. - CPU suites: tests/valen/worker.rs (contract + finishing + head fixture), frontend integration tests/frontend/native_worker.rs (direct vs proxied byte identity). - recipe/valen/dump_choice_parity.py records the reference choice answers for the end-to-end GPU gate; env-gated tests/valen/worker_gpu.rs consumes it.
Fixture recipe/valen/fixtures/choice-parity-reference.json from the pinned reference (RTX 5060 Laptop, sm_120, BF16, eager, TF32 off): - text choice: answer exact (r1_and_r3, reference margin 0.514), max probability delta 8.713e-3, confidence 1.743e-2. - image choice: max probability delta 7.629e-2; the answer flips on the reference's own near-tie (margin 0.108) — declared in the fixture (declared_flips.image), attributed by the decomposition diagnostic: pixels bit-exact, tower drift 1.03125 (the declaration on this fixture), language+head on reference tower rows within 9.2e-3 logits. - head probe: 4.530e-6 max logit delta on the reference's own rows. Declared tolerances: probability 0.1, confidence 0.1, head 1e-4; a null tolerance fails the gate closed (measure first, then declare). Also: worker README, recipe/valen/native.md (startup, examples, scope, gates, RFC-style contract table), gate accepts exactly the recorded flip.
…ch, guard removal (N5)
The hand-written {:e} derivation picked the upper digit at exact
shortest-decimal ties where CPython's repr picks the even one — the same
hazard tests/qwen3_5/json.rs pins for the shared json.rs, which is why it
routes through serde_json's correctly-rounded shortest digits. python_float
now delegates to omni_qwen3_5_native::json::float_repr (dependency already
present), keeping the non-finite guard; the tie bit-pattern oracle vectors
move with it into tests/valen/worker.rs. Recorded response bodies are
unchanged — none of the recorded cases hits a tie bit pattern (all 15 worker
contract tests pass byte-identical).
A panic inside the guarded decision left the engine mutex poisoned, and the lock's expect() then panicked on every later request — a permanent 500 for the worker's lifetime, where the reference's with-block lock just releases and the next request proceeds. The lock now recovers the poison (unwrap_or_else(into_inner)); the new Decide seam lets the CPU contract suite drive a flaky stub behind the same Mutex (first call panics, second answers), pinning panic -> 500 "inference failed" then a normal 200.
The parity gate accepted any choice flip in a case that carries a declared_flips entry, so one declared near-tie question would excuse a different question's flip in the same case. The declared set is now the qids the entry's note names (each with its reference margin — the same margins recorded per case), checked question by question in the gate loop; anything unrecognized declares nothing, so the gate fails closed. The matcher is pure fixture logic and pinned on CPU against the committed fixture (image declares exactly 'color'); the fixture bytes are unchanged.
…deviations CPython's scanner errors the moment it reaches the first non-finite constant, whatever follows; the native wire layer answers with the specific detail only when the whole substituted document is well-formed. Exact parity would mean porting the scanner's error ordering (constants at reach, the duplicate-key hook at object close, surrogates and out-of-range floats at the round-trip), so the deviation is recorded instead: a contract-table row, a limitations paragraph, and wire.rs tests that pin the native behavior with truthful rationales (the old comment claimed Python fails at the missing separator before the constant, which it does not). Also recorded: the slow-body timeout semantics (reference per-recv 15 s vs native one total 15 s budget) and integers beyond u64 in rendered text states silently re-serializing as doubles (inherited from the shared InferMatrixLab#143 JSON layer, left untouched).
README's Supported Models paragraph and table row, the docs/supported-models.md matrix, the mkdocs nav and recipe/README.md all described Valen as a Python reference worker only; the model README's intro still said native CUDA execution is not part of this slice while the same file documents the native/ path two sections down. All five now state the actual status: the native choice/noul/score worker on the shared Qwen3.5 backends, author-run on RTX 5060 Laptop (sm_120, WSL2), end-to-end parity gates with declared tolerances (probability/confidence 0.1, score 0.05, head probe 1e-4) and the one declared image near-tie flip attributed to vision-tower drift. native.md joins the mkdocs nav (strict build passes).
The branch's committed formatting predates the current stable rustfmt style (one-line ensure! collapsing, import ordering); CI runs cargo fmt --all --check on stable, and the base commit of this branch already fails it, as does every intermediate commit. One mechanical pass over the branch's files — no semantic change; all suites re-run green (valen CPU contracts, frontend byte-forwarding).
# Conflicts: # docs/supported-models.md
Rebuilt libqwen3_5_cuda.so for sm_120 from the synced source (36 upstream commits, ABI 8 -> 9: InferMatrixLab#123 multimodal CUDA Graph, InferMatrixLab#98 fixed-algorithm GEMM handle, InferMatrixLab#143 landing) and re-ran every Valen GPU gate serially on the RTX 5060 Laptop: vision tower, per-projection, end-to-end text hidden, and the full worker_gpu suite including the image decomposition diagnostic. Every layer reproduced its recorded figure verbatim, so no tolerance moved and nothing was re-declared — the eager numeric path this worker takes (default cs1_gemm_create, no CUDA Graph, no fixed handle) is unchanged by the sync. native.md now records the re-verification evidence block with the verbatim ABI 9 numbers and marks the original 2026-10-21 verdict as measured at ABI 8, re-verified at ABI 9.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Review: PR #158 — "Valen: serve natively through an eager Rust/CUDA worker"
Reviewer pass: first full review, posted 2026-10-24.
Head pin and merge-base
- PR state: OPEN, non-draft, MERGEABLE. Head repo/branch: MrDongsls
model/valen-native-worker, head66557122593f18293dadc2b6a97ccc0e32e351db(2026-10-24 01:34 +0800, "valen: drop internal work-package references from docs and comments"). - Base and merge-base:
f5ae5acf65e2d68150aef944aae00f4dffc0da27= current upstream main tip. Re-fetchedpr/158andmainover SSH at review time; head unchanged. - History: 57 commits total, 50 authored + 7 merges.
- CI at exact head (observed via
gh pr checks, runs on the InferMatrixLab fork): benchmarks pass (34s), build pass (13s), cuda-contract pass (9s), rust pass (5m55s), deploy skipped. Re-observed independently before this posting. - Note: the body's Test Plan says "head
66557122onspec/valen-native" — the commit hash matches the PR head, but the PR branch ismodel/valen-native-worker(informational; branch evidently renamed after the body was written).
Provenance / skill contract
- Skill:
system1-omni-review, read from/Users/hsliu/thinkflowlab/system1-omni/.agents/skills/system1-omni-review/SKILL.md(local working-tree copy, unpublished at local HEADca5f69b5c104; SHA-256 of bytes read58be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108). - The PR does not touch
.agents/(no review-instruction edits in the 69-file diff). References actually read:CONTRIBUTING.md(Large code changes; Adding a model),docs/architecture.md(ownership/admission sections), the review skill's referenced self-review artifact-hygiene rules as applied below; all key source/test/doc files of the PR (list under Inspected scope). - This review grants no approval authority; verdict below is advisory.
Inventory
69 files, +17479/−220. By CONTRIBUTING exclusion convention (reviewer-computed):
| Class | Lines |
|---|---|
Code (source, tests, scripts, .cu, crate manifests) |
+13750 / −177 (13,927) |
| Documentation (.md) | +935 / −43 |
| Generated/recorded fixtures (tests/valen/data, recipe/valen/fixtures, example JSONs) | +2772 / −0 |
| Lockfile (Cargo.lock) | +22 / −0 |
Areas: src/models/valen/native/ (new crate, 11 modules, 2709 lines), tests/valen/ (17 files), recipe/valen/ (17 files), shared Qwen3.5 native crate (vision/valen.rs 287 new, lora.rs +120/−3, model.rs +93/−0, vision/{mod,model}.rs +11/−2, Cargo.toml +1), one CUDA file (reference_norm.cu +2/−2), src/frontend/Cargo.toml (+5, dev-dep + test target only; zero transport-code changes), root README, docs/supported-models.md, mkdocs.yml, recipe/README.md, root Cargo.toml, Cargo.lock.
Carried content from open PRs. #146 ("serve noul and score through the reference worker", OPEN) and #147 ("export the trained stack", OPEN) are NOT merged and their heads are not ancestors of this branch, but this PR carries their work packages (body line 80 admits this; commit history confirms — the Oct 14–18 commits are the #146/#147 lines). At file level: +1478/−146 of this PR's diff sits on #146's 17 files, +3815/−74 on #147-only files, +12186/−0 on files unique to this PR. A patch-identity spot check shows a mix: src/models/valen/postprocess.py is byte-identical to #146's version; recipe/valen/test_compare_reference.py evolved further on this branch. The body commits to rebasing after #146/#147 land.
Per-contract findings
Findings (actionable)
P3 — stale shared-component doc for the changed CUDA entry points.
src/backends/cuda/qwen3_5/README.md:73 (PR head): "Norm additionally supports hidden size 2560." The PR's only CUDA change is the d==2048 whitelist in cs1_reference_rms_norm/cs1_reference_add_rms_norm (reference_norm.cu:28,32 — verified to be exactly the two gate lines; no ABI change, no new C symbols, runtime check only), and the third vision layout comment in vision/mod.rs correctly documents the 2B merger width — but the backend README's norm-width sentence was not updated. Trigger: a maintainer auditing the shared norm entry points' supported shapes from the component README misses the width this PR's whole image path depends on. Consequence: doc understates the contract for the new consumer; CONTRIBUTING's adding-a-model section requires updating affected shared-component documentation. One-line fix.
P3 — large-change accounting in the body is incomplete against CONTRIBUTING.
CONTRIBUTING requires the authored code-line count reported separately from total, with the stated exclusions, plus a per-area code count and validation summary. The body's only count is "Authored code exceeds 3,000 lines" (true — reviewer-computed code-class total is ~13.9k before removing carried content), with no exact authored number, no per-area counts, and no exclusion convention. This matters more than usual here because +17479 materially includes content belonging to two other OPEN PRs (~5.3k of insertions on #146/#147 files, some verbatim — see inventory), which the body discloses only in a trailing merge-order sentence without quantifying it. Trigger: reviewer computes attribution from scratch; if #146/#147 land with evolved content, the rebase cost is invisible in the body. Consequence: readiness/process gap, not correctness. Split rationale (one commit group per work package), component map (Purpose bullets + src/models/valen/native/README.md layout table) and per-area validation summaries are present; an explicit suggested review order is implied by the commit-group ordering but not stated for reviewers.
Informational (no action required)
src/models/valen/native/Cargo.toml:7: crate description still says "(choice)"; the worker now serves choice/noul/score. Stale one-word doc.- Live A/B run (commit 8fa3592): recorded only as prose in
recipe/valen/native.md; raw responses are at/tmp/valen_ab/{ref,native}.jsonon the authoring host — disclosed as scratch, not archived in-repo. The gate evidence itself (fixturechoice-parity-reference.json+ verdict tables + ABI-9 re-verification) is committed, and the A/B numbers match the fixture-gate figures, so this is an acceptable disclosure posture; noting the raw evidence is not reviewer-accessible. - Tokenizer identity: pinned structurally (5 token→ID assertions in
compile.rs::Processor::load, token IDs pinned by the recorded fixtures) but not hash-pinned; the checkpoint is fully pinned (revision81b9c639…+ SHA-256836622ef…in both the vision loader and fixtures), base by revision15852e8c…in the manifest pins. Consistent with how siblings treat the public base snapshot. - The fixture
tests/valen/data/head-readout.jsonhas a consumer (tests/valen/worker.rs:350) and format pin but no committed regeneration script (weights are a documented LCG pinned in the test itself);native_compiler.jsonand the parity/vision fixtures all have committed generators (generate_native_fixtures.py,dump_choice_parity.py,dump_text_reference.py,dump_vision_reference.py). No orphan artifacts found. - Poisoned-mutex recovery (commit 64e0178,
server.rs:62): recoveringinto_inner()matches the reference'swith-block lock-release semantics (failing request → 500, next request proceeds) and is pinned bya_poisoned_engine_mutex_serves_the_next_requestat the mutex level. Engine-level post-panic consistency is argued by the per-forward state design (per-prompt GDN reset, scratch rewritten per forward) but cannot be GPU-proven; a real mid-forward panic leaving the CUDA stream mid-work would serialize behind already-queued kernels. Design matches the reference; residual risk accepted. - Recorded wire deviations are honest and tested: hyper normalizes duplicate/equal Content-Length (411 unreachable), 405 vs the reference's 501 for unimplemented methods, slow-body total-15s-budget vs per-recv timeout, non-finite-constant detail ordering on doubly-invalid bodies,
>u64integer re-serialization inherited from the shared #143 JSON layer. All pinned inwire.rs/worker.rstests and the contract table (native.md). parse_bodyrecursion cutoff at depth 127 (serde limit) vs CPython's ~1000-frame limit: both surface the generic 400 body; only deep-nesting threshold differs, same wire surface.- Python
tests/valen/test_*.py(108 tests) are not wired into CI — consistent with the repository convention (CI runs only benchmarks/decider/cuda Python suites; these need torch). The Rust 70-case corpus and all CPU contract tests ARE in the CI rust job (cargo test --workspace,[[test]]entries insrc/models/valen/native/Cargo.toml:32-55). - No Cua-S1 26-letter choice restriction was imposed:
MAX_OPTIONS = 255(protocol.rs:17), 1–8 questions, noul/score finishing per the reference. Verified in corpus, finisher and engine.
Contract traces (source inspected)
- Model contract: serialization →
wire::parse_body(CPython-parity JSON decode, 70-case corpus pinned) →protocol::parse_request(limits, validation order, 422 bodies) →preprocess::prepare(records, chat-template-equivalent rendering, smart_resize with ties-to-even) →compile(branch suffixes, candidate/decision position capture, max_length 8192 with the reference's exact 422 wrapping, logical/compute accounting) → one-pass prefill per branch (text::forward_positions/forward_multimodal_positionswith the meshgrid mrope port) → CPU FP32 head (logit = (W_cand·h_cand)·(W_dec·h_dec)/16, FP32 throughout, finite-checked) →finish(FP64 softmax, Valen confidence formulas, CPythonreprfloats via the sharedfloat_repr,json.dumpsbyte semantics) → response. Custom-wording noul answerstype: noulvia the wire-kind dispatch. Shape checks at every boundary (head↔backbone hidden, vision rows↔layout, placeholder run↔grid). - Load contract: the five-file export is enforced, not just claimed —
valen_export.jsonmanifest (formatvalen-checkpoint-export/2) +config.json+lora.safetensors+head.safetensors+model.safetensors, each verified size+SHA-256 (text.rs:112-135,head.rs:53-63,vision/valen.rs::verify_export_files), the vision loader additionally rejects any unlisted artifact in the export directory, the LoRA profile is pinned (rank 32/alpha 64/scale 2/FP32/186 targets/372 tensors) and cross-checked against the configuration-derived inventory, and the vision manifest pins checkpoint revision+SHA-256 and base repo+revision.export.mddocuments "Five files, nothing else." - Readiness:
main.rsruns one real image and one real text warmup inference beforeTcpListener::bind—/health's ready is structural, matching the reference. - Wire contract: byte-parity machinery is real. The 70-case rejection corpus (57×422, 12×400, 1×413) was recorded from the pinned reference processing stack by
generate_native_fixtures.pyand is asserted entry-by-entry (status + exact detail text) in CI (tests/valen/native.rs:146). Recorded response bodies (fixtures/choice-parity-reference.json, formatvalen-choice-parity/2) pin exact usage accounting, legends, discrete answers, and per-float tolerances;nulltolerance fails closed (worker_gpu.rs::compare_case). Response-body byte identity holds for equal logits (recorded-vector finisher tests); end-to-end floats are gated, as declared. - Numerics: gates are measured-then-declared max-abs on deterministic fixtures, with the measurement figures and headroom stated in
native.md(text 8.7e-3/0.1, noul 2.9e-3, score 1.5e-2/0.05, head probe 4.5e-6/1e-4, tower 1.03125/1.25; text hidden 0.25/0.5). Not statistical (no p99) — appropriate for deterministic pinned fixtures. The image near-tie flip is disclosed with a real decomposition: pixel preprocessing bit-exact (0/393216), language+head on the reference's tower rows within 9.2e-3, tower drift 1.03125 closing a 0.108 reference margin; the flip is declared per-qid in the fixture (declared_flips.image, matched againstreference_decision_margins, fails closed on anything unrecognized, and a CPU test pins that onlycoloris declared). BF16 rounding: LoRAy = bf16(fp32(bf16(x@W)) + fp32 delta)per the export contract; device rows download as BF16 and upcast to FP32 on host before the CPU head (reduction-order difference vs the reference's device GEMMs disclosed inhead.rs). - CUDA/FFI: two-line
d==2048whitelist only;ABI_VERSIONuntouched; no new extern symbols;enable_valen_text_numericsselects the reference profile strictly before any forward (scratch/reference/fp32_gdn allNonechecked) and destroys the default GEMM handle it replaces; eager path — no graph capture anywhere in the Valen path (shared capture machinery untouched). - Frontend:
src/frontend/Cargo.tomladds only a dev-dependency and a[[test]]target; transport code is unchanged.tests/frontend/native_worker.rsruns the realomni_jevapp against the real worker router over sockets and pins direct-vs-forwarded byte identity for 200s and for the 400/422 rejection bodies, plus health forwarding. - Registration: root README,
docs/supported-models.md(two new rows),mkdocs.ymlnav (export + native),recipe/README.md,src/models/valen/README.md,src/models/valen/native/README.md, repository-level tests with explicit[[test]]paths — all present. All relative links in every touched doc resolve (0 dead anchors found by script). Tests live under roottests/per CONTRIBUTING;#[cfg(test)] #[path]wiring is used only for private-item tests, as allowed.
Commands and results (run by reviewer, on a worktree at head 66557122)
git fetch git@github.com:ThinkFlowLab/system1-omni.git pr/158 main(+pull/146/head) — pins above.git worktree add /tmp/pr-review-pass92/wt158 origin/pr/158— OK.PYTHONPATH=. python -m pytest tests/benchmarks -q(miniconda python) — 65 passed.mkdocs build --strict— OK, no warnings.PYTHONPATH=src python -m pytest tests/valen -qexcluding the torch/peft-dependenttest_export.pyandtest_export_parity.py— 47 passed in the weight-free subset (the full "108 passed 2 gated" needs torch; the 2 gated areVALEN_CKPT/CUDA-skipped).- Fixture inspections: corpus composition (70 rejections), fixture tolerances
{probability 0.1, confidence 0.1, score 0.05, head_probe 1e-4},declared_flips(image only,color), per-casereference_decision_margins. - Doc link checker over all touched markdown — 0 dead relative links.
gh pr checks 158— benchmarks/build/cuda-contract/rust pass at head.
Unverified (evidence gaps, author-reported)
- All GPU evidence (parity gates, ABI-9 re-verification, component gates, the live A/B) is author-reported on RTX 5060 Laptop sm_120 — disclosed as such in
native.mdwith committed fixture + verdict tables; not reproducible here (no GPU). A/B raw responses unarchived (see informational). - cua-s1 4B and JEMM GPU regressions not run by the author (no local weights) — disclosed in the body.
cargo fmt/clippy/test/buildnot run locally (no cargo on this host); covered by the observed green rust CI job at head.- Full Python suite (108) with torch, and the export/parity suites needing the pinned artifacts — author-reported.
- The pinned upstream artifacts themselves (Valen source
750bfcfb, checkpoint836622ef…, base15852e8c…) — identity pins verified in code, artifact bytes not verifiable here.
Verdict
COMMENTED (first full review of a large PR). Non-draft, template complete (four answers + necessity + self-review checkboxes + evidence section), no P0–P2 findings. Two P3s: the stale norm-width line in the shared CUDA backend README, and the incomplete authored-vs-total code accounting in the body (made material by the carried #146/#147 content). The engineering core — five-file hash-enforced load contract, recorded 70-case rejection corpus and byte-level finisher parity in CI, measured-then-declared GPU gates with a disclosed and decomposed near-tie flip, unchanged transport, and a genuinely two-line CUDA change — is verified in source. Sequencing note for maintainers: land #146 and #147 first as the body requests, then rebase this PR.
|
Both P3s addressed:
|
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Incremental re-review at head 30b3c7a66 (re-pinned via git rev-parse + gh pr view before posting; OPEN, non-draft, MERGEABLE). Prior review: COMMENTED at 665571225 (2026-10-24, first full review) with exactly two P3 findings.
Delta
One authored commit, 30b3c7a6 "valen: state the 2048 norm width in the CUDA backend README" — 1 file, +2/−1, doc-only. Own series otherwise untouched (prior head is the direct first-parent ancestor; no range-diff gate needed). The body was also updated (no commit required for that half).
Prior-finding status — BOTH P3s FIXED
- P3-1 (stale shared-doc) — FIXED. The commit rewrites the affected sentence to "Norm additionally supports hidden sizes 2560 and 2048" (
src/backends/cuda/qwen3_5/README.md:73-74), exactly matching the PR's two-lined==2048whitelist inreference_norm.cu:28,32. The shared backend README now states the contract the image path depends on. - P3-2 (unquantified large-change accounting) — FIXED in the body. The Diff Accounting section now states total +17479/−220 over 69 files; authored code +13750/−177 with the explicit exclusion convention (markdown, recorded fixtures, Cargo.lock); the carried open-PR content quantified (+4489/−103 on the 24 files shared with open #146/#147, split 15 byte-identical +2984/−49 / 9 evolved +1505/−54); and per-area authored counts. This matches — and in the carried-content split exceeds — what the prior review asked for.
Zero findings
No other content changed. The engineering core verified in the first review stands (hash-enforced five-file export, CI-wired 70-case rejection corpus, measured-then-declared gates, two-line CUDA change, no ABI impact).
PR state, template, CI
- Template answers, necessity decision and self-review checklist remain present (re-checked today). The merge-order note (land #146, then #147, then rebase this PR) is maintained in the body — a maintainer sequencing item, not a finding.
- CI at the exact head
30b3c7a66(observed): benchmarks/build/cuda-contract/rust all pass, deploy skipped (fork runs 40004431590/40004431641).
Unverified checks
- Unchanged from the first review: all GPU evidence (parity gates, ABI-9 re-verification, live A/B) is author-reported with committed fixture/verdict tables and disclosed archive posture; no GPU on this host. Rust suite covered by the green exact-head CI runs. Full torch-dependent Python suite author-reported.
Verdict
APPROVED — non-draft, zero findings (both prior P3s fixed by this delta), template complete, CI green at the exact head. The #146→#147→#158 merge-order sequencing remains with maintainers; when #146/#147 land, this PR rebases and any content evolution at the shared files would be a new review scope.
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent local re-review at 30b3c7a6668ca6e7494c6fd32cf06ef17484b496, base/merge-base f5ae5acf65e2d68150aef944aae00f4dffc0da27. Inventory: 70 files, +17481/-221; authored code including tests 13863 changed lines, excluding JSON/Markdown/manifests. The contributor's split rationale, component map and review order are present. The head is unchanged from the prior approval; the additional cases below reveal gaps outside its covered corpus. Head rechecked before posting.
Requesting changes for three independently reproduced P2 compatibility defects:
- Equal logits do not always produce the reference response bytes. For FP32 logits
[0, -1, ..., -9], the reference Python 3.12 softmax denominator is1.5819048852379487, versus native1.5819048852379485. P0 becomes0.6321492583604866versus0.6321492583604867, so the serialized choice answers differ. A 255-option case also differs. This tiny CPU reduction difference breaks the explicit byte-identical-for-equal-logits contract independently of model or CUDA drift. - Valid nested JSON is rejected as malformed. A request with a 127-level nested array state passes both reference
parse_bodyandparse_request, but native returns HTTP 400. Depth 126 passes both; depths 127, 128 and 200 pass Python and fail native. The outer serdeRawValueparser accepted these inputs in an isolated probe; the native custom depth guard rejects them. - A single-frame APNG is rejected as animated. A valid 32×32 PNG with
acTL.num_frames=1and matching frame-control/image data loads in Pillow withn_frames=1,is_animated=False, and passes the reference validator. Native returns HTTP 422 because the helper treats anyacTLchunk as multiple frames.
The nested-state request is generated without any model artifacts:
raw = (
'{"model":"valen-preview-0923","state":'
+ '[' * 127 + '0' + ']' * 127
+ ',"questions":{"q":{"type":"choice","instructions":"i","criteria":{"a":"A"}}}}'
).encode()The identical-logit check directly compared native finish::choice_answer with json.dumps(models.valen.postprocess.answer(...)) using the same candidate keys and exact integer-valued FP32 logits; no inference or estimated tolerance is involved.
Complete synthetic single-frame APNG request (reference wire accepts; native wire rejects with 422):
{"model":"valen-preview-0923","state":{"image":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAACGFjVEwAAAABAAAAALQt6aAAAAAaZmNUTAAAAAAAAAAgAAAAIAAAAAAAAAAAAAEAZAAA2G7FwAAAAClJREFUeJztzTEBAAAIwzDAv+dhAr5UQNNJ6rN5vQMAAAAAAAAAAIDDFsfxAz0uGANjAAAAAElFTkSuQmCC"},"questions":{"q":{"type":"choice","instructions":"i","criteria":{"a":"A"}}}}Agent-executed local validation on Linux x86_64, Rust/Cargo 1.98.1, Python 3.12.13, CUDA toolkit 13.0.88:
cargo test --locked -p omni-valen-native: 35 passed, 13 ignored.cargo test --locked -p omni-qwen3-5-native --lib: 33 passed, 13 ignored.cargo test --locked -p omni-jev --test native_worker --test frontend: 16 passed, including all four new forwarding tests over actual localhost sockets and a stub model boundary.cargo fmt --all --check: passed.cargo clippy --workspace --locked --all-targets --features omni-laya/serve -- -D warnings: passed.cargo test --workspace --locked --features omni-laya/serve: 309 passed including 2 doctests, 0 failed, 68 ignored. The targeted counts above overlap this run.cargo build --workspace --release --locked --features omni-laya/serve: passed.CUDA_VISIBLE_DEVICES='' PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=src python -m pytest tests/valen tests/frontend -q: 108 passed, 2 skipped. The skips are real-artifact export determinism and original/exported-model parity.python recipe/valen/test_compare_reference.py: passed; comparison-helper checks only.python -m compileall -q recipe/valen src/models/valen: passed, bytecode routed outside the checkout.python -m mkdocs build --strict --site-dir <external directory>: passed with pinned dependencies.CUDA_VISIBLE_DEVICES='' CUDA_HOME=/usr/local/cuda LIBRARY_PATH=/usr/local/cuda/lib64/stubs bash src/backends/cuda/qwen3_5/build.sh <fresh output directory> 89: passed, freshly compiling the changed 2048-wide norm dispatch; no GPU device execution.- Backend contract checker: 2 backends, 0 errors, one existing Laya ABI-declaration warning.
- Three extra regression cases failed on the reviewed implementation, confirming the three issues above.
git diff --check: passed.
Validation limits: the 68 ignored Rust cases and two skipped Python cases are not passes. No real checkpoint export/load, base-tokenizer reconstruction, model warmup, GPU kernel execution or native/reference model qualification was run. Author-run RTX 5060/ABI9 evidence and the declared image decision flip were inspected as author evidence, not independently reproduced. No performance claim is made. The documented transport, large-integer and JPEG deviations were treated as declared scope rather than reported again.
Same-head CI and Docs succeeded separately. Additional repository-layout note: CONTRIBUTING requires new test bodies/helpers under root tests/, while this PR adds inline tests in native wire.rs and a sizeable fake-CUDA helper under shared lora.rs. This preparation issue is separate from the three behavior findings.
Review provenance: agent-assisted review using .agents/skills/system1-omni-review/SKILL.md at trusted main a893f0cf55135623fb093f401d8ce4713330d801, SHA-256 ae2dfac116376ae2ec41f65122cc1bf5aea10751b8b907198a56154e5d67230e. Architecture, repository-contracts reference, CONTRIBUTING, artifact hygiene and affected CI/component contracts were read. Commands below were executed by the agent; this does not attest personal human execution.
| let exponentials: Vec<f64> = scaled.iter().map(|&v| (v - maximum).exp()).collect(); | ||
| let total: f64 = exponentials.iter().sum(); | ||
| Ok(exponentials.iter().map(|&e| e / total).collect()) |
There was a problem hiding this comment.
[P2] Match the pinned Python floating-point reduction semantics. The reference runs on Python 3.12, so its sum(exponentials) is not the ordinary left fold used here. With the exact same FP32 logits [0, -1, ..., -9], reference P0 is 0.6321492583604866 and native P0 is 0.6321492583604867; the denominators are 1.5819048852379487 and 1.5819048852379485. A 255-option case also differs. This is independent of CUDA/model drift and violates the documented byte-identical-for-equal-logits contract. Reproduce the reference reduction semantics for softmax and score reductions and add longer nonuniform same-logit oracle cases.
| if depth >= 127 { | ||
| // Python raises RecursionError, which parse_body reports as malformed. | ||
| return Err(de::Error::custom("recursion limit exceeded")); |
There was a problem hiding this comment.
[P2] Preserve the reference's accepted JSON nesting range. A body with a 127-level nested array in state is within the byte/text limits and passes both Python reference parsing and request validation, but this custom guard makes native answer 400 as malformed JSON. I reproduced depth 126 passing both and depths 127/128/200 passing Python but failing native; the outer serde RawValue parser accepted all of these in an isolated probe. Python does not raise its recursion error at depth 127, contrary to the comment. Align the supported nesting boundary or establish an explicit documented API limit with boundary tests rather than claiming identical admission.
| if kind == b"acTL" { | ||
| return true; |
There was a problem hiding this comment.
[P2] Inspect the APNG frame count before rejecting it. acTL may declare exactly one frame. A valid local 32×32 APNG with num_frames=1 loads in Pillow with n_frames=1 and is_animated=False and passes the Python reference validator, while this unconditional return makes native reject it with 422. Inspect the declared frame count and add the accepted single-frame case beside the two-frame rejection. A complete synthetic reproducer request is included in the companion review evidence; it requires no model to exercise the wire validators.
The requested-changes review at this head reproduced three native-wire divergences from the Python reference; each fix is pinned by recorded reference oracles under root tests/: - finish: CPython 3.12's sum() over floats is the Neumaier compensated accumulation, not the naive left fold the finisher ran, so equal logits rounded differently (FP32 [0, -1, ..., -9]: denominator 1.5819048852379487 vs ...485, P0 ...66 vs ...67). All four reference reductions (softmax total, score distance, uniform MAD, expected level) now go through one python_sum with those semantics, pinned by the review's 10-option case, a 255-option descent, an f32-domain softmax divergence and a score whose expected level only matches under the compensated sum(i*p). - wire: the JSON depth guard rejected nesting at 127 levels on a wrong RecursionError claim — CPython's own budget is under 1000 levels on 3.11 and ~1490 on 3.12+ (Py_C_RECURSION_LIMIT 1500), both surfacing as the same malformed-JSON 400. The guard is now the documented 2048-level API limit above every stock build's acceptance, with 127/2047/2048 boundary tests (release profile pinned to a 2 MiB serving stack) and the deviation recorded in recipe/valen/native.md. - protocol: any acTL chunk was treated as animation, rejecting valid single-frame APNGs Pillow loads with n_frames=1. The frame count now follows Pillow's semantics: the declared count decides, 0 and >2^31 fall back to the default image, a second acTL drops the animation, the header scan stops at the pixel data, a missing fcTL counts the default image separately, and a truncated acTL is the reference's invalid-image-data 422. The review's exact request plus a 10-case matrix are pinned on both sides (native.rs, test_protocol.py). Checks: cargo test --workspace --locked --features omni-laya/serve (314 passed, 0 failed, 68 ignored), fmt/clippy -D warnings clean, release build clean, release depth boundary on a 2 MiB stack, pytest tests/valen tests/frontend (109 passed, 2 skipped), mkdocs --strict.
…tests The review's repository-layout note: CONTRIBUTING keeps all test bodies, helpers and fixtures in the repository-level tests/ tree, with only the #[cfg(test)] and module-path wiring in src/. Two additions of this PR grew beyond that wiring: - the wire layer's inline suite (7 tests, including the private nonfinite_spans coverage) moved to tests/valen/wire.rs, wired from src/models/valen/native/src/wire.rs exactly like the qwen3_5 suites; - the shared fake-CUDA helper (FAKE_CUDA_LOCK + the ~90-line fake_cuda_library compiler) moved from lora.rs to tests/qwen3_5/fake_cuda.rs, re-exported through crate::lora so both wired suites (lora.rs, lora_valen.rs) keep their call sites; the include_str anchors now point from the tests tree. No behavior or test content changes: the moved bodies are verbatim. Checks: cargo test -p omni-valen-native --lib (7 passed) + cargo test -p omni-qwen3-5-native --lib (33 passed, 13 ignored, fake boundary included) + workspace suite (314 passed, 0 failed, 68 ignored) + fmt/clippy -D warnings clean.
|
Thanks for the thorough re-review — all three reproduced defects are fixed at
The separate layout note is also addressed in Validation at |
|
fix conflicts |
|
this PR is so large |
|
The A concrete way to address both the conflicts and review size is to resolve the stack in order: #146 → #147 → #158. The fresh comments on #146/#147 describe the comparison-gate and import-pin issues to settle upstream. Once those prerequisites are integrated, refresh this branch against the resulting main so its remaining native-worker delta can be reviewed on its own. Please provide an updated component map and review order for protocol/CPU reconstruction, native model/readout, shared Qwen/CUDA changes, and vision handling. Record the residual authored size separately from inherited dependency changes; if an independently reviewable component can be split without breaking the contract, identify it, otherwise retain a concrete coupling explanation. On the resolved head, rerun the three exact reproductions, combined native/Python/frontend tests and CI. Keep real-artifact skips, GPU qualification, and the declared image decision flip visible beside the existing author evidence. This follow-up at Agent-assisted readiness follow-up using the repository review skill pinned at trusted main |
…worker # Conflicts: # src/models/qwen3_5/native/src/model.rs # src/models/qwen3_5/native/src/vision/mod.rs
Component map at
|
| Slice | Files | Lines |
|---|---|---|
| Total net vs current main | 72 | +17,949 / −221 |
| Identical to #146→#147, drops on rebase | 11 | +2,725 / −46 |
| Authored native-worker residual | 61 | +15,224 / −175 |
Authored residual by area: src +3,315, tests +7,337, recipe +4,541, root wiring +31. Of that, +2,739 lines are committed JSON fixtures holding recorded oracles.
| Component | Files | Lines | Contents |
|---|---|---|---|
| A. Protocol and CPU reconstruction | 12 | +4,964/−3 | crate skeleton: protocol.rs, compile.rs, wire.rs, finish.rs, preprocess.rs; tests: native.rs, native_tokenization.rs, wire.rs, test_protocol.py, data/native_compiler.json |
| B. Shared Qwen/CUDA and vision | 17 | +2,558/−74 | B1 + B2 |
| B1. Text hooks and LoRA loading | 12 | +1,767/−72 | qwen3_5: model.rs, lora.rs, reference_norm.cu; valen: text.rs; tests: lora_valen.rs, valen_text_gpu.rs, fake_cuda.rs, tests/valen/text.rs, text_reference.rs |
| B2. Vision handling | 5 | +791/−2 | qwen3_5 vision: valen.rs, model.rs, mod.rs; tests: valen_vision.rs, valen_manifest.rs |
| C. Native model, readout, worker, evidence | 26 | +7,662/−93 | src: engine.rs, server.rs, head.rs, main.rs, READMEs, src/frontend/Cargo.toml; tests: worker.rs, worker_gpu.rs, native_worker.rs, test_export_parity.py, data/head-readout.json; recipe: fixture generators, dump and verify tools, native.md, JSON fixtures |
| Wiring and docs | 6 | +40/−5 | Cargo.{toml,lock}, README.md, docs/supported-models.md, mkdocs.yml, recipe/README.md |
Review order A → B → C. A uses only existing qwen3_5 APIs and runs in CI without weights. B's hooks are exercised by their own tests, so nothing lands unused. C cannot split from B: engine.rs is the sole caller of forward_multimodal_positions and drives the checkpoint loaders from B2. Once #146 and #147 merge, a rebase removes the carried 11 files above; the stack is then A, B, C cut from this branch with #158 retargeted to C.
Validation at 0499dd5d: cargo test 405 passed, 0 failed, 82 ignored; clippy --all-targets -D warnings and fmt clean; pytest 109 passed, 2 skipped. The three 6cd57154 parity fixes are present after the merge.
Required human contributor answers
Serve Valen natively behind the existing Rust frontend.
Numerical consistency.
The native worker is the baseline any later acceleration must be checked against. This PR establishes it by reusing the landed #143 profile and gating every numeric layer measured-then-declared.
Users who want to serve Valen without a Python serving stack.
After answering the four questions: is a PR needed at all to achieve this outcome?
Yes, native serving has to land in
mainfor system1-omni to serve Valen without Python.For a bug fix: link actual observed failure evidence.
N/A, not a bugfix.
Purpose
Second slice of #84: a native eager Valen worker serving
choice,noulandscoreover text and one inline image, behind the reference worker's wire contract, with no Python in the serving path. Correctness milestone only, no performance claim.omni-valen-nativecrate: wire and compiler equivalence, eager per-branch execution, CPU FP32 head, finishers and the HTTP worker. The only CUDA change is a two-lined==2048whitelist inreference_norm.cu, no ABI impact.Contract table, tolerances, deviations and parity records:
recipe/valen/native.md.Test Plan
System1-Omni Version / Commit: head
30b3c7a6onmodel/valen-native-worker, base and merge basef5ae5acf= current upstream main, latest main merged.Diff accounting, merge-base
f5ae5acf→ head: total +17479/−220 over 69 files; authored code +13750/−177, excluding markdown docs (+935/−43), recorded/generated fixtures (tests/valen/data/,recipe/valen/fixtures/,recipe/valen/example-*.json; +2772) andCargo.lock(+22). Of the authored code, +4489/−103 sits on the 24 files shared with the open #146/#147 — 15 byte-identical to their heads (+2984/−49), 9 evolved further on this branch (+1505/−54) — and +9261/−74 on the 45 files unique to this PR. Per area (authored code):src/models/valen/native/+2763; sharedsrc/models/qwen3_5/additions +512/−5;src/models/valen/Python outsidenative/+198/−44;tests/valen/+5219/−5;tests/qwen3_5/+tests/frontend/+925/−66;recipe/valen/scripts +4123/−54; CUDAreference_norm.cu+2/−2;src/frontend/Cargo.toml+5; root manifest and nav +3/−1.The split is one commit group per work package — vision loading on the shared executor, wire and compiler equivalence, the unmerged text LoRA path, the worker, the question-type finishers — each independently tested and merged before the next. Suggested review order follows that grouping: shared-crate vision additions first, then the native crate bottom-up (wire → compile → text/vision execution → finishers → worker), then the frontend forwarding test and the registration docs.
GPU on RTX 5060 Laptop sm_120: rebuild the shared CUDA library and run the four env-gated tests serially, commands in
recipe/valen/native.md.Test Result
66557122: clippy-Dclean, workspace 300 passed 0 failed, frontend 12 plus native_worker 4, tests/valen 108 passed 2 gated, mkdocs strict ok. The post-review head30b3c7a6adds one backend-README line only, checked against thereference_norm.cuwidth gate.Demo / evidence
recipe/valen/native.mdholds launch commands, examples, the contract table, the tolerance table and both parity records. Fixtures regenerate via the pinned dump scripts.Self-review
Merge order: #146 first, then #147, then this PR; it carries their content until they land and will be rebased onto the result.