Repository navigation
Conversation
added 6 commits
October 14, 2026 22:19
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.
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. |
added 16 commits
October 15, 2026 22:14
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 (spec 02, E2): 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 E1's 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 spec-02 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 spec-02 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 (local ticket E4). 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.
MrDongsls
force-pushed
the
spec/valen-export
branch
from
October 15, 2026 14:16
9f55f55 to
cfce7ba
Compare
MrDongsls
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f55f55088
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+935
to
+936
| payload = torch.load( | ||
| checkpoint_path, map_location="cpu", weights_only=False, mmap=True |
There was a problem hiding this comment.
When --checkpoint points to a corrupted or attacker-supplied directory, its sibling training_metadata.json can simply declare the corrupted file's hash, so the preceding check is not a trust anchor. This call then uses weights_only=False, allowing pickle reducers to execute before any inventory validation; unlike ValenExecutor, the exporter never compares the file to the fixed CHECKPOINT_SHA256 in src/models/valen/engine.py. Verify the hard-coded pinned digest before this load (or avoid unrestricted pickle loading).
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Addressed in 5bad3c1: the exporter now enforces the worker's pinned CHECKPOINT_SHA256 (src/models/valen/engine.py) before torch.load, so training_metadata.json remains a consistency record only. Regression test test_export_rejects_checkpoint_below_the_worker_pin proves the rejection fires before any unpickling.
Comment on lines
+278
to
+280
| passed = decision_exact and legend_exact and prob_ok and tokens_exact | ||
| if scalar_answers: | ||
| passed = passed and max_answer_delta <= tolerance |
There was a problem hiding this comment.
For the newly added noul/score cases, a regression in the worker's confidence formula can still make this harness exit successfully: max_conf_delta is computed and printed, but it is never included in passed. For example, matching probabilities, score, legend, and tokens with worker confidence 0.99 versus reference confidence 0.4 reports a 5.9e-01 confidence delta and still returns True, so the advertised reference comparison would not catch an incorrect response field.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Addressed by 507c448 (the same fix as #146), which this branch includes since the rebase — the reviewed 9f55f55 was the pre-rebase tree: max_conf_delta is gated by its own confidence tolerance verdict line and included in passed.
| from frontend.valen import _verify_source_revision | ||
|
|
||
| source = Path(args.valen_source).resolve() | ||
| _verify_source_revision(source) |
There was a problem hiding this comment.
When the pinned Valen checkout has local modifications, this check still passes because _verify_source_revision in src/frontend/valen.py only runs git rev-parse HEAD. Both the original and exported subprocesses then import the same modified compiler/model code, so a shared local change (for example, a changed Valen.forward) can make the two stacks agree and fill a passing parity record even though neither used the claimed pinned source; require a clean tracked working tree (and account for relevant untracked files) before producing evidence.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Addressed in 5bad3c1 and extended in b6490ae: both evidence harnesses (verify_export_parity.py and compare_reference.py) now reject a dirty pinned checkout — tracked modifications and untracked .py files — before any record is produced, and reference.md's pre-flight checks match. Regression tests cover both rejection paths.
added 4 commits
October 15, 2026 22:50
hsliuustc0106
left a comment
hsliuustc0106
left a comment
Contributor
There was a problem hiding this comment.
Independent review — PR #147 "valen: export the trained stack to an unmerged safetensors artifact set"
Reviewer: independent read-only review (system1-omni-review skill). No repo modifications, nothing posted to GitHub. Review body drafted 2026-10-18.
Provenance
- Repository: ThinkFlowLab/system1-omni (ID 1386454853).
- Base:
main, livee66ff34996760a84ad56690f0ae4cf434e669e65(moved past the PR's recorded basef2d3d3289…; the PR's stated upstream base037c44b9…is the merge-base — main moved by the unrelated #111/#116 merges after branching). PR GitHub base field:f2d3d32897c1fe190fecf853a82a9456d3791462. - Head:
369202d60da14dd061936d03963671f78531a9da(spec/valen-export), re-fetched live before reporting; GitHub headRefOid matches. - Skill load record:
.agents/skills/system1-omni-review/SKILL.mdread from the local working tree, SHA-25658be3bc60806c4c5af7038a0c6a80fadb94f3360d714556a3e965c8903dde108(matches the pinned value). References actually read:docs/architecture.md(no export-specific clauses; export is recipe-level per the jemm/open_jev precedent),.agents/skills/system1-omni-review/references/repository-contracts.md,CONTRIBUTING.mdat the head,.agents/skills/self-review/SKILL.mdat the head (incl. Committed artifact hygiene), the Valen recipe and docs at the head. - Inspected scope: full
git diff origin/main...origin/pr/147(24 files, +4924/−144), deep read of the export-side files; the tree was extracted to/tmp/pr-review-2026-10-15/head(working tree untouched).
PR structure and stacking
- The branch stacks on open PR #146 (
model/valen-noul-score, head75efa400…): commits 1–6 of this branch are #146's line. This is disclosed first thing in the PR body ("merge #146 first … drops the noul/score commits automatically once #146 merges"). - Full diff vs main: 24 files, +4924/−144. Export-only delta (
origin/pr/146...origin/pr/147): 12 files, +3672/−16 —export_checkpoint.py(1063),verify_export_parity.py(567),export.md(212),tests/valen/test_export.py(1099),tests/valen/test_export_parity.py(389),recipe/valen/test_export_reference.py(158), plus reference.md/README/compare_reference/mkdocs/recipe-README touches. - Authored vs total: all 25 commits authored by the PR author (Mr.Dong / MrDongsls); authored == total. Above the 3,000-line threshold, CONTRIBUTING's self-review requirements apply: full self-review checklist checked by the contributor, split rationale present (stacked PR, serving split from export), validation across affected areas present. The body has no explicit component map/review order — minor readiness note only, since the layout is recipe/tests/docs and the Purpose section enumerates the artifacts.
- Merge order matters: until #146 merges, this PR's GitHub diff shows both lines. The export review below is based on the export-only delta plus the head tree.
Verdict
No blocking findings. The export side is unusually carefully engineered: pins enforced before the pickle is loaded, every artifact byte digest-pinned in the manifest, fail-closed validation of partial/tamper/stale states, deterministic streaming writer, and a parity gate whose executable form matches its declared semantics (tested bit-equal against installed peft). Findings below are low/informational.
Findings (by severity)
- Low — atomicity is exception-driven cleanup, not staging+rename.
run_export(recipe/valen/export_checkpoint.py:330-384) writes in place into a fresh-or-empty--outand removes partial artifacts on any raised exception (tested:test_run_export_failure_leaves_no_partial_output,test_run_export_failure_cleans_preexisting_empty_output). A hard-killed process (SIGKILL/OOM/power loss) can leave partial files, and the not-empty guard then blocks retries until an operator removes the directory. Documented behavior; no corruption risk becausevalidate_exportrejects any non-exact directory. Temp-dir + atomic rename would close the gap. - Low —
compare_casezip truncation nit (recipe/valen/verify_export_parity.py:95-98): the original/exported probability lists are zipped without an equal-length check; a malformed stage payload with a different list length would silently truncate the max-delta scan. Candidate order is checked and both sides are produced by the same tool, so this is a robustness nit in the evidence harness, not a gate criterion. - Low —
run_export(base_config_path=None)writes a set its ownvalidate_exportrejects (allowed set requiresconfig.json,export_checkpoint.py:690-709). Not reachable viaexport()/CLI/tests (all pass the config); API-surface consistency only. - Info — the manifest does not record the exporting code's revision.
sourcesrecords checkpoint SHA-256 + revision, per-shard base digests + revision + repo, and torch/safetensors/peft versions; the format stringvalen-checkpoint-export/2is the contract anchor and the PR pins the code — but a repo commit field insourceswould make the manifest self-contained for future audits. Note the asymmetry: the parity harness pins the Valen source revision and demands a clean checkout with no stray.py(_verify_clean_checkout), which is stronger than the #140 fixture-pinning precedent. - Info — the artifact set deliberately excludes tokenizer/processor files ("Five files, nothing else"). "Nothing else is needed to reconstruct the model" is accurate; reconstructing the serving stack still needs the pinned base for the processor/tokenizer. A one-line note in export.md could preempt confusion. Optional.
- Info — supported-models row not extended for the export. The Valen row (
docs/supported-models.md:18) is updated fornoul/scorebut does not mention the export recipe; defensible, since the row's native-worker column is still "Not supported" and the export is reachable viarecipe/README.mdand the mkdocs nav. Revisit when the native consumer lands.
Per-focus review
1. Export identity/pinning
- Pinned bytes:
manifest.filescarries size + SHA-256 for every non-manifest artifact:model.safetensors(~4.5 GiB),lora.safetensors,head.safetensors,config.json— four of four. The manifest itself (valen_export.json) cannot self-hash; itsparityslot is filled only by the tool, formatting-preserving, refusing silent overwrite (fill_parity_slot, tested idempotent + refuse-without-force). - Source pins, fail-closed, before any write or unpickle:
checkpoint.ptSHA-256 vs the worker's hard-codedCHECKPOINT_SHA256(src/models/valen/engine.py:22, value matches the PR body and the env-gated test assertion836622ef…f42f5a012);training_metadata.jsonmust exist and agree; base shard digest must be declared by bothvalen_manifest.jsonand the checkpoint's embeddedbase_manifestwith exact per-shard mapping, and revisions must agree;stagemust bevision_top. Missing pins raise; they are never skipped (tests cover each missing/mismatched pin individually). This is the review-bot P1 from an earlier round, fixed and regression-tested ("the exporter must enforce the worker's pinned digest before the pickle is ever loaded"). - Rejection of stale/partial exports:
validate_exportenforces exactly-five-files directory contents (extra files or directories fail), format match, streamed file digests + sizes, full two-way tensor inventory vs safetensors headers with shape/dtype, downcast list vs per-tensor flags, head-config consistency, and the adapter inventory (one A + one B per target at the declared rank, each mapping onto a listed backbone tensor).verify_export_parityadditionally re-hashes the checkpoint and base shard against the manifest'ssourcesbefore loading (a swapped base fails:test_verify_sources_rejects_swapped_base_shard_bytes). - #140 lesson (pin executables/config too): the parity harness pins the Valen source revision (
frontend.valen._verify_source_revision) and rejects dirty checkouts or untracked.pyfiles before evidence is produced. The export itself pinsconfig.jsonbytes. No tokenizer is part of the artifact set (finding 5).
2. Unmerged-ness semantics
- The set preserves original base weights + separate FP32 adapters + separate FP32 head:
model.safetensors= base verbatim (578 pass-through, of which 36 FP32linear_attn.A_log/norm.weightdowncast to BF16) with trained vision-top blocks 20–23 and merger substituted as plain BF16 replacements (full tensors, not merges);lora.safetensors= 372 FP32 adapter tensors verbatim under PEFT key names;head.safetensors= 2 FP32 matrices. Exporting a merged path was deliberately dropped (commit "export the language LoRA separately and drop the merged path"), with the merge-vs-unmerged math documented (export.md: "Merging … changes the computed outputs relative to the trained artifact's unmerged semantics, which is why the export does not do it"). - BF16 rounding locations are explicit and machine-checked:
LORA_APPLICATIONis recorded verbatim in the manifest (y = bf16(fp32(bf16(x @ W.t())) + scaling·((x.float() @ A.t()) @ B.t()))— base GEMM BF16, delta FP32, exactly one final rounding, peft 0.18.1 unmerged semantics with FP32 adapters) andunmerged_lora_outputis its executable form, tested bit-exact against installed peft plus a hand-computed rounding case and a zero-B reduction case. Downcasts are claimed lossless against the reference's own BF16 runtime and gated: any FP32 base tensor outside the two declared families is an error. Head math stays FP32 with formulas recorded. - Loader-side contract: the manifest's
consumptionblock + export.md's "Consumption" section specify the full load (from_pretrained backbone, peft re-wrap at manifest rank/alpha loadinglora.safetensorsby key with adapters kept FP32, head rebuild). The native consumer is future work and is disclosed (PR body: "the native worker is separate upcoming work"; export.md: "serving from it is the consumer's side"; src/models/valen/README.md: "Consuming that export does not change this worker"). The documented consumption path is exercised for real only in the author's GPU parity run (see unverified checks).
3. Determinism / reproducibility
- Sorted keys everywhere (classification, tensor plans, manifest JSON
sort_keys=True), streaming writer with fixed per-file ordering, single-threaded CPU math, no timestamps, no absolute paths — tested byte-identical across runs and across different input directory names (test_run_export_is_deterministic_and_path_independent, run locally). The real-artifact double-export determinism gate is env-gated (VALEN_CKPT/VALEN_BASE) — author-run, with per-file digests recorded in the PR body. - Source revisions pinned (checkpoint SHA-256 + HF revision when derivable, base per-shard digests + revision + repo, tool versions in
sources.environment). Exporting code pinned by the PR head + FORMAT version (finding 4 for the residual gap).
4. Docs
recipe/valen/export.mdis complete and matches the code: usage + expected console output, artifact table with exact counts (578/372/48/6/2 = 1006 manifest tensor records), manifest schema, LoRA application semantics, downcast rationale, head formulas, consumption, pins/licenses (Apache-2.0 per the #115 record, with a re-check instruction), parity-evidence placement, determinism + regression commands with recorded counts ("105 tests passed, 2 artifact-gated skips" — matches my run).reference.mdgains a full "Export parity" section with verbatim tool output (RTX 5060 Laptop, sm_120, bit-level deltas 0.0, recorded 2026-10-15) and the reproduce command; the honest "~3e-8 worker-vs-reference spread" between the parity run's original stack and the earlier serving records is disclosed. Anchors referenced by export.md exist.src/models/valen/README.mdadds a "Checkpoint export" section;recipe/README.md, mkdocs nav, README matrix and supported-models updated for noul/score. Template readiness: complete (see below).
5. Tests
- Weight-free coverage is strong and CPU-only: unmerged-vs-peft bit equality, hand-computed BF16 rounding, downcast RNE, key classification incl. adversarial cases (block 24 swap, unpaired adapters, unknown keys, head bias), tiny-fixture end-to-end export contract, path-independent byte determinism, failure cleanup, per-pin rejection tests,
validate_exporttamper matrix (flipped bytes, dropped tensor entries, downcast/head/lora drift, extra files/dirs), streaming-digest enforcement (monkeypatchedPath.read_bytesrefusal), parity verdict matrix, parity-slot overwrite policy, source-pin cross-checks, clean-checkout gate. - Fixture provenance: all synthetic, seeded, generated in-test; the two example request JSONs are maintained fixtures consumed by reference.md curl commands and
compare_reference.py— no committed run output anywhere in the 24 files (all text source/docs/fixtures, no binaries).
6. Artifact hygiene (all 24 files)
16 modified + 8 added files; all are source, tests, docs, or maintained request fixtures. No exported weights, no parity dumps, no logs, no site/ build output committed. Authored counts = total (all commits by the PR author).
Commands run (this host, macOS arm64, no GPU, no Rust)
All on the extracted head tree /tmp/pr-review-2026-10-15/head with a throwaway venv (Python 3.12.8; torch 2.14.1 CPU, safetensors 0.8.0, peft 0.21.2, pytest 9.1.1, pillow — not the author's pinned GPU matrix, torch 2.11.0+cu128 / peft 0.18.1):
git fetch origin main --force→ live maine66ff34996760a84ad56690f0ae4cf434e669e65;git fetch origin pull/147/head:refs/remotes/origin/pr/147 --force→ head369202d60da14dd061936d03963671f78531a9da(matches GitHub).shasum -a 256 .agents/skills/system1-omni-review/SKILL.md→58be3bc6…dde108(pin verified).PYTHONPATH=src python -m pytest tests/valen -q→ 105 passed, 2 skipped in 4.99s — matches the author's claim and export.md verbatim. Skips are exactly the artifact-gated gates (test_export.py:1054needs VALEN_CKPT/VALEN_BASE;test_export_parity.py:348needs VALEN_* + CUDA).- Export-only subset
pytest tests/valen/test_export.py tests/valen/test_export_parity.py -q→ 58 passed, 2 skipped. python recipe/valen/test_export_reference.py→export_reference tests: ok (2 skipped)(full 39-test battery in-process + skip lines for artifact-gated phases, exit 0).python recipe/valen/test_compare_reference.py→compare_reference tests: ok(incl. the new score-confidence-fallback case).python -m compileall -q recipe/valen tests/valen→ pass.python -m mkdocs build --strict→ exit 0 (done because the Docs workflow evidence was initially not visible to me; it turned out green at head — see below).gh pr checks 147/ check-runs API at head369202d→ rust: pass (2m0s), build: pass, benchmarks: pass, cuda-contract: pass, deploy: skipped, Docs: pass (run 2026-10-15T15:21:57Z at the head SHA). Note: no GPU/model validation runs in CI; the rust/docs results are consistent with a Python/docs-only change.
Template state (post-#138)
Complete and traceable: the four required human answers are present and specific; the PR-necessity decision answered ("Yes, without this every consumer converts the pickle on its own"); bug-fix evidence marked N/A (not a bug fix); Purpose/Test Plan/Test Result/Demo-evidence filled with pinned head SHA (369202d), environment, per-file manifest digests, and verbatim parity output; all four self-review checkboxes checked by the contributor; the stacking on #146 and the not-yet-implemented native consumer are both disclosed. NON-draft, so template completeness was required — it is met.
Material unverified checks
- GPU parity evidence is author-run, not independently reproduced (this host has no GPU, no pinned weights): bit-level 0.0 deltas on the text/image fixtures, RTX 5060 Laptop (sm_120), torch 2.11.0+cu128, peft 0.18.1. It is recorded verbatim in
reference.md(with reproduce command) and the manifestparityslot; the env-gated tests (test_real_artifact_export_is_deterministic,test_real_artifact_export_parity_gate) are the reproducible form and were skipped here. Per the skill: missing GPU/weights is an evidence gap on my side, not a PASS by me. - The documented consumption path executed against real weights (
from_pretrained+ peft re-wrap key-name mapping + head rebuild) ran only in the author's parity run; there is no weight-free test ofload_exported_stack(inherent — it needs transformers and a real model). The PEFT key-name round-trip (file keysbackbone.language_model.base_model.model.…vs wrapped-model parameter names through the top-levelValencontainer) reads consistent and is covered end-to-end by the author's run, but I could not execute it. - Real-artifact byte determinism — author-run (digests in the PR body); env-gated test skipped locally.
- transformers-internals claim in export.md ("no keep_in_fp32_modules for this architecture; upcasts with .float() at use") — not independently verified against the pinned transformers version here; it is corroborated indirectly by the bit-level parity result.
- Rust checks: not run locally (no Rust toolchain on this host); CI rust job green at head. My local Python versions differ from the author's pinned matrix (noted above); the peft bit-equality test passing on peft 0.21.2 is a small independent corroboration of the declared semantics, not a substitute for the pinned-env GPU run.
Bottom line
The export half is ready on the evidence presented, modulo the disclosed future consumer: pins enforced before load, every artifact byte pinned, fail-closed validation, deterministic output, semantics anchored and tested. Merge-order dependency on #146 is real (the GitHub diff collapses after #146 merges). The low findings are polish; none block on correctness.
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).
Collaborator
Author
|
Thanks for the thorough review. All three Low findings are addressed in
On the Info items:
Regression coverage: three new tests ( |
hsliuustc0106
approved these changes
Oct 18, 2026
hsliuustc0106
left a comment
hsliuustc0106
left a comment
Contributor
There was a problem hiding this comment.
Incremental re-check at new head d1236c661b: one new commit since my review at 369202d60d — "valen: stage the export atomically and gate compare_case on list lengths" (5 files, +95/−24); the 25 reviewed commits are patch-identical (git range-diff, named refs, cat-file verified).
All three Low findings from my review are fixed, verified in the diff and by execution:
- Atomicity — fixed:
run_exportnow stages the artifact set in a sibling.export-partialdirectory and moves it into place with a single rename, soout_dironly ever holds a complete export; a killed run leaves at most the staging directory, which the next run with the same target clears. - compare_case zip — fixed: a case whose probability lists don't cover every candidate now fails the case instead of silently truncating the delta scan.
- base_config_path=None — fixed:
run_exportnow raises instead of producing a set its ownvalidate_exportrejects.
Also carried: the tokenizer/processor exclusion got its documented one-liner in export.md, and the suite count was updated. Locally run in a fresh CPU-only venv at this head: 108 passed, 2 skipped (exactly the doc's updated count; the 2 skips are the artifact-gated determinism gates). mkdocs build --strict passes; fork CI (rust/build/cuda-contract/benchmarks) green at this exact head.
No actionable findings remain. APPROVED. Unverified checks (unchanged from my first review): the RTX 5060 bit-level parity and real-artifact double-export determinism remain author-run (no GPU/weights on this host — the 2 skips are those gates); the documented consumption path against real weights exists only in the author's run; Rust suite covered by exact-head CI. Provenance: review skill at main@71857e0 (SHA-256 58be3bc6…dde108); remote head re-pinned immediately before posting.
4 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacks on #146 (
model/valen-noul-score) — merge #146 first; this PR's diff shows both changes until then, and drops the noul/score commits automatically once #146 merges.Required human contributor answers
Make the pinned Valen checkpoint loadable outside the training-framework pickle, with proof it computes identically.
Numerical parity between the exported artifact set and the original checkpoint through the same kernels.
Every transformation is recorded in a JSON manifest, the LoRA application semantics are anchored, and the parity gate and determinism check are reproducible.
Valen users who need a loadable artifact set with provenance.
After answering the four questions: is a PR needed at all to achieve this outcome?
Yes, without this every consumer converts the pickle on its own.
For a bug fix: link actual observed failure evidence.
N/A.
Purpose
One deterministic command turns the pinned
checkpoint.ptplus its Qwen3.5-2B base into a five-file safetensors artifact set:model.safetensors— the base verbatim, except the trained vision-top blocks 20–23 and merger as plain BF16 replacements, and the 36 FP32 tensors downcast to BF16 (lossless against the reference's BF16 runtime).lora.safetensors— the 372 FP32 LoRA tensors verbatim under their PEFT key names, served unmerged; the manifest'slorablock records the rank 32 / alpha 64 / scale 2 configuration and the application semantics a consumer implements (BF16 base GEMM plus the FP32 delta on the activations, one final rounding — peft 0.18.1Linear.forwardboundaries).head.safetensors— the decision head's two FP32 (256×2048) projections, no bias.config.json— the base config copied verbatim forfrom_pretrained.valen_export.json— the manifest (valen-checkpoint-export/2): per-tensor name/shape/dtype/component/provenance, source revisions and SHA-256s, the downcast list, the head configuration, the consumption path, and the parity record.The export enforces the artifact pins before writing, is deterministic and byte-identical, and validates with streaming digests.
Worker behavior is unchanged; no Rust changes.
Test Plan
From the repository root with the pinned artifacts (
recipe/valen/reference.mddocuments the setup):Test Result
System1-Omni Version / Commit: head
369202donspec/valen-export(based onmodel/valen-noul-score2192c27, upstream base main037c44b); the CPU checks were re-run from this committed tree, and the determinism gate (double export, byte-identical) was re-run against the pinned artifacts at this head. The GPU parity record is the run pinned inrecipe/valen/reference.mdand the exported manifest's parity slot.Validated on WSL2 x86_64, RTX 5060 Laptop GPU (sm_120), BF16, PyTorch 2.11.0+cu128, peft 0.18.1, pinned Valen source / Preview checkpoint / Qwen3.5-2B base:
pytest tests/valen: 105 passed, 2 artifact-gated skips.compileall,test_export_reference.py(with and withoutVALEN_EXPORT, and withoutPYTHONPATH=src, as documented): pass.filesdigests —model.safetensors4,548,216,808 B0313882b…,lora.safetensors134,613,824 B4f6d9590…,head.safetensorsf365a09e…,config.jsoned1c1723….recipe/valen/reference.mdand the manifest'sparityslot (tool-filled; re-runs refuse to overwrite a different record).Not run / unverified: the Rust suite (no Rust changed; CI covers it), any other hardware, and consumption by an actual native Rust loader — the consumption path is documented in the manifest and exercised here through
from_pretrainedplus the peft application; the native worker is separate upcoming work.Demo / evidence
Nonvisual tooling change: video N/A. The evidence is the parity record — verbatim
verify_export_parity.pyoutput at the PR head, also recorded inrecipe/valen/reference.mdwith reproduction commands and the pinned revisions:Usage and artifact documentation:
recipe/valen/export.md; the manifest schema is documented in the tool's docstrings.Self-review