Repository navigation
Conversation
There was a problem hiding this comment.
Verdict: MUST_FIX: invalid values reach completed sets, code is duplicated, and a comment names an absent consumer.
Must fix
1. Reject non-finite logits at the argmax owner
tools/ref/q35mtp_ref.cpp:134 Kind B: non-finite logits silently yield token ids. With x[0] = NaN, every comparison is false, so trunk target_next becomes 0 (366) and is published (513). Untapped NaNs are also ignored by std::max (435), leaving a plausible delta in the manifest (508).
Cost: protect these records before gates depend on them. Replace first_argmax:
static int first_argmax(const float * x, int n) {
int best = 0;
for (int i = 0; i < n; ++i) {
if (!std::isfinite(x[i])) {
fprintf(stderr, "q35mtp_ref: non-finite logit at id %d\n", i);
std::exit(1);
}
if (x[i] > x[best]) best = i;
}
return best;
}2. Reject malformed numeric options
tools/ref/q35mtp_ref.cpp:274 -c bad becomes zero, then the prompt length (303). -ngl bad becomes zero GPU layers (308). With otherwise valid inputs, inference can finish and publish a completed set instead of rejecting the option.
Cost: reject bad options before reference sets depend on this recipe. Replace all four atoi calls with integer:
static int integer(const char * s) {
char * end = nullptr;
errno = 0;
const long v = strtol(s, &end, 10);
if (end == s || *end || errno || v < INT_MIN || v > INT_MAX) {
fprintf(stderr, "q35mtp_ref: invalid integer %s\n", s);
std::exit(2);
}
return (int) v;
}3. Share the existing oracle input and output contracts
tools/ref/q35mtp_ref.cpp:57 copies tools/ref/hidden_ref.cpp:70. write_bytes (84) also repeats write_raw (97) there: exact writes and close failures share a contract.
Cost: prevent policy drift (R14). Include this tools/ref/ref_common.h from both harnesses; delete the four local functions:
#pragma once
#include "llama.h"
#include <cerrno>
#include <climits>
#include <cstdint>
#include <cstdio>
#include <cstdlib>
#include <fstream>
#include <string>
#include <vector>
inline bool read_token_file(const std::string & path, long long count,
std::vector<llama_token> & ids) {
std::ifstream in(path);
if (!in) { fprintf(stderr, "ref: cannot read %s\n", path.c_str()); return false; }
std::string line;
while ((long long) ids.size() < count && std::getline(in, line)) {
if (!line.empty() && line.back() == '\r') line.pop_back();
char * end = nullptr;
errno = 0;
const long v = strtol(line.c_str(), &end, 10);
if (end == line.c_str() || *end || errno || v < 0 || v > INT32_MAX) {
fprintf(stderr, "ref: invalid token id in %s\n", path.c_str());
return false;
}
ids.push_back((llama_token) v);
}
if ((long long) ids.size() != count) {
fprintf(stderr, "ref: short prompt in %s\n", path.c_str());
return false;
}
return true;
}
inline bool write_bytes(const std::string & path, const void * data, size_t bytes) {
FILE * f = fopen(path.c_str(), "wb");
if (!f) { fprintf(stderr, "ref: cannot write %s\n", path.c_str()); return false; }
const bool ok = fwrite(data, 1, bytes, f) == bytes;
if (fclose(f) != 0 || !ok) {
fprintf(stderr, "ref: short write on %s\n", path.c_str());
return false;
}
return true;
}Replace the MTP ids and hidden-state write calls:
if (!read_token_file(tokens_file, n, ids)) return 2;
if (!write_bytes(out + "/result_norm.0.f32", rows.data(), rows.size() * sizeof(float))) return 1;Hash the header too. In both build scripts:
SRC_SHA=$(cat "$SRC" "$HERE/tools/ref/ref_common.h" | sha256sum | cut -d' ' -f1)In hidden.sh and dump-q35mtp.sh, respectively:
want_src=$(cat "$HERE/tools/ref/hidden_ref.cpp" "$HERE/tools/ref/ref_common.h" | sha256sum | cut -d' ' -f1)
want_src=$(cat tools/ref/q35mtp_ref.cpp tools/ref/ref_common.h | sha256sum | cut -d' ' -f1)4. Reuse the scratch-directory owner
crates/refset/src/arch/qwen35moe/mtp/tests.rs:30 duplicates crates/refset/src/clefvis/testkit.rs:9, including cleanup and directory creation.
Cost: keep one cleanup policy (R14). Delete dir; replace the path import with:
use crate::clefvis::testkit::scratch as dir;
use std::path::Path;5. Remove the nonexistent gate consumer
tools/ref/dump-q35mtp.sh:6 names gate_qwen35moe_mtp clause (m) as a consumer. That gate is absent.
Cost: correct coverage before users rely on it. Replace line 6:
# mtp-qwen35moe-mainline checks.Checked
Confirmed both harness I/O pairs and the scratch owner. Both deduplication fixes reduce elements.
Traced invalid values to manifest publication. Tests inspected, not run. Inline replacements are valid on added lines. No existing gate pins changed. No escape-catalog match or hot path touched.
Notes
GPU evidence lacks the required nvidia-smi output.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #243
| // The first index of the largest value: the argmax a tie gives to the lower id. | ||
| static int first_argmax(const float * x, int n) { | ||
| int best = 0; | ||
| for (int i = 1; i < n; ++i) { |
There was a problem hiding this comment.
Reject non-finite logits before argmax turns them into ordinary token ids.
| for (int i = 1; i < n; ++i) { | |
| for (int i = 0; i < n; ++i) { | |
| if (!std::isfinite(x[i])) { fprintf(stderr, "q35mtp_ref: non-finite logit at id %d\n", i); std::exit(1); } |
| # the prompt's first MTP_PROMPT prose ids, fed teacher-forced (step i: token ids[i+1], position i, the trunk's h_nextn | ||
| # row i), every step run in order and the graph nodes MTP_TAPS dumped at the steps MTP_ROWS, by q35mtp_ref | ||
| # (tools/ref/q35mtp_ref.cpp), into $BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64 — the set the refset family | ||
| # mtp-qwen35moe-mainline checks and gate_qwen35moe_mtp's clause (m) reads. |
There was a problem hiding this comment.
This tree has no gate_qwen35moe_mtp consumer.
| # mtp-qwen35moe-mainline checks and gate_qwen35moe_mtp's clause (m) reads. | |
| # mtp-qwen35moe-mainline checks. |
There was a problem hiding this comment.
Verdict: MUST_FIX: Incomplete sets pass validation, and concurrent dump writers can corrupt the oracle.
Must fix
1. Validate the completion counts
crates/refset/src/mtptaps.rs:211 discards both trailer fields. In the test fixture, delete both ffn_moe_topk manifest rows but retain # complete\t4\t0. The two remaining taps pass check_taps: it derives the expected names from the surviving rows at :378. check_family and Family::check_set then succeed, so refset-check reports PASS for an incomplete set.
Reject this before the set is used as an oracle. Replace this match arm:
"# complete" => {
if f.len() != 3 || f[2] != "0"
|| number(f[1], &at)? as usize != set.taps.len()
|| text.lines().skip(i + 1).any(|l| !l.is_empty())
{
return Err(RefError::malformed(&at, "invalid completion trailer"));
}
set.complete = true;
}Add a refusal case that removes one tap name at every selected row without changing the trailer.
2. Serialize writers to the destination
tools/ref/dump-q35mtp.sh:58 gives CPU and GPU runs the same staging directory. Start --cpu alongside a GPU dump to the same destination: CPU mode bypasses the card runner at :25, and both runs delete and write SET.staging. One can erase the other's files; interleaved writes can be installed under one run's manifest at :77–78.
Protect the reference data before concurrent runs corrupt it. Replace lines 57–60 with a destination lock held through EXIT cleanup and installation:
SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}
mkdir -p "$(dirname "$SET")"
exec {set_lock}> "$SET.lock"
flock -n "$set_lock" || {
echo "dump-q35mtp: $SET already has a writer" >&2
exit 75
}
STAGE=$SET.staging
rm -rf "$STAGE"3. Share the missing-column rule
crates/refset/src/mtptaps.rs:577 repeats crates/refset/src/vision.rs:256 and crates/refset/src/visref.rs:358: a data row requires its column header (R14).
Consolidating now removes two helper definitions and gives this validation rule one owner. Move the existing helper into columns.rs, replacing all three local definitions:
pub(crate) fn need<'a>(
cols: &'a Option<Columns>,
what: &str,
at: &dyn Display,
) -> Result<&'a Columns, RefError> {
cols.as_ref().ok_or_else(|| {
RefError::malformed(at, format!("a {what} row before its column line"))
})
}Add this import in vision.rs and mtptaps.rs:
use crate::columns::need;Add this import in visref.rs:
use crate::columns::need as need_cols;Replace the three mtptaps.rs calls:
let c = need(&trunk_cols, "trunk", &at)?;
let r = need(&step_cols, "step", &at)?.row(line, &at)?;
let r = need(&tap_cols, "tap", &at)?.row(line, &at)?;Checked
Duplicates: confirmed the three missing-column helpers; consolidation uses the existing columns module and reduces definitions and lines.
Complexity: diagnostics reports new TapSet::read at 46 and Family::check_set at 24→26; neither is a separate finding.
Silent failure: followed the ignored completion count through check_taps, check_family, Family::check_set, and the PASS output at refset_check.rs:47–57.
Tests, by inspection: the malformed-set cases do not remove an entire tap name or alter the completion count.
Gates: no existing bands or pins moved. Escape catalog: no exception changes these classifications.
Notes
Changed shell files pass bash -n. Rust and GPU tests were not run.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
There was a problem hiding this comment.
Verdict: MUST_FIX: malformed inputs still become valid token IDs or prompt lengths.
Must fix
1. Reject embedded NULs in token-file lines
tools/ref/ref_common.h:47 accepts \0bad as token 0 and 7\0bad as token 7. strtol stops at the embedded NUL, so checking *end does not check the whole string. Both harnesses feed the accepted ID to inference and can write a complete reference set.
Fixing the shared reader now prevents its callers from publishing more sets from malformed input. Replace the condition with:
if (end == line.c_str() || end != line.c_str() + line.size() || errno != 0 ||
v < 0 || v > INT32_MAX) {Keep regression cases for both NUL-containing lines.
2. Close the malformed-option bug on the sibling dumpers
tools/ref/dump_mtp.cpp:618, tools/ref/dump_draft.cpp:529 and tools/ref/dump_ref.cpp:567 still parse --tokens-count 64x as 64. With valid remaining arguments, they run and publish a set instead of refusing the option. This is the same numeric-option defect this PR fixes in hidden_ref.
The new parser makes all three remaining repairs mechanical now. Include ref_common.h in each file and replace the assignments, respectively:
tokens_count = ref_integer("dump_mtp", "--tokens-count", argv[++i]);
tokens_count = ref_integer("dump_draft", "--tokens-count", argv[++i]);
tokens_count = std::max(0, ref_integer("dump_ref", "--tokens-count", argv[++i]));Hash the header alongside the source in build-dump.sh / dump.sh and build-dump-mtp.sh / dump-mtp.sh, as the changed mainline runners do.
Non-blocking
⚠ Retain the malformed-option checks as an automated regression: tools/ref/ref_common.h:19 is exercised by the commands in the PR body, but the new Rust tests do not call it. Add those cases to the host checks. Replacing ref_integer with atoi must turn the check red.
Checked
Duplicates: 63 new definitions checked. All three dryer pairs share structure across different domains. The remaining ID-reader copies predate this change.
Complexity: TapSet::read is new at 51; its separate stages are headers, data rows and trailer validation. Family::check_set rises 24→26 through one additional identity arm.
Silent failure: six parser sites followed to consumers. The two surviving failure paths are above.
Tests (static mutation check): reversing byte order at crates/refset/src/mtptaps.rs:580, dropping the build check at crates/refset/src/mtptaps.rs:305, and dropping trailer validation at crates/refset/src/mtptaps.rs:214 each breaks a corresponding new test assertion.
Gates: no existing pins or thresholds moved. Destination locking, step positions, row coverage and trailer counts checked.
Escape catalog: no C1–C4 pattern matched. No engine hot path changed.
Notes
Validation was static; runtime tests and GPU gates were not run. The PR body supplies card-run evidence.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| char * end = nullptr; | ||
| errno = 0; | ||
| const long v = strtol(line.c_str(), &end, 10); | ||
| if (line.empty() || *end != '\0' || errno != 0 || v < 0 || v > INT32_MAX) { |
There was a problem hiding this comment.
An embedded NUL hides unparsed bytes; require a parsed integer spanning the whole line.
| if (line.empty() || *end != '\0' || errno != 0 || v < 0 || v > INT32_MAX) { | |
| if (end == line.c_str() || end != line.c_str() + line.size() || errno != 0 || | |
| v < 0 || v > INT32_MAX) { |
There was a problem hiding this comment.
Verdict: MUST_FIX: malformed token-list input still reaches decode, and exact-write validation has multiple owners.
Must fix
1. Apply the numeric parser to the sibling token-list option
tools/ref/dump_ref.cpp:533 --tokens bad,42 becomes tokens 0,42; 64x becomes 64. With the required model and write options, these valid IDs reach llama_decode at line 779 and produce an oracle for a different prompt. This PR fixes the same malformed-number behavior in --tokens-count and token files; this sibling remains reachable.
Closing this sibling now prevents another reference-set repair. Replace lines 531–534:
const std::string list(argv[++i]);
for (size_t at = 0;;) {
const size_t end = list.find(',', at);
const std::string field = list.substr(at, end == std::string::npos ? end : end - at);
int id = 0;
if (!parse_i32("dump_ref", "--tokens", field.c_str(), 0, id)) return 2;
tokens.push_back((llama_token) id);
if (end == std::string::npos) break;
at = end + 1;
}2. Finish consolidating the exact-write contract
tools/ref/ref_io.h:88 repeats the open/write/close checks in tools/ref/dump_ref.cpp:203, tools/ref/dump_mtp.cpp:229 and tools/ref/dump_draft.cpp:175. All three now include this header. R14 requires one owner for these checks.
Consolidating them now removes three implementations before more callers attach. Add this overload to the header and delete those three local definitions; their callers stay unchanged:
inline bool write_raw(const std::string & path, const void * data, size_t size, size_t count) {
return write_bytes(path, data, size * count);
}Non-blocking
⚠ Check a step whose argmaxes differ: crates/refset/src/arch/qwen35moe/mtp/tests.rs:138 checks row 2, where both argmaxes are 8. Changing mtp_argmax to read target_next at crates/refset/src/mtptaps.rs:581 passes every new assertion.
Using the unequal row already in the fixture prevents this regression without another test. Replace the two lines:
let s = set.step(1).unwrap_or_else(|e| panic!("{e}"));
assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (12, 1, 6, 7));Checked
Duplicates: the shared writer and all three local copies confirmed; the fix removes three definitions and adds one overload.
Complexity: diagnostics report new TapSet::read (41), new LogitsSet::read (24), and Family::check_set 24→28.
Silent failure: the token-list fallback reaches decode; the shared parser rejects malformed fields.
Tests: static mutation audit confirms that the existing unequal row catches the field-mapping error missed by the current assertion.
Gates: Rust/C++ suites and card gates were not rerun during validation.
Escape catalog: no C1–C4 pattern underlies the retained findings. These paths load reference sets or run reference tools.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| let s = set.step(2).unwrap_or_else(|e| panic!("{e}")); | ||
| assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (13, 2, 8, 8)); |
There was a problem hiding this comment.
Use the existing row with unequal argmaxes so a swapped field mapping fails this assertion.
| let s = set.step(2).unwrap_or_else(|e| panic!("{e}")); | |
| assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (13, 2, 8, 8)); | |
| let s = set.step(1).unwrap_or_else(|e| panic!("{e}")); | |
| assert_eq!((s.token, s.pos, s.mtp_argmax, s.target_next), (12, 1, 6, 7)); |
There was a problem hiding this comment.
Verdict: MUST_FIX: malformed manifests pass the family check, and the new reader adds a third i32 decoder.
Must fix
1. Reject repeated entries in # rows (kind A)
crates/refset/src/mtptaps.rs:395: in the existing three-step fixture, set # rows to 0 0 2, add both tap names at row 5 using the existing files, and set the trailer count to 6. All checks pass: six taps equal three declared rows times two names. refset-check reports PASS although step 5 does not exist.
Reject this before consumers depend on the accepted tap rows. Insert after the empty-set guard in check_taps:
if self.rows.iter().collect::<HashSet<_>>().len() != self.rows.len() {
return Err(RefError::malformed(
at.display(),
"duplicate # rows entry",
));
}Add this case to the existing malformed-set test. Removing the new guard must make it fail:
at(
"repeated rows hide taps at a nonexistent step",
&[
"tap\tmtp_eh_proj\t5\tf32\t2\tmtp_eh_proj.r0.f32",
"tap\tffn_moe_topk\t5\ti32\t2\tffn_moe_topk.r0.i32",
],
&|d| swap(d, "# rows\t0\t2", "# rows\t0\t0\t2"),
"duplicate # rows entry",
);2. Reuse one little-endian i32 decoder (kind B, R14)
crates/refset/src/mtptaps.rs:504 repeats crates/refset/src/dsref.rs:573 and crates/refset/src/mtpref.rs:637. All three own the same four-byte little-endian conversion.
Consolidating now replaces two local helper definitions with one common definition and removes the new inline copy. Move the existing decoder to columns.rs:
pub(crate) fn i32s_of(b: &[u8]) -> Vec<i32> {
b.as_chunks::<4>()
.0
.iter()
.map(|c| i32::from_le_bytes(*c))
.collect()
}Delete both local i32s_of definitions and import use crate::columns::i32s_of; in their modules. Replace the inline return in tap_i32 with:
Ok(crate::columns::i32s_of(&bytes))Checked
Duplicates: all three i32 decoders confirmed. The three dryer pairs share shape, not knowledge.
Complexity: TapSet::read is new at 41; headers and data-row dispatch are its separable stages. Family::check_set grows 26→28 through one ordinary identity arm.
Silent failure: two suppression sites followed. The test's .ok() is asserted; failed step lookup returns named Malformed.
Tests: static mutation review. Selecting row 0 at tap line 465 breaks the finished-set test; dropping check_build at line 279 breaks the identity-refusal test; dropping step consistency at line 344 breaks the malformed-set test. Repeated-row padding is uncovered.
Gates: no pins or thresholds moved; family FAIL-first evidence is supplied.
Escape catalog: C1–C4 do not apply to the retained findings. No engine hot path changed.
Notes
No local builds or test runs were performed. Test mutation results above are from code inspection.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #261
There was a problem hiding this comment.
Verdict: MUST_FIX: invalid reference data and failed provenance checks can return success.
Must fix
1. Reject non-finite MTP reference values (kind A)
crates/refset/src/mtptaps.rs:410 and crates/refset/src/columns.rs:163: replace one f32 value in a tap or trunk file with NaN, keeping its length. TapSet::open succeeds and refset-check prints PASS. The writer rejects these values, but the reader admits them.
Close this before these sets become the next gate's oracle.
Replace columns::read_f32 with:
pub(crate) fn read_f32(path: &Path, want: u64, file: &str) -> Result<Vec<f32>, RefError> {
let values: Vec<f32> = read_bytes(path, want, file)?
.as_chunks::<4>()
.0
.iter()
.map(|c| f32::from_le_bytes(*c))
.collect();
if let Some(i) = values.iter().position(|v| !v.is_finite()) {
return Err(RefError::malformed(
path.display(),
format!("non-finite value at index {i}"),
));
}
Ok(values)
}In check_taps, replace the file-check loop with:
for t in &self.taps {
match t.elem {
Elem::F32 => { self.tap_f32(&t.name, t.row)?; }
Elem::I32 => { read_bytes(&self.dir.join(&t.file), u64::from(t.n), &t.file)?; }
}
}Remove the now-redundant finite check in nemotron::LogitsSet::logits; change its test's expected text to non-finite value at index 1. Add NaN/infinity cases for both MTP file kinds; removing the shared finite check must make them red.
2. Stop when the clean-tree query fails (kind A)
tools/ref/build-q35mtp-ref.sh:23: a corrupt or unreadable Git index makes git status exit 128 with empty stdout. The conditional succeeds as a clean-tree check; rev-parse HEAD can still succeed, so compilation and the commit-labelled build record continue. A shell reproduction returned 0 after the failed query.
Fix this before an unchecked oracle build is published. Replace line 23 with:
dirty=$(git_lcpp status --porcelain --untracked-files=no)
if [ -n "$dirty" ]; then3. Use one Rust owner for the MTP model path (kind B, R14)
crates/refset/src/arch/qwen35moe/mtp.rs:12 repeats crates/gpu-gates/src/bin/gate_qwen35moe_mtp.rs:71. Both identify the same draft file, and the gate already depends on refset.
Remove the duplicate before the gate and reference family acquire separate file updates. Keep the family's constant; replace the gate's DRAFT_FILE definition with:
use refset::arch::qwen35moe::mtp::MODEL as DRAFT_FILE;Non-blocking
⚠ Enforce the untapped comparison: tools/ref/q35mtp_ref.cpp:381 computes distances, but a nonzero distance still reaches # complete and installation. Add a refusal before completion, or state the admissible difference and its evidence. The supplied run establishes zero only for that run.
Checked
Duplicates: confirmed the model-path copy and the gate's existing dependency on refset.
Complexity: TapSet::read dispatches manifest headers and rows; Family::check_set gains one identity arm.
Silent failure: followed the MTP reader through the family's check to refset-check's PASS output. A failed Git-status stub bypasses the original conditional (exit 0); the proposed assignment stops with exit 128.
Tests: the wrong-build fixture would fail if check_build were removed. The MTP tests contain no non-finite-value refusal case. Rust and GPU tests were not run during validation.
Gates: no existing pin or band moved; the PR reports FAIL-first runs for the family identity checks.
Escape catalog: no exact C1–C4 pattern; the lost command-substitution status is related to C1. No engine hot path changed.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Filed outside this PR: #265
| SRC=$HERE/tools/ref/q35mtp_ref.cpp | ||
| L=$MTP_LCPP | ||
| git_lcpp() { git -c safe.directory="$L" -C "$L" "$@"; } | ||
| if [ -n "$(git_lcpp status --porcelain --untracked-files=no)" ]; then |
There was a problem hiding this comment.
A failed git status can have empty stdout and bypass this check; capture it separately so set -e stops the build.
| if [ -n "$(git_lcpp status --porcelain --untracked-files=no)" ]; then | |
| dirty=$(git_lcpp status --porcelain --untracked-files=no) | |
| if [ -n "$dirty" ]; then |
A mainline harness (q35mtp_ref) runs the target's trunk with its next-token rows kept, then the in-file MTP block in its own context step by step, and dumps the named graph tensors at chosen rows under a manifest that carries `# mtp` only when that context ran. The refset family mtp-qwen35moe-mainline reads them by tap name and row and refuses a set of another file, build or architecture, or one without the MTP context. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ct, and refuse a bad option or logit tools/ref/ref_common.h owns the numeric option parse, the ids file read and the exact binary write for hidden_ref and q35mtp_ref; each build record's source sha256 covers the header. A numeric option that is not a whole integer ends the run with 2 (Closes #243). q35mtp_ref's argmax ends the run with 1 on a non-finite logit, so no token or distance is written from such a row. The MTP taps tests take clefvis's scratch directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…'s one pin The family reads clefvis's QVIS_LCPP_BUILD, the commit of the mainline tree q35mtp_ref links against, instead of a second constant for it. The step-count check compares in u64, so a `# mtp` line at the integer's end is malformed, not an overflow. Four reader checks gain a case each: a short MTP run, a trunk of another length, a tapped row that is no step, and a tap at an unnamed row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… writer dumps a set The `# complete <tap rows> 0` trailer must count the rows the manifest holds and be its last line, so a set that lost a tap name at every row is malformed instead of passing. dump-q35mtp.sh takes a lock on the destination for the run, card or --cpu, through the install and the staging cleanup. columns::need is the one owner of "a data row needs its column line" for the MTP, vision and visref readers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… step's position Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The family's set is the card's. A --cpu run (another activation form) no longer replaces it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… a malformed count dump_ref, dump_mtp and dump_draft parse --tokens-count with parse_i64, so 64x is refused by name instead of read as 64. Each build record and run check hashes ref_io.h beside the harness source. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and an ids file write columns::one and columns::file_name serve the MTP step and nemotron readers; file_name also refuses a backslash and a NUL. ref_io.h's write_ids writes the ids both mainline harnesses record. Two option checks that could not fire are gone, and the MTP reader's tests cover a lost step row and a trunk file outside the set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…trailer check is its own function dump_ref, dump_mtp and dump_draft drop their own read_token_file, so the header's reader (NUL, sign and space refused) is the one that runs. TapSet::read hands the trailer to check_trailer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he trunk row is its own function columns::read_bytes and read_f32 read a set's value file by count and byte length; the nemotron logits read takes them. TapSet::read hands the trunk row to trunk_row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… pin is written once, and a step row's number is held Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… refuses an empty id line Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tes go through ref_io.h dump_ref parses each --tokens field with parse_i32, so `bad,42` is refused by name instead of read as `0,42`. ref_io.h's write_raw(path, data, size, count) replaces the three dumpers' own writers. q35mtp_ref parses each --rows piece with parse_i64. The tap file size check reads through columns::read_bytes. The reader test reads the step whose two argmaxes differ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…UL refusals Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…decoder serves three readers columns::i32s_of replaces dsref's and mtpref's copies and the MTP tap read's inline decode. check_taps refuses a repeated row, so taps cannot hide at a step that does not exist. Cases cover the repeated row, a set with no rows and no taps, no # tokens_count line and no trunk row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rows and no taps Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ble tree index stops the oracle build, and the MTP file has one owner columns::read_f32 refuses a NaN or infinite value by index, so MTP tap and trunk files and the nemotron logits share one finite check; check_taps reads each f32 tap through it. build-q35mtp-ref.sh stops when git status fails. gate_qwen35moe_mtp names its file through the MTP step family's MODEL. q35mtp_ref refuses to write a set whose tapped run moved the logits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Verdict: MUST_FIX: an existing destination with a trailing slash causes the dump installer to delete both the old set and its replacement.
Must fix
1. Normalize the destination before deriving the staging and lock paths
tools/ref/dump-q35mtp.sh:62 Kind A. With an existing Q35MTP_SET=/data/trial/, STAGE becomes /data/trial/.staging and the lock is /data/trial/.lock. After a successful dump, rm -rf "$SET" at line 81 deletes the old set, the finished stage, and the lock file. The mv at line 82 fails because its source is gone; the EXIT trap cannot recover it.
Fix this assignment before reruns can erase installed reference data. Normalize the destination before deriving either path:
SET=$(realpath -m -- "${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}")Checked
Duplicates: all three dryer pairs read; they represent distinct graph/element tags or step/case lookups.
Complexity: TapSet::read (new, 41) separates header/data parsing; check_taps (new, 17) checks coverage and files. Family::check_set (28 → 30) adds one dispatch arm.
Silent failure: the test's .ok() is asserted; unrecognized comment headers are skipped; first_argmax refuses non-finite logits by name.
Tests: static inspection confirms that the new NaN/infinity cases protect columns.rs:170. The inline replacement passes bash -n in the full script. Path derivation checked for trailing slashes and spaces; no model or GPU run performed during validation.
Gates: the existing GPU gate takes its model path from the shared constant; its bands and pins do not change. The PR reports FAIL-first results and GPU gate output.
Escape catalog: C1–C4 checked; none matches the finding.
Card check: gpu-evidence. This pull request changes crates/gpu-gates/src/bin/gate_qwen35moe_mtp.rs and carries a gate run on the author's card, which counts as verified.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| esac | ||
| done | ||
| if [ ${#taps[@]} = 0 ]; then for t in "${MTP_TAPS[@]}"; do taps+=(--tap "$t"); done; fi | ||
| SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64} |
There was a problem hiding this comment.
With an existing destination ending in '/', the stage and lock are inside SET, so installation deletes the completed dump before mv.
| SET=${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64} | |
| SET=$(realpath -m -- "${Q35MTP_SET:-$BLOOMERY_DATA/ref-mtp/qwen35moe_mainline_prose64}") |
|
Closing for now: this reference set is step M2b-1 of #30 and has no use until the draft it checks exists. The branch |
Part of #30 (qwen35moe NextN MTP draft), step M2b-1: mainline's MTP graph as a tap-by-tap reference set. The next step (M2b-2) holds the card's draft step to it in
gate_qwen35moe_mtp. Its one gate edit:gate_qwen35moe_mtptakes the draft file's name fromrefset::arch::qwen35moe::mtp::MODELinstead of a copy of it; no clause, band or pin moves. It builds on #236'stools/ref/ref_io.h(merged as d0cbd4b). Rebased onto main73c307c4ata4e71119; the PR's own diff equals the fold's at2cfad01fon4308ac0c(git patch-id --stable0fd0a68a on both).What
tools/ref/q35mtp_ref.cpp, a harness linked against mainline 36a73916:load_mtp.llama_set_embeddings_nextn).LLAMA_CONTEXT_TYPE_MTP), it runs every step 0..=62 in order, with tokenids[i+1], positioniand the trunk's rowh[i](llama_batch_ext_set_embd_token, ascommon.cpp:2220does;set_embd_stateis a stub in this tree).--tap, repeatable) at rows {0, 5, 31, 62}. The recipe's default is 17 taps, frommtp_hnormtoresult_output, plus the router's logits, top-k, weights and the shared gate.# mtp <layers> <steps>is written only after the MTP context ran every step, and# completeonly by the final rename.build-q35mtp-ref.shrecords the source sha256, the commit and libllama's md5.dump-q35mtp.shrefuses a binary that differs from that record, stages, and runs undergpu-gate.sh.crates/refset: the familymtp-qwen35moe-mainline(the A1 file, build36a73916e) and the readermtptaps(by tap name and row). The family check refuses by name a set of another file, build or architecture, or one without# mtp.공통화한 것 (Commonized)
Identityandcheck_set: one new arm, no second check.tools/ref/ref_io.h(refset: Nemotron 3 Nano last-position logits, read from one reference set #236) is the one owner of the mainline and ik harnesses' input and output contract.q35mtp_ref.cppuses itsparse_i32,read_token_file,write_rawand the newwrite_bytes(write_rawiswrite_bytesof f32 rows).dump_ref,dump_mtpanddump_draftread--tokens-countwith itsparse_i64, so64xis refused by name instead of read as 64. Each build record and run check hashesref_io.hbeside the harness source; the installed ik dumpers rebuild once (just build-ref-dump,build-ref-dump-mtp,build-ref-dump-draft) before their next dump.testkit::scratch, the crate's one scratch-directory owner.columns::needis the one owner of "a data row needs its column line" for the MTP, vision and visref readers (two local copies removed).At,one(), the header loop).corpus-prose.ids' first 64 ids: the d1k set's first 64, the same md5.Evidence
Cards: RTX A6000, driver 615.71.09, CUDA 13.4. Base origin/main e6050c5, head 0378d6e (parent 3915120; the trailer fold is 7939746, parent b177903; 1d97576 is the M2b-1 commit rebased).
nvidia-smi: NVIDIA GeForce RTX 3090, 615.71.09; NVIDIA RTX A6000, 615.71.09.
Dump: job s590 on the A6000. 42/42 layers offloaded, and the CUDA MTP context ran.
gpu-gate.sh: q35mtp_dump ran 4 s (exit 0).# complete 68,# build 36a73916e,# mtp 1 63,# rows 0 5 31 62.Mainline's MTP acceptance (printed, not pinned), as gate (s) counts it: 11/16 of the last 16 steps (68.8 %), 47/63 over every step. Our card draft, gate (s): 12/16 (75 %). A CPU trial of the same harness gave 13/16 and 49/63.
just gate-refset:hw_refset: mtp-qwen35moe-mainline … PASS, 175 sets, 0 failed;cargo test -p bloomery-refset --lib200 passed.just refset-check: 338 sets, 0 failed.tools/check-recipes.sh: ok.recipes.py check --fill: 0 rows.The dump and the FAIL-first ran before a machine reboot at 13:36. The gate runs are from before it; the reader tests were run again after it.
The review fold, job s672 on the A6000:
q35mtp_refrebuilt from this tree (the shared header in its source sha256; that run was before the move toref_io.h). A trial dump to a scratch set ran:gpu-gate.sh: q35mtp_dump ran 4 s (exit 0). Its 70 data files are byte-identical to the published set, and the manifests are equal outside the flags and date lines. The scratch set was removed.Malformed options, with the model path absent so that only the parse is under test:
q35mtp_ref(-c bad,-ngl 9x,-t '',--tokens-count 64x) and the newhidden_ref(-ngl bad,-ub 1.5,-c 99999999999,--tokens-count 64x, job s671) each exit 2 with<tool>: <opt> takes an integer, not '<value>'.q35mtp_refbuilt from 0e4dc85 with-c badand the installedhidden_refwith-ngl badboth reachfailed to load /nonexistent.gguf, rc 1.cargo test -p bloomery-refset --lib: 200 passed.The second review fold (7939746): the
# complete <tap rows> 0trailer must count the tap rows the manifest holds and be its last line. The published set's trailer is# complete\t68\t0over 68 tap rows.just refset-checkafter this fold: 338 sets, 0 failed,mtp-qwen35moe-mainline … PASS.dump-q35mtp.sh --cpuexits 64 unlessQ35MTP_SETnames a trial set, so the card's family set is never replaced by a CPU run (bash tools/ref/dump-q35mtp.sh --cpu→dump-q35mtp: --cpu writes a trial set; name it with Q35MTP_SET …, rc 64, before any profile or binary check).dump-q35mtp.shholds a lock on the destination (<set>.lock,flock -n, exit 75 when another writer holds it) for the run, card or--cpu, through the install and the staging cleanup.The move to
ref_io.h(headf35de2fd, on refset: Nemotron 3 Nano last-position logits, read from one reference set #236's433f8e3d), box CPU jobs, each binary built into a scratch directory:dump_ref(build-dump.sh) andq35mtp_ref(build-q35mtp-ref.sh) built;--tokens-count 64xexits 2 withdump_ref: --tokens-count wants an integer from 1 to 9223372036854775807, got 64xandq35mtp_ref: --tokens-count wants an integer from 2 to 2147483647, got 64x.dump_mtpanddump_draft: their build scripts rebuild the ik libraries under a timing lease, so they were compiled-fsyntax-onlyagainst their profiles' ik headers instead: both rc 0.ref_io.hwith itsref_io_testcase (just test-ref-io: ok).cargo test -p bloomery-refset --lib: 210 passed.Head
05031f98(Shingler de8dba9 fold, on maind0cbd4bb):dump_ref --tokensparses each field withparse_i32:--tokens bad,42→dump_ref: --tokens wants an integer from 0 to 2147483647, got bad, rc 2;--tokens 64xthe same (box build).ref_io.h'swrite_raw(path, data, size, count)replacesdump_ref,dump_mtpanddump_draft's own writers;dump_mtpanddump_draftcompile-fsyntax-only, rc 0.q35mtp_ref --rowsparses each piece withparse_i64:+5,5and5,are refused by name,0,5,31,62reaches the model load (box build).columns::read_bytes. The reader test reads step 1, whose two argmaxes differ.cargo test -p bloomery-refset --lib: 211 passed;just test-ref-io: ok.3d9df876: two cases holdfile_name's backslash and NUL refusals (the two characters dropped →a tap file with a backslashred, killed; the clean run green).Head
de8dba95: the nemotron tests taketestkit::scratch(tags prefixednemotron-);TapSet::idsrefuses an empty id line asnemotron.rsandref_io.hdo (the refusal dropped →an empty line among the prompt idsred, killed; the clean run green).Head
d39c851d: the nemotron reader takescolumns::need;qwen35moe.sh's d1k step sets readMTP_IDSandMTP_IDS_SHA256, so the prose ids path and pin are written once; a case swaps two step rows' numbers (s.row != idropped → red, killed; the clean run green).Head
a05aa083, restacked on refset: Nemotron 3 Nano last-position logits, read from one reference set #236's07aa96b1: the nemotron logits read iscolumns::read_f32plus refset: Nemotron 3 Nano last-position logits, read from one reference set #236's finite check;dump-q35mtp.shtakesref-paths.sh'sdrop_incomplete_stage.cargo test -p bloomery-refset --lib: 211 passed.Head
6da0dc73:columns::read_bytesandread_f32are the one f32 file reader of the MTP step and nemotron sets;TapSet::readhands the trunk row totrunk_row.cargo test -p bloomery-refset --lib: 210 passed.Head
2edf04b8:dump_ref,dump_mtpanddump_draftdrop their ownread_token_file;ref_io.h's reader runs in all of them.dump_refbuilt on the box: an ids file with a NUL inside a record →invalid token id '2', rc 2;--tokens-count 64x→ rc 2.dump_mtpanddump_draftcompile-fsyntax-only, rc 0.TapSet::readhands the trailer tocheck_trailer.Head
e6c2a4bd:columns::oneandcolumns::file_nameserve the MTP step and nemotron readers (file_namealso refuses\\and NUL);ref_io.h'swrite_idswrites both mainline harnesses' ids;dump_ref's zero-count andnemotron_ref's-ubchecks that could not fire are gone.q35mtp_ref,nemotron_refanddump_refcompile (-fsyntax-onlyon the box, rc 0);just test-ref-io: ok. Two reader cases added (a lost step row; a trunk file outside the set), each FAIL-first killed by its mutant, the clean run green.Head
7a9ef349(Shingler 3d9df87 fold):check_tapsrefuses a repeated# rowsentry;columns::i32s_ofis the one little-endian i32 decoder (dsref.rsandmtpref.rscopies removed, the tap read takes it). New cases: repeated rows hiding taps at a step that does not exist, no# rowsand no tap rows, no# tokens_countline, no trunk row. FAIL-first in one--clean oncerun: the repeated-row guard, the empty-set guard and the tokens-count check each dropped → its case red, all killed; the clean run green.cargo test -p bloomery-refset --lib: 211 passed.Head
db5c9370: a case removes only the tap rows under# rows 0 2(the guard's||made&&→ red, killed; the clean run green).Head
2cfad01f(Shingler db5c937 fold, on main4308ac0c):columns::read_f32refuses a non-finite value by index, so every MTP tap and trunk file and the nemotron logits share one finite check (check_tapsreads each f32 tap through it);build-q35mtp-ref.shstops whengit statuson the tree fails;q35mtp_refrefuses to write a set whose tapped logits differ from the untapped ones or whose argmax differs at any step. Two cases: a NaN in a tap file (index 0), an infinity in the trunk file (index 3). FAIL-first (--clean once): the finite check dropped → both cases red, killed; the clean run green.cargo test -p bloomery-refset --lib: 216 passed.just gate-gpu-qwen35moe-mtpat 2cfad01f on the A6000 (job s825):gpu-gate.sh: gate_qwen35moe_mtp ran 9 s (exit 0),gate_qwen35moe_mtp: PASS.q35mtp_refbuilds against llama.cpp36a73916(job s828, rc 0). Ona4e71119:cargo test -p bloomery-refset --lib217 passed (main added one);tools/check-recipes.sh: ok.FAIL-first (
tools/mutant-run.sh, each mutant alone)# mtp:the_family_refuses_a_set_of_another_file_build_or_arch_or_without_the_mtp_contextFAILED, killed.q35mtp_ref: non-finite logit nan at id 7,q35mtp_dump ran 3 s (exit 1), no set installed. Killed.--clean once, the gatecargo test -p bloomery-refset --lib a_set_whose_files_or_rows_disagree_is_malformed). Each mutant was killed by its own case, and the clean run is green:mtp.steps + 1in u32 (an overflow panic).--clean once, the same gate): the count check dropped →a tap name gone at every row, the trailer unchangedred; the last-line check dropped →a line after the trailerred. Both killed, the clean run green.--clean once):f[2] != "0"dropped →a trailer whose second field is not 0red;s.pos != s.rowdropped →a step at another position than its rowred. Both killed, the clean run green.Follow-up (not in this PR)
mtptaps.rslets a repeated# rows,# modelor# tokens_countheader replace the earlier one (the writer emits each once; it takes a hand-edited manifest). Refusing a header already set, asstepandtaprows do throughseen, is a reader change of its own.dump-q35mtp.shremoves the old set before it moves the new one in; a failedmvleaves the family without a set until the next dump. Moving the old set aside first is a change of its own.nemotron.rsdoes not require its# completetrailer on the last line, asmtptaps.rsdoes; the shared trailer rule is one change with the scans below.crates/refsethas elevenAttypes and four# completescans; one taggedAtand one scan touch nine files, a change of their own.build-dump-draft.shrecords only the ik build indump_draft.build, sodump-draft.shdoes not refuse adump_draftbuilt before this PR (which reads--tokens-count 64xas 64). Rebuild it (just build-ref-dump-draft) before its next dump; recording the source andref_io.hsha256 there, asbuild-dump.shdoes, is a change of its own.Pre-review CLEAN at 2cfad01f215003f4a72326ea3a389f90eca65752 (the same patch as a4e7111).
🤖 Generated with Claude Code