[CLM 1/2] Checkpoint and typed decision scoring - #28
Conversation
xiaoyu-xyz
changed the title
7bacfa2 to
2422066
Compare
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Reviewed commit 2422066a707dc5a39f08be0087d486f868a62c42.
[P2] Respect layernorm=false when loading hidden blocks. src/models/clm/src/weights.rs:138 unconditionally loads norms.0.weight/bias for depth=3. Config and head_tensors accept layernorm=false and require these tensors to be absent, so a valid no-LayerNorm checkpoint passes inventory validation then fails to load. Reproduced with a tiny synthetic safetensors checkpoint. Load the norm tensors only when cfg.layernorm is true. Default tests: 1 passed, 2 checkpoint-dependent tests ignored.
|
Fixed in b918ca2. The regression test writes three checkpoints — no LayerNorm, LayerNorm with the tensors, and one that declares LayerNorm and omits them — and the third is still rejected by the inventory. It fails on the previous revision. |
Levius-Fubuki
left a comment
Levius-Fubuki
left a comment
There was a problem hiding this comment.
Reviewed b918ca2f5ce516d42fb6ffb08f49a429f58c2baa, including the full config/weights/scoring change and the prior LayerNorm finding.
The original LayerNorm finding is fixed: the synthetic cases accept a checkpoint without norms when layernorm=false, accept a complete normed checkpoint, and reject a checkpoint claiming norms but missing their tensors.
Fresh Linux/Rust 1.98.1 validation: formatting, strict all-target Clippy, workspace tests and release build passed. The workspace default run passed 15 tests (two CLM external-data tests skipped there). All four CLM tests subsequently passed with the published head checkpoint: 16 tensor FP32 hashes and the five NumPy head-oracle decisions matched. Checkpoint revision e939398d4556fcd9400c76fa8c5a513202f42b0a was verified against official SHA-256 b2b4a8c9c2d39263eff78a351eb909a342ce9b3bf21a3f07c1d1bf15f1c4eda5.
Changes requested for the metadata compatibility defect below; the score-label tie mismatch is a lower-priority additional finding. Both were reproduced with small CPU probes and compared with official CLM source pinned to bb42c6c5bf914fd449bed2f6ca65be80602cb1f7. No real Qwen/vLLM or GPU execution was performed. Existing merge conflicts also need resolution before merging.
| ); | ||
| let cfg = metadata.get("cfg").context("metadata has no cfg")?; | ||
| let mut head: HeadConfig = | ||
| serde_json::from_str(cfg).with_context(|| format!("parse cfg {cfg}"))?; |
There was a problem hiding this comment.
[P2] Apply top-level geometry fallback before deserializing required cfg fields
The following fallback promises to load exports whose cfg omits hidden_size/projection_dim, but both are required fields of HeadConfig, so deserialization fails before that fallback is reached. A metadata fixture with top-level hidden_size="4", projection_dim="2", format="clm-heads", finite logit_scale, and cfg={"width":3,"depth":3,"activation":"gelu","layernorm":false,"residual":false} returns missing field hidden_size. The supplied exporter can produce this layout: it preserves cfg while obtaining dimensions from the checkpoint's top-level entries. Resolve the dimensions into the config before deserialization (or deserialize optional fields and then validate the resolved dimensions), and add a regression for both absent cfg fields.
| Answer::Noul { noul } => if *noul >= 0.5 { "true" } else { "false" }.to_string(), | ||
| Answer::Score { probabilities, .. } => probabilities | ||
| .iter() | ||
| .max_by(|a, b| a.1.partial_cmp(&b.1).unwrap_or(std::cmp::Ordering::Equal)) |
There was a problem hiding this comment.
[P3] Keep the first maximum when score labels tie
Iterator::max_by returns the last equal maximum, whereas the upstream schema.label_of keeps the first key. For score probabilities [("0", 0.5), ("1", 0.5)], the current public Answer::label() returns "1" and the pinned upstream returns "0". Equal probabilities can arise from duplicate score-level descriptions, so this changes the discrete label despite identical probabilities. Preserve first-maximum tie breaking and add a tie case. This affects the label helper; it does not change the serialized expected score.
b918ca2 to
e1a1211
Compare
|
Fixed in e1a1211, and the stack is rebased on
Reverting either fix turns its test red. Strict Clippy, workspace tests and the release build pass on |
hsliuustc0106
left a comment
hsliuustc0106
left a comment
There was a problem hiding this comment.
Independent local review — CLM 1/2
Verdict: changes requested — one convention finding; the model work itself is verified and impressive.
[Medium] Test bodies live in src/models/clm/tests/ — a crate-local tests/ directory under src/. CONTRIBUTING prohibited this at this PR's own base ("Do not add inline test bodies or crate-local tests/ directories under src/… the test implementation stays under root tests/"), and current main follows it (tests/laya, tests/cua_s1, tests/open_jev, tests/runtime, tests/qwen3_5). The tests do run via cargo autodiscovery, so this is placement, not breakage — please move them to repo-root tests/clm/ with [[test]] registration or the #[path] convention.
Verified: the exp(logit_scale) cap (scale() = exp(4.6132).min(100) = 100, not 100.82) is correctly implemented and documented as binding; weights loading validates format/tensor names/shapes/dtypes; scoring preserves first-maximum tie semantics (matching Python max), name-based noul lookup, clamped top-minus-mean confidence, bounded temperature. Locally at head e1a1211: cargo test -p omni-clm → 4 passed / 2 ignored (checkpoint-gated, disclosed); clippy -D warnings clean. The exported-checkpoint gates (hash oracle, five decisions vs head_oracle.py) remain author-reported — no checkpoint on this machine.
Local reviewer report per the repo review skill; reflects head e1a1211 only.
The same rule `src/models/clm/tests/` broke in ThinkFlowLab#28 applies here, and to the inline test in `embedding.rs`: CONTRIBUTING keeps test bodies in the repository-level `tests/` tree, and for a private item only the `#[cfg(test)]` and the module path may stay in `src/`. `HttpEncoder::body` is private, so that is the `#[path]` form, with the body in `tests/clm/embedding.rs`. Found in review.
|
Moved to the repository-level tree in #29 does the same for its own tests, and for the one test body that was inline in
|
CLM is the second model tracked in ThinkFlowLab#9, and it decides differently in a way LAYA does not cover: the engine does not compute embeddings. A frozen Qwen3-8B encoder runs as its own process, and the engine owns everything after it. This is the half that can be checked without a GPU -- the checkpoint, the head geometry, and the decision arithmetic. A CLM checkpoint is a torch.save dict, so recipe/clm/native/export_weights.py converts it first: tensors to safetensors with the head name as a prefix, and cfg, hidden_size, projection_dim and logit_scale into the metadata. The published checkpoint is 75 MB and holds the two heads, not the 8B encoder; the export is 16 tensors and 18.9 M parameters. - config reads the head geometry from the metadata and checks it. - weights lists the expected inventory from that geometry, checks shapes before reading, and loads FP32. - scoring projects a state and each candidate, L2-normalises both, scores them by cosine under exp(logit_scale), softmaxes across the question's candidates, and assembles choice, noul and score. The three types differ only after the distribution exists, so they share one path. confidence is the top probability minus the mean of the rest, clamped, and 1.0 for a single candidate, which is the definition src/clm/schema.py uses. exp(logit_scale) is capped at 100, as the reference caps it in heads.py. The published logit_scale is 4.6132, whose exponential is 100.82, so the cap binds: without it every probability is about 0.8 % off. Checks. head_oracle.py is an independent NumPy implementation of the same arithmetic, and tests/checkpoint.rs checks 16 tensor conversion hashes against the export oracle and five decisions against head_oracle.py. Default cargo test needs no checkpoint. fmt, clippy -D warnings.
`head_tensors` lists `norms.N.*` only when `cfg.layernorm` is set, so a `layernorm: false` checkpoint is complete without them — but `load_head` read them unconditionally. A valid checkpoint passed the inventory check and then failed to load. The new test writes both shapes plus one that declares LayerNorm and omits the tensors, and checks the loader accepts the first two and rejects the third. Found in review.
…labels - `Config::from_metadata` applied the top-level `hidden_size`/`projection_dim` fallback *after* deserializing `cfg`, but both are required fields, so a `cfg` that omits them failed to parse and the fallback never ran. The geometry is now filled in before deserialization, and a `cfg` that omits it loads while one that omits it with no top-level copy is still an error. - `Answer::label` used `max_by`, which keeps the *last* of several equal values. `schema.label_of` is `max(p, key=p.__getitem__)`, which keeps the first, so a tie in a score distribution named a different level. Found in review.
CONTRIBUTING puts every test body in the repository-level `tests/` tree and asks for explicit `[[test]]` registration, which is what `src/models/laya` does. `src/models/clm/tests/` was a crate-local directory under `src/`, which that rule prohibits; the tests ran either way, so this is placement rather than breakage. Found in review.
|
Rechecked |
f4286a9 to
9650e2a
Compare
The same rule `src/models/clm/tests/` broke in ThinkFlowLab#28 applies here, and to the inline test in `embedding.rs`: CONTRIBUTING keeps test bodies in the repository-level `tests/` tree, and for a private item only the `#[cfg(test)]` and the module path may stay in `src/`. `HttpEncoder::body` is private, so that is the `#[path]` form, with the body in `tests/clm/embedding.rs`. Found in review.
|
Rebased onto The only conflict was the workspace Checks on the rebased head, locally:
The CI job for The checkpoint-dependent tests you could not rerun, with The pre-rebase head was reproduces that comparison; it prints the first commit with only that line changed and the |
3 participants
CLM is the second model tracked in #9, and it decides differently in a way LAYA does not cover: the engine does not compute embeddings. A frozen Qwen3-8B encoder runs as its own process, and the engine owns everything after it. This is the half that can be checked without a GPU — the checkpoint, the head geometry, and the decision arithmetic.
Core code: 495 lines (
config76,weights154,scoring259,lib6), within the 500-line budget. Tests and the export/oracle scripts are separate.Two PRs, this one then #29:
clm-run, and the comparison against CLMThe checkpoint is converted first
A CLM checkpoint is a
torch.savedict, so it is a pickle and no non-Python reader can open it.recipe/clm/native/export_weights.pywrites the tensors to safetensors with the head name as a prefix, and keepscfg,hidden_size,projection_dimandlogit_scalein the metadata. The publishedCLM_v0.1-8B.ptis 75 MB and holds the two heads, not the 8B encoder; the export is 16 tensors and 18.9 M parameters.The decision
softmax(exp(logit_scale) * cos(state_head(s), action_head(c)) / temperature)over a question's candidates. Both heads areinp → [LayerNorm →] hidden → outwith GELU, and both projections are L2-normalised before the dot product.The three types differ only after the distribution exists, so they share one path:
choiceis the argmax key,noulis thetrueentry of a two-candidate distribution, andscoreis the expected level index.confidenceis the top probability minus the mean of the rest, clamped, and1.0for a single candidate — the definitionsrc/clm/schema.pyuses.The scale cap
exp(logit_scale)is capped at 100, asheads.pycaps it. The publishedlogit_scaleis 4.6132, whose exponential is 100.82, so the cap binds — without it every probability is about 0.8 % off, and more where candidates are close.I had this wrong until a real encoder disagreed with me. The CPU-side oracle could not catch it:
head_oracle.pywas written from the same reading of the format and made the same mistake, so both sides agreed and the test was green. It tookcompare_with_reference.py(in #29) comparing against CLM's own heads over real Qwen3-8B embeddings.Checks
With the checkpoint exported:
6 tests: 16 tensor conversion hashes against the export oracle, five decisions against
head_oracle.py— an independent NumPy implementation of the same arithmetic — on hash-derived embeddings, the loader against three synthetic checkpoints that differ only inlayernorm, acfgthat carries its geometry only at the top level, and a tied score label. Defaultcargo testneeds no checkpoint (4 passed, 2 ignored).The CI job's commands pass locally:
cargo fmt --all --check,cargo clippy --workspace --locked --all-targets -- -D warnings,cargo test --workspace --locked,cargo build --workspace --release --locked.