From e5b22116fe77d5c939aadbc4ff1617ed9b51b71f Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sat, 15 Aug 2026 00:04:03 +0000 Subject: [PATCH] fix(GATE-OP-PARITY-MANIFEST): refuse a throwing golden by name instead of aborting the pass (#776) Closes #776. `test_op_parity` threw `json.exception.type_error.302` out of the CPU golden pass instead of failing an assertion, so the walker stopped at the first bad manifest and left every golden behind it unchecked. The check that goes silent with it is `no runner for op`, which is what caught #559's missing runner arm. A gate that stops gating is the defect here, not the red line. The artifact that threw was `tests/parity/goldens/minimax_music3_oracle/manifest.json`, and that half is already fixed: #755 landed `043e56862` eight minutes after #776 was filed, so the two issues describe one capture. Reproducing #776 on the FIXED tree is what shows the rest of the hole. #755 closed which files the walker accepts. It did not close what happens when an accepted file throws anyway. Setting one tensor `dtype` to `null` in `rmsnorm_f32_8x128` still threw `type_error.302` at the TEST_CASE line and took the pass from 142 assertions to 37, with 45 committed goldens never reached. Two throw sites remained: `json::parse` on the manifest, and any runner reading a field out of it. Both now funnel through `GuardGoldenStage`, which turns a `std::exception` into a `FAIL_CHECK` naming `goldens//manifest.json`, quotes the original exception, and continues to the next golden. The dispatch chain moves into `RunGoldenCase` so its `continue` statements become `return false`, which is what lets one case be caught without unwinding the pass. The sibling `#755` audit case gets the same guard, because it reads every committed manifest and must not stop at the first unparsable one either. `std::exception` is the deliberate width. It covers json's `type_error` and `parse_error` and the `std::runtime_error` that `LoadTensor` raises for a shape or dtype-size mismatch, none of which name a file the caller already knows. doctest's `TestFailureException` is not derived from `std::exception` (`third_party/doctest/doctest.h:2563`), so a `REQUIRE` or `FAIL` inside a runner still aborts as before. That is load-bearing: the `no runner for op` refusal is a `FAIL`, and a guard that swallowed it would have made this red go away without restoring the gate. Mutation evidence, tree restored byte-for-byte after each (823 files, md5). Before: nulled `dtype` throws, 37 assertions run. After: refused as `goldens/rmsnorm_f32_8x128/manifest.json: the runner for op "rmsnorm" threw ...` with 45 op cases still run and 152 assertions still checked. An unparsable manifest and a `null` or absent `"op"` are refused by name the same way. A manifest naming `no_such_runner_op_559` still hits `FATAL ERROR: no runner for op`, so the #559 guard still bites. Clean tree is green at 13 cases and 153 assertions, 46 op cases and 1 non-op skip, unchanged from before the change, and green again under the `sanitize-cpu (address,undefined)` lane with the job's own environment. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [Claude Code] --- .agents/roadmap_v1.md | 1 + tests/parity/goldens/README.md | 23 +++ tests/parity/test_op_parity.cpp | 300 ++++++++++++++++++++++++-------- 3 files changed, 248 insertions(+), 76 deletions(-) diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index b82d22b5c..9e82ff90d 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -223,6 +223,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#824](https://github.com/mudler/vllm.cpp/issues/824) | `ENG-EXPERT-STREAM` | The spec's record is stale in four places: it puts GGUF expert streaming out of scope because it "needs per-expert slicing of 3D tensors" and must wait for `QUANT-GGUF-KEEPQ-LOADER`, but that slicer landed `429e19d6a` on 2026-07-22, twelve days AFTER the spec was written, and is now the production decode path (`OwnGgufQuantBlocks` row_offset at `qwen3_5_gguf_weights.cpp:57`; `GemmRowSlice` per-expert at `deepseek_v4.cpp:1004,2487` and `laguna.cpp:1225`). The correction inverts a scheduling decision: the GGUF lane needs NO Marlin bank at all (the mmap'd GGUF file already is one), so it is plausibly the CHEAPER lane, and it is the only one that can serve a Qwen3.8-class checkpoint since every such checkpoint that exists is GGUF. FIXED IN FLOW | bug | | [#834](https://github.com/mudler/vllm.cpp/issues/834) | — | No row owns router-lookahead prefetch for offloaded MoE experts. `ENG-EXPERT-STREAM` W3 copies router identifiers device to host and waits once per MoE layer (`specs/expert-streaming.md:377`), which is a synchronous stall. The only overlap work in that row is W6, and W6 runs `only if W3 trace shows wait dominance` and needs a separate accepted spike (`:380`). `ENG-WEIGHT-OFFLOAD` has a `PrefetchOffloader` arm, and it selects layers by position and never reads the router (`vllm/config/offload.py:48-76`). Prefetch is the lever that converts the per-layer fetch stall into an overlapped transfer, so the gap is recorded rather than left to be rediscovered | feature | | [#835](https://github.com/mudler/vllm.cpp/issues/835) | — | No row owns GPUDirect Storage, also called GDS or cuFile, for weight reads. `ENG-EXPERT-STREAM` W2 uses an `O_DIRECT` pool with aligned staging (`specs/expert-streaming.md:376`), which bypasses the page cache and still stages every expert through host memory. GPUDirect appears twice in the records and neither entry covers weights: `KV-MOONCAKE-STORE` names it for KV blocks over a fabric no box we own has, and `specs/lmcache-cpp-client-connector.md:305` marks GDS `NOT SCHEDULED` as an LMCache backend. The value differs by host, so a row must measure both paths before it claims a number | feature | +| [#776](https://github.com/mudler/vllm.cpp/issues/776) | `GATE-OP-PARITY-MANIFEST` | `test_op_parity` THREW `json.exception.type_error.302` out of the CPU golden pass instead of failing an assertion, so the walker's `no runner for op` guard — the check that caught #559's missing runner arm — stopped running for every golden after the offender. The offending artifact was `tests/parity/goldens/minimax_music3_oracle/manifest.json`, and that half is #755, already fixed by `043e56862`. What #755 closed was the walker's INPUT set; what it did not close is the walker's EXCEPTION surface, and reproducing #776 on the fixed tree shows why the distinction matters: nulling one tensor `dtype` in `rmsnorm_f32_8x128` still threw `type_error.302` at the TEST_CASE line and cut the pass from 142 assertions to 37, with 45 committed goldens left unchecked. Two throw sites remained — `json::parse` on the manifest, and any runner reading a field out of it — and both now funnel through `GuardGoldenStage`, which converts a `std::exception` into a `FAIL_CHECK` naming `goldens//manifest.json` and continues to the next golden. `doctest::detail::TestFailureException` is deliberately NOT a `std::exception` (`third_party/doctest/doctest.h:2563`), so the #559 `FAIL` still aborts loudly and the widening cannot mute it. FIXED IN FLOW | bug | ## Top-level portfolio diff --git a/tests/parity/goldens/README.md b/tests/parity/goldens/README.md index 6ca29c8ce..9dc20a1b5 100644 --- a/tests/parity/goldens/README.md +++ b/tests/parity/goldens/README.md @@ -32,6 +32,29 @@ exactly the set of manifests declaring a known op, and everything else is either loud-and-listed or loud-and-failing. A future oracle capture dropped into this tree cannot land unnoticed — it fails, by name, with the fix in the message. +## A malformed golden never aborts the pass + +Closing the input set is not the same as closing the *exception* surface, and +#776 is the difference. A manifest can be listed, named, and accept a runner and +still make the walker throw: the file can be invalid JSON, or a field the runner +reads can be absent or `null`. Either exception escaping `RunGoldenPass` aborts +the whole test case, so **every golden the walker had not reached yet goes +unchecked** — including the `no runner for op` check this directory depends on. +That is a gate that has stopped gating while still looking like one red line. + +Both throw sites are now guarded. An exception from parsing a manifest, or from +the runner reading it, becomes a `FAIL_CHECK` that names +`goldens//manifest.json` and quotes the original exception, and the pass +**continues to the next golden**. So a malformed golden costs you exactly its +own case, and the report names the file instead of a line number in +`test_op_parity.cpp`. + +The guard catches `std::exception` and nothing wider. doctest's +`TestFailureException` is deliberately not derived from it, so a `REQUIRE` or +`FAIL` inside a runner — the unregistered-op refusal above included — still +aborts the pass exactly as before. The guard cannot mute an assertion; it only +converts a thrown diagnostic that names no file into one that does. + ## Adding an op-parity golden `manifest.json` carries at least: diff --git a/tests/parity/test_op_parity.cpp b/tests/parity/test_op_parity.cpp index 19d01ae6f..f9699f6be 100644 --- a/tests/parity/test_op_parity.cpp +++ b/tests/parity/test_op_parity.cpp @@ -1930,6 +1930,136 @@ ManifestClass ClassifyGoldenManifest(const std::string& case_name, const json& m return {ManifestVerdict::kOpGolden, m.at("op").get(), ""}; } +// GATE-OP-PARITY-MANIFEST (#776). Formats the by-name refusal for a golden +// that made the walker throw. `stage` names which half threw: parsing the +// manifest, or the runner reading a field out of it. +// +// The classifier above closed the walker's INPUT set; it did not close the +// walker's exception surface. Two throw sites remain — `json::parse` on a +// manifest that is not valid JSON, and any runner reading a field that is +// absent or null. Either one escaping `RunGoldenPass` aborts the whole test +// case, so every golden the walker had not reached yet goes UNCHECKED, and the +// doctest report names a TEST_CASE line and no file. That is the shape of #776 +// and it is why diagnosing it needed a bisect. +// +// Pure, so the mutation test below drives it without touching committed +// evidence. +std::string GoldenExceptionMessage(const std::string& case_name, + const std::string& stage, + const std::string& what) { + return "goldens/" + case_name + "/manifest.json: " + stage + " threw \"" + + what + + "\" — a malformed golden is refused BY NAME and the pass continues, " + "because an exception escaping the walker leaves every later golden " + "unchecked (#776). See tests/parity/goldens/README.md"; +} + +// Runs `body` and converts any std::exception it throws into that by-name +// refusal. Returns nullopt when `body` completed. +// +// std::exception is the deliberate width: it catches json's type_error and +// parse_error AND the std::runtime_error that LoadTensor raises for a shape or +// dtype-size mismatch, all of which name a manifest the caller knows and the +// exception does not. doctest's own TestFailureException is NOT derived from +// std::exception (third_party/doctest/doctest.h:2563), so a REQUIRE or FAIL +// inside a runner still aborts the case exactly as before — in particular the +// `no runner for op` FAIL that this walker exists to raise. +template +std::optional GuardGoldenStage(const std::string& case_name, + const std::string& stage, + F&& body) { + try { + body(); + } catch (const std::exception& e) { + return GoldenExceptionMessage(case_name, stage, e.what()); + } + return std::nullopt; +} + +// Dispatches one classified op golden to its runner. Returns true when the case +// RAN and counts toward the callers' floors; false when it declared itself +// skipped (no checkpoint, wrong device, or owned by a focused test). +// +// Extracted from the walker so the whole chain sits inside one guarded call: +// the old `continue` statements are `return false` here, which is what lets a +// throw be caught per case instead of unwinding the entire pass. +bool RunGoldenCase(Backend& b, Queue& q, Device dev, const fs::path& dir, + const std::string& op, const json& m) { + if (op == "rmsnorm") { + RunRmsNorm(b, q, dir, m); + } else if (op == "matmul") { + RunMatmul(b, q, dir, m); + } else if (op == "silu_and_mul") { + RunSiluAndMul(b, q, dir, m); + } else if (op == "embedding") { + RunEmbedding(b, q, dir, m); + } else if (op == "rope") { + RunRope(b, q, dir, m); + } else if (op == "long_context_rope") { + RunLongContextRope(b, q, dir, m); + } else if (op == "causal_conv1d_fwd") { + RunCausalConv1dFwd(b, q, dir, m); + } else if (op == "causal_conv1d_update") { + RunCausalConv1dUpdate(b, q, dir, m); + } else if (op == "l2norm") { + RunL2Norm(b, q, dir, m); + } else if (op == "rmsnorm_gated") { + RunRmsNormGated(b, q, dir, m); + } else if (op == "gdn_prefill") { + RunGdnPrefill(b, q, dir, m); + } else if (op == "gdn_decode") { + RunGdnDecode(b, q, dir, m); + } else if (op == "moe_router_topk") { + RunMoeRouterTopK(b, q, dir, m); + } else if (op == "moe_block") { + RunMoeBlock(b, q, dir, m); + } else if (op == "dense_attention") { + RunDenseAttention(b, q, dir, m); + } else if (op == "qwen36_embed") { + if (!RunQwen36Embed(b, q, dir, m)) return false; // skip (no ckpt) + } else if (op == "qwen36_norm") { + if (!RunQwen36Norm(b, q, dir, m)) return false; + } else if (op == "qwen36_gdn_layer" || op == "qwen36_fullattn_layer") { + if (!RunQwen36Layer(b, q, dir, m)) return false; + } else if (op == "qwen36_logits") { + if (dev.type != DeviceType::kCUDA) { + MESSAGE("SKIP " << dir.filename().string() + << ": full real-model logits gate is CUDA-only"); + return false; + } + // Same op for both gates; dispatch by tag (27B dense vs 35B MoE loader). + const bool ran = (GoldenTag(dir) == 27) ? RunQwen27Logits(b, q, dir, m) + : RunQwen36Logits(b, q, dir, m); + if (!ran) return false; + } else if (op == "qwen3_5_mtp_head") { + // Multi-GiB, two-checkpoint gate owned by the focused test case below. + // Recognize it here so committed goldens never trip the stale-runner + // guard, but do not make every generic op-parity pass reload both models. + MESSAGE("SKIP op '" << op << "' case '" << dir.filename().string() + << "': owned by focused Qwen3.5 MTP head parity test"); + return false; + } else if (op == "gdn_packed_decode_bf16") { + // This diagnostic replays several alternative consumer boundaries and + // requires CUDA. Keep it in the focused test below instead of making the + // generic CPU/CUDA golden pass execute the same fixture twice. + MESSAGE("SKIP op '" << op << "' case '" << dir.filename().string() + << "': owned by focused GDN packed-decode boundary test"); + return false; + } else if (PendingRunnerOps().count(op)) { + MESSAGE("SKIP op '" << op << "' case '" << dir.filename().string() + << "': runner pending (see PendingRunnerOps)"); + return false; // does not count toward the case floor + } else { + // THE #559 GUARD. FAIL throws doctest's TestFailureException, which is not + // a std::exception, so GuardGoldenStage does NOT swallow it: a golden + // naming an unregistered op still aborts the pass, loudly, as it always + // did. The `return false` below is unreachable and exists for the compiler. + FAIL("no runner for op '" << op << "' — add one before committing goldens"); + return false; + } + return true; +} + // Runs every golden case on `dev` and returns how many ran. Both passes use // the same manifests and the same tolerances — the committed goldens are the // bar for every backend. `non_op_out`, when given, receives how many manifests @@ -1949,9 +2079,14 @@ int RunGoldenPass(Device dev, int* non_op_out = nullptr) { // test_tokenizer_parity) carry no manifest.json; the `cases >= 24` floor // in the callers still guards against op cases silently disappearing. if (!fs::exists(mf)) continue; - json m = json::parse(std::ifstream(mf)); - const ManifestClass mc = - ClassifyGoldenManifest(entry.path().filename().string(), m); + const std::string name = entry.path().filename().string(); + json m; + if (auto bad = GuardGoldenStage(name, "parsing manifest.json", + [&] { m = json::parse(std::ifstream(mf)); })) { + FAIL_CHECK(*bad); + continue; + } + const ManifestClass mc = ClassifyGoldenManifest(name, m); if (mc.verdict == ManifestVerdict::kMalformed) { // FAIL_CHECK, not FAIL: name EVERY offender in one run rather than // aborting the pass at the first one. @@ -1964,81 +2099,20 @@ int RunGoldenPass(Device dev, int* non_op_out = nullptr) { continue; } const std::string& op = mc.op; - INFO("case " << entry.path().filename().string()); + INFO("case " << name); if (std::getenv("VLLM_PARITY_PRINT_MARGINS") != nullptr) - std::printf("case %s (%s)\n", entry.path().filename().string().c_str(), + std::printf("case %s (%s)\n", name.c_str(), dev.type == DeviceType::kCUDA ? "cuda" : "cpu"); - if (op == "rmsnorm") { - RunRmsNorm(b, q, entry.path(), m); - } else if (op == "matmul") { - RunMatmul(b, q, entry.path(), m); - } else if (op == "silu_and_mul") { - RunSiluAndMul(b, q, entry.path(), m); - } else if (op == "embedding") { - RunEmbedding(b, q, entry.path(), m); - } else if (op == "rope") { - RunRope(b, q, entry.path(), m); - } else if (op == "long_context_rope") { - RunLongContextRope(b, q, entry.path(), m); - } else if (op == "causal_conv1d_fwd") { - RunCausalConv1dFwd(b, q, entry.path(), m); - } else if (op == "causal_conv1d_update") { - RunCausalConv1dUpdate(b, q, entry.path(), m); - } else if (op == "l2norm") { - RunL2Norm(b, q, entry.path(), m); - } else if (op == "rmsnorm_gated") { - RunRmsNormGated(b, q, entry.path(), m); - } else if (op == "gdn_prefill") { - RunGdnPrefill(b, q, entry.path(), m); - } else if (op == "gdn_decode") { - RunGdnDecode(b, q, entry.path(), m); - } else if (op == "moe_router_topk") { - RunMoeRouterTopK(b, q, entry.path(), m); - } else if (op == "moe_block") { - RunMoeBlock(b, q, entry.path(), m); - } else if (op == "dense_attention") { - RunDenseAttention(b, q, entry.path(), m); - } else if (op == "qwen36_embed") { - if (!RunQwen36Embed(b, q, entry.path(), m)) continue; // skip (no ckpt) - } else if (op == "qwen36_norm") { - if (!RunQwen36Norm(b, q, entry.path(), m)) continue; - } else if (op == "qwen36_gdn_layer" || op == "qwen36_fullattn_layer") { - if (!RunQwen36Layer(b, q, entry.path(), m)) continue; - } else if (op == "qwen36_logits") { - if (dev.type != DeviceType::kCUDA) { - MESSAGE("SKIP " << entry.path().filename().string() - << ": full real-model logits gate is CUDA-only"); - continue; - } - // Same op for both gates; dispatch by tag (27B dense vs 35B MoE loader). - const bool ran = (GoldenTag(entry.path()) == 27) - ? RunQwen27Logits(b, q, entry.path(), m) - : RunQwen36Logits(b, q, entry.path(), m); - if (!ran) continue; - } else if (op == "qwen3_5_mtp_head") { - // Multi-GiB, two-checkpoint gate owned by the focused test case below. - // Recognize it here so committed goldens never trip the stale-runner - // guard, but do not make every generic op-parity pass reload both models. - MESSAGE("SKIP op '" << op << "' case '" - << entry.path().filename().string() - << "': owned by focused Qwen3.5 MTP head parity test"); - continue; - } else if (op == "gdn_packed_decode_bf16") { - // This diagnostic replays several alternative consumer boundaries and - // requires CUDA. Keep it in the focused test below instead of making the - // generic CPU/CUDA golden pass execute the same fixture twice. - MESSAGE("SKIP op '" << op << "' case '" - << entry.path().filename().string() - << "': owned by focused GDN packed-decode boundary test"); + bool ran = false; + if (auto bad = GuardGoldenStage( + name, "the runner for op \"" + op + "\"", + [&] { ran = RunGoldenCase(b, q, dev, entry.path(), op, m); })) { + // FAIL_CHECK, not FAIL, for the same reason the classifier uses it: name + // every offender in one run instead of stopping at the first. + FAIL_CHECK(*bad); continue; - } else if (PendingRunnerOps().count(op)) { - MESSAGE("SKIP op '" << op << "' case '" << entry.path().filename().string() - << "': runner pending (see PendingRunnerOps)"); - continue; // does not count toward the case floor - } else { - FAIL("no runner for op '" << op << "' — add one before committing goldens"); } - ++cases; + if (ran) ++cases; } b.DestroyQueue(q); // std::string, not the bare ternary: doctest 2.5.2 streams a `const char*` @@ -2157,8 +2231,16 @@ TEST_CASE("every committed goldens manifest declares what it is (#755)") { const fs::path mf = entry.path() / "manifest.json"; if (!fs::exists(mf)) continue; const std::string name = entry.path().filename().string(); - const ManifestClass mc = - ClassifyGoldenManifest(name, json::parse(std::ifstream(mf))); + // Guarded for the same reason the walker is (#776): this case audits EVERY + // committed manifest, so an unparsable one must be named and counted, not + // allowed to throw and leave the rest of the tree unaudited. + json m; + if (auto bad = GuardGoldenStage(name, "parsing manifest.json", + [&] { m = json::parse(std::ifstream(mf)); })) { + FAIL_CHECK(*bad); + continue; + } + const ManifestClass mc = ClassifyGoldenManifest(name, m); if (mc.verdict == ManifestVerdict::kMalformed) { FAIL_CHECK(mc.message); continue; @@ -2174,6 +2256,72 @@ TEST_CASE("every committed goldens manifest declares what it is (#755)") { CHECK(fs::exists(root / "minimax_music3_oracle" / "manifest.json")); } +// GATE-OP-PARITY-MANIFEST (#776). The classifier above closed WHICH files the +// walker accepts; this closes what happens when one of them throws anyway. +// The property under test is not "the message is nicer" — it is that a single +// malformed golden can no longer take the rest of the pass down with it, which +// is how #776 left the unregistered-op guard silently not running. +TEST_CASE("a golden that throws is refused by name and does not abort (#776)") { + // The exact exception that closed the CPU pass on main: nlohmann raises + // type_error.302 for a string read of a null, and its what() names neither + // the file nor the key. + const auto null_read = [] { + const json spec = json{{"dtype", nullptr}}; + (void)spec.at("dtype").get(); + }; + const auto raw = [&]() -> std::string { + try { + null_read(); + } catch (const std::exception& e) { + return e.what(); + } + return ""; + }(); + REQUIRE(raw.find("302") != std::string::npos); + CHECK(raw.find("rmsnorm_f32_8x128") == std::string::npos); // names no file + + // Guarded, the same throw becomes a refusal that names the case. + const std::optional bad = + GuardGoldenStage("rmsnorm_f32_8x128", "the runner for op \"rmsnorm\"", + null_read); + REQUIRE(bad.has_value()); + CHECK(bad->find("goldens/rmsnorm_f32_8x128/manifest.json") != + std::string::npos); + CHECK(bad->find("the runner for op \"rmsnorm\"") != std::string::npos); + CHECK(bad->find(raw) != std::string::npos); // keeps the original diagnosis + + // A parse failure — the walker's other throw site — is named the same way. + const std::optional unparsable = + GuardGoldenStage("some_case", "parsing manifest.json", [] { + const json parsed = json::parse("{,}"); + (void)parsed; + }); + REQUIRE(unparsable.has_value()); + CHECK(unparsable->find("goldens/some_case/manifest.json") != + std::string::npos); + + // THE MUTATION THIS GUARD EXISTS FOR: the pass keeps going. A case after the + // offender still runs, so the goldens behind it stay gated. + int later_ran = 0; + const std::optional ok = + GuardGoldenStage("next_case", "the runner for op \"matmul\"", + [&] { ++later_ran; }); + CHECK_FALSE(ok.has_value()); + CHECK(later_ran == 1); + + // And the widening must NOT reach doctest's own abort: TestFailureException + // is what FAIL("no runner for op ...") throws, so swallowing it here would + // turn the #559 guard into a message nobody fails on. + bool propagated = false; + try { + (void)GuardGoldenStage("some_case", "the runner for op \"nope\"", + [] { throw doctest::detail::TestFailureException{}; }); + } catch (const doctest::detail::TestFailureException&) { + propagated = true; + } + CHECK(propagated); +} + TEST_CASE("op parity vs upstream goldens (CPU)") { int non_op = 0; int cases = RunGoldenPass(Cpu(), &non_op);