[bong-water-water-bong]

vision tower test: the join's two row counts, pinned on a real pack with no oracle needed

npu/vision_plan.h defines PiecePlan::rows as "grid_h/4 * grid_w/4" -- the rows the text model receives -- and
npu/vision_tower.h:44 says the optional tower out-parameter receives "step 5's output, (gh/2 x gw/2, hidden): the tower
before the merger". Two different quantities, one merger apart, and until now the only place they were forced to agree was
minicpm_image_probe.cpp:252,260 -- at run time, on a 21 GB text model.

This adds that to the tower's own test, gated on ONEBIT_MINICPM_PACK alone:

out.size() / geometry().out_hidden   == plan.source.rows        one text row per placeholder
tower.size()                           == (gh/2) * (gw/2) * hidden   the tower before the merger

Measured on the real MiniCPM-V-4.7-35B-A3B-NPU2 vision component: 17 checks, 0 failed. Without the pack the arm prints
"did not run" and the suite reports 15 checks, so CI -- which has no pack -- is unaffected.

Three things worth knowing, all of which cost me a failed attempt first:

1. The plan is built at four patches per side, not at the pack's 980-pixel image. scale_resolution must be at least
   4 * patch, and a larger request is resized by best_resize(), which silently changes the grid the plan describes. 4 * patch
   is the smallest geometry the planner leaves alone, and this arm is about the row counts rather than about running a 70 x 70
   grid on four cores.
2. The first version of this arm compared the plan against tower, and was wrong by exactly the merger's second halving --
   tower is the pre-merger buffer. That is the mistake the arm now pins against.
3. const vis::Pack pk(std::string(pdir)); is a function declaration (most vexing parse); it needs a named string.

The reference arm is unchanged: it still needs ONEBIT_MINICPM_ORACLE for its pixel/tower/out goldens, and this one deliberately
needs nothing but the pack a lane already has.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2324 - Partially compliant Compliant requirements: The PR adds a test that verifies the correct row counts in the vision tower output and the pre-merger tower output, which is a necessary step for condition 6 The PR ensures that the vision tower's behavior matches the expected geometry for image processing, which is required for condition 6 Non-compliant requirements: The PR does not fully implement the picture prefix cache functionality (condition 8) - it only adds a test to verify row counts The PR does not actually run the full vision tower inference pipeline with image inputs, which is required for condition 6 Requires further human verification: The actual performance measurement of ttft for the prefix cache (condition 8) cannot be verified through code review alone The full integration of the vision tower with the text model and cache mechanism (condition 6) requires hardware testing

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incomplete Implementation of Condition 6 The PR adds a test to verify the row counts in the vision tower output, but does not actually implement the full vision tower inference pipeline with image inputs. This is required to fulfill condition 6 regarding image processing. { const char* pdir = std::getenv("ONEBIT_MINICPM_PACK"); if (pdir && *pdir) { try { // `Pack(pack_dir)` reads config.json and the container it names. The plan is built at FOUR PATCHES PER SIDE // over the pack's own patch size, not at the pack's 980-pixel image: 4 * patch is the smallest geometry the // planner leaves alone (scale_resolution must be at least 4 * patch, and a bigger request is resized), and // this arm is about the two row counts, not about running a 70 x 70 grid on four cores. const std::string pdir_s = pdir; // `Pack pk(std::string(pdir))` is a function declaration const vis::Pack pk(pdir_s); const vis::Geometry& g = pk.geometry(); const int side = 4 * g.patch; const vis::ImagePlan plan = vis::plan_image(side, side, /*max_slice_nums=*/1, /*scale_resolution=*/side, /*patch=*/g.patch); const vis::PiecePlan& src = plan.source; const int gh = src.grid_h, gw = src.grid_w; std::vector<float> px(size_t(3) * size_t(gh * g.patch) * size_t(gw * g.patch)); for (size_t k = 0; k < px.size(); ++k) px[k] = float(int(k % 251) - 125) / 128.0f; std::vector<float> tower; const std::vector<float> out = vis::tower_forward(pk, px.data(), gh, gw, &tower); const size_t text_rows = g.out_hidden ? out.size() / size_t(g.out_hidden) : 0; check(int(text_rows) == src.rows, "join (real pack): the merger hands the text model exactly PiecePlan::rows rows"); check(tower.size() == size_t(gh / 2) * size_t(gw / 2) * size_t(g.hidden), "join (real pack): the tower before the merger is grid/2 x grid/2 rows"); } catch (const std::exception& e) { check(false, std::string("join (real pack): opened and ran (") + e.what() + ")"); } } else { std::printf(" (ONEBIT_MINICPM_PACK not set: the join arm did not run)\n"); } } Missing Prefix Cache Implementation The PR does not implement the picture prefix cache functionality required by condition 8. It only adds a test to verify row counts, but does not actually test the caching behavior or ttft improvements. { const char* pdir = std::getenv("ONEBIT_MINICPM_PACK"); if (pdir && *pdir) { try { // `Pack(pack_dir)` reads config.json and the container it names. The plan is built at FOUR PATCHES PER SIDE // over the pack's own patch size, not at the pack's 980-pixel image: 4 * patch is the smallest geometry the // planner leaves alone (scale_resolution must be at least 4 * patch, and a bigger request is resized), and // this arm is about the two row counts, not about running a 70 x 70 grid on four cores. const std::string pdir_s = pdir; // `Pack pk(std::string(pdir))` is a function declaration const vis::Pack pk(pdir_s); const vis::Geometry& g = pk.geometry(); const int side = 4 * g.patch; const vis::ImagePlan plan = vis::plan_image(side, side, /*max_slice_nums=*/1, /*scale_resolution=*/side, /*patch=*/g.patch); const vis::PiecePlan& src = plan.source; const int gh = src.grid_h, gw = src.grid_w; std::vector<float> px(size_t(3) * size_t(gh * g.patch) * size_t(gw * g.patch)); for (size_t k = 0; k < px.size(); ++k) px[k] = float(int(k % 251) - 125) / 128.0f; std::vector<float> tower; const std::vector<float> out = vis::tower_forward(pk, px.data(), gh, gw, &tower); const size_t text_rows = g.out_hidden ? out.size() / size_t(g.out_hidden) : 0; check(int(text_rows) == src.rows, "join (real pack): the merger hands the text model exactly PiecePlan::rows rows"); check(tower.size() == size_t(gh / 2) * size_t(gw / 2) * size_t(g.hidden), "join (real pack): the tower before the merger is grid/2 x grid/2 rows"); } catch (const std::exception& e) { check(false, std::string("join (real pack): opened and ran (") + e.what() + ")"); } } else { std::printf(" (ONEBIT_MINICPM_PACK not set: the join arm did not run)\n"); } }

[bong-water-water-bong]

Closing as superseded — by #2335, which landed (6) and (8) properly, and by my own premise going stale.

When I opened this, the justification was that the join's two row counts were checked only by minicpm_image_probe.cpp:252,260, at run time on a 21 GB model. That stopped being true: #2335 (merged, 2d1324312) wires pictures on the chat route, adds npu/vision_request.{h,cpp}, and its tests/npu_image_request_test.cpp covers the decode, multiple pictures, the refusals, and vision::images_hash — the pixel-covering cache key my #2334 comment had said was missing. The arithmetic is now covered where it is used, which is better than covered by a test on a side.

What this PR did add, for the record, is the one thing still not covered elsewhere: the invariant on a real pack (MiniCPM-V-4.7-35B-A3B-NPU2, 1.1 GB vision component, no oracle) rather than on fixtures — 17 checks, 0 failed. If someone wants that arm, the patch is small and its three traps are documented on #2324: build the plan at 4 patches per side (a bigger request is resized by best_resize(), which silently changes the grid), compare the plan against out and never tower (the pre-merger buffer), and note that const Pack pk(std::string(pdir)); is a function declaration.

Closing rather than rebasing: a test whose stated reason for existing has been fixed by a better change is maintenance, not evidence.

[bong-water-water-bong]

VERDICT: READY 6476f8b

This is the right place to put this invariant, and it fixes a real gap in where a bad assumption could have lived.

Two quantities one merger apart, pinned against the geometry rather than against each other:

check(text_rows == src.rows,                  "the merger hands the text model exactly PiecePlan::rows rows");
check(tower.size() == (gh/2)*(gw/2)*hidden,   "the tower before the merger is grid/2 x grid/2 rows");

Pinning both against gh/gw is stronger than pinning their ratio: it implies the merger's 2×2 and catches a geometry that is wrong for both. Before this, the only place those two numbers were forced to agree was minicpm_image_probe.cpp:252,260 — at run time, on a 21 GB text model — so a mismatch would have surfaced as a wrong or refused answer on the heaviest path in the repo, discovered by whoever happened to run it.

ONEBIT_MINICPM_PACK alone gates it, which matters: the tower is the host f32 path, so this runs with no device, no npu-hold ticket and no text model — a condition that a lane can satisfy while queueing for something else.

The failure path is deliberate and I agree with it: a pack that opens and then throws reports check(false, "opened and ran (…)") with e.what() attached — so set-but-broken is a failure, not a skip, and the message carries the reason. That is the distinction I have argued for all night in the other direction: skipping is right when the input is absent, wrong when it is present and malformed.

One note, and it is about the next reader rather than this change: the comment explains that PiecePlan::rows is "grid_h/4 * grid_w/4" and the pre-merger tower is "grid_h/2 * grid_w/2", i.e. the merger is a fixed 2×2. If a future vision family merges differently (a 3×3, or a token-selection merger rather than pooling), these two assertions become the statement of this family's merger and will fail informatively — which is what you want, provided the comment names it as the 2×2 assumption rather than as arithmetic. One clause would make that explicit.

Not verified by me: a run against a real MiniCPM pack (ONEBIT_MINICPM_PACK), which I do not have. The assertions and the gating are what I read, and both are host-side.