From 0a3e6f27d7151877e633c99a51aa7da836f7a6d3 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Mon, 17 Aug 2026 20:45:16 +0900 Subject: [PATCH] fix(vision): stop VLM wrappers re-enabling a padded prefill their backbone refuses LanguageModel::supports_padded_prefill defaults to true. Every hybrid and recurrent text model overrides it to false, and the comments say why: a tile-aligned prefill appends up to 31 pad positions, and while the causal mask and trim_caches_to_actual_len undo their effect on the KV caches, a Mamba / GatedDeltaNet / RWKV / DeltaCache state that has already absorbed them cannot be rewound. Qwen35VLModel and MiniCPMV46VLModel both hold a Qwen35Model, which answers false, and both forwarded four other capability predicates while leaving these two defaulted. So on Neural Accelerator hardware a text-only run through either wrapper padded the prompt to a 32-token tile and corrupted the backbone's recurrent state. Nothing failed. Greedy output just changed, and only when the prompt length was not already a multiple of 32. Measured on M5 Max with qwen3.8-27b-4bit, whose architectures field routes it through the VLM wrapper. A 75-token prompt padded to 96 and produced a different 120-token completion than the same prompt with padding disabled. After this change the padded and unpadded outputs agree, and so does speculative MTP decode, which never padded and was therefore right all along. That disagreement is how this was found (#1201); the classic path was the wrong one. The guard is a source-level test rather than a runtime one, because constructing every wrapper needs weights and the property is about which methods each impl block carries. It derives the refusing-backbone set by scanning src/models rather than hard-coding it, so a new hybrid family is covered on the day it lands. Verified to fail by removing one override. Fixes #1201 --- src/vision/minicpmv4_6_vl.rs | 23 +++ src/vision/qwen3_5_vl.rs | 23 +++ tests/vlm_wrapper_capability_delegation.rs | 195 +++++++++++++++++++++ 3 files changed, 241 insertions(+) create mode 100644 tests/vlm_wrapper_capability_delegation.rs diff --git a/src/vision/minicpmv4_6_vl.rs b/src/vision/minicpmv4_6_vl.rs index 8395ea514..08b5549e5 100644 --- a/src/vision/minicpmv4_6_vl.rs +++ b/src/vision/minicpmv4_6_vl.rs @@ -166,6 +166,29 @@ impl MiniCPMV46VLModel { } impl LanguageModel for MiniCPMV46VLModel { + /// Delegated, not defaulted: the trait's default is `true` and the + /// Qwen 3.5 backbone answers `false`, because it is a hybrid whose + /// GatedDeltaNet layers carry a recurrent state. Tile-aligned padded + /// prefill appends up to 31 pad positions, and while the causal mask and + /// `trim_caches_to_actual_len` undo their effect on the KV caches, a + /// recurrent state that has already absorbed them cannot be rewound. A + /// wrapper that inherits the default silently re-enables an optimization + /// its own backbone disabled for correctness, which is what this fixes + /// (#1201): on Neural Accelerator hardware a text-only run through this + /// wrapper produced different greedy output than the same prompt padded + /// to a tile boundary. + fn supports_padded_prefill(&self) -> bool { + mlxcel_core::generate::LanguageModel::supports_padded_prefill(&self.text_model) + } + + /// Delegated for the same reason as [`Self::supports_padded_prefill`]: + /// the maskless variant is only consulted once padding is allowed, but + /// leaving it defaulted would put the two predicates on different + /// sources of truth. + fn supports_maskless_padded_prefill(&self) -> bool { + mlxcel_core::generate::LanguageModel::supports_maskless_padded_prefill(&self.text_model) + } + fn forward( &self, input_ids: &MlxArray, diff --git a/src/vision/qwen3_5_vl.rs b/src/vision/qwen3_5_vl.rs index 123c60e0f..714918b53 100644 --- a/src/vision/qwen3_5_vl.rs +++ b/src/vision/qwen3_5_vl.rs @@ -420,6 +420,29 @@ impl LanguageModel for Qwen35VLModel { mlxcel_core::generate::LanguageModel::supports_batching(&self.text_model) } + /// Delegated, not defaulted: the trait's default is `true` and the + /// Qwen 3.5 backbone answers `false`, because it is a hybrid whose + /// GatedDeltaNet layers carry a recurrent state. Tile-aligned padded + /// prefill appends up to 31 pad positions, and while the causal mask and + /// `trim_caches_to_actual_len` undo their effect on the KV caches, a + /// recurrent state that has already absorbed them cannot be rewound. A + /// wrapper that inherits the default silently re-enables an optimization + /// its own backbone disabled for correctness, which is what this fixes + /// (#1201): on Neural Accelerator hardware a text-only run through this + /// wrapper produced different greedy output than the same prompt padded + /// to a tile boundary. + fn supports_padded_prefill(&self) -> bool { + mlxcel_core::generate::LanguageModel::supports_padded_prefill(&self.text_model) + } + + /// Delegated for the same reason as [`Self::supports_padded_prefill`]: + /// the maskless variant is only consulted once padding is allowed, but + /// leaving it defaulted would put the two predicates on different + /// sources of truth. + fn supports_maskless_padded_prefill(&self) -> bool { + mlxcel_core::generate::LanguageModel::supports_maskless_padded_prefill(&self.text_model) + } + fn supports_batched_prefill(&self) -> bool { mlxcel_core::generate::LanguageModel::supports_batched_prefill(&self.text_model) } diff --git a/tests/vlm_wrapper_capability_delegation.rs b/tests/vlm_wrapper_capability_delegation.rs new file mode 100644 index 000000000..3264719bb --- /dev/null +++ b/tests/vlm_wrapper_capability_delegation.rs @@ -0,0 +1,195 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! A VLM wrapper may not silently re-enable a padded prefill its own text +//! backbone disabled. +//! +//! `LanguageModel::supports_padded_prefill` defaults to `true`. Every hybrid +//! and recurrent text model in this tree overrides it to `false`, and the +//! comments say why: tile-aligned padded prefill appends up to 31 pad +//! positions, and although the causal mask and `trim_caches_to_actual_len` +//! undo their effect on the KV caches, a Mamba / GatedDeltaNet / RWKV / +//! DeltaCache state that has already absorbed them cannot be rewound. +//! +//! A vision wrapper that forwards to such a backbone but does not forward this +//! predicate answers `true` by default, and the offline generator then pads. +//! Nothing fails: it compiles, it runs, and greedy output silently changes on +//! Neural Accelerator hardware whenever the prompt length is not a multiple of +//! 32. That is #1201, found only because a speculative path that never pads +//! disagreed with the classic path that did. +//! +//! This is a source-level check rather than a runtime one because constructing +//! every wrapper needs weights. The property is about which method each `impl` +//! block carries, which the source states directly. + +use std::collections::BTreeSet; +use std::fs; +use std::path::{Path, PathBuf}; + +const PREDICATES: [&str; 2] = [ + "supports_padded_prefill", + "supports_maskless_padded_prefill", +]; + +fn repo_root() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) +} + +fn rust_sources(dir: &Path, out: &mut Vec) { + let Ok(entries) = fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + rust_sources(&path, out); + } else if path.extension().is_some_and(|e| e == "rs") { + out.push(path); + } + } +} + +/// The body of the `impl LanguageModel for X` block in `src`, if any, plus the +/// type name `X`. +/// +/// Brace-counted from the impl header rather than regex-matched, so a nested +/// block or a later `impl` for a different trait cannot leak in. +fn language_model_impl(src: &str) -> Option<(String, String)> { + let header = src.find("impl LanguageModel for ")?; + let after = &src[header + "impl LanguageModel for ".len()..]; + let name: String = after + .chars() + .take_while(|c| c.is_alphanumeric() || *c == '_') + .collect(); + let open = header + after.find('{')? + "impl LanguageModel for ".len(); + let mut depth = 0usize; + for (i, c) in src[open..].char_indices() { + match c { + '{' => depth += 1, + '}' => { + depth -= 1; + if depth == 0 { + return Some((name, src[open..open + i].to_string())); + } + } + _ => {} + } + } + None +} + +/// Text-model types whose `LanguageModel` impl answers `false` for `predicate`. +fn backbones_answering_false(predicate: &str) -> BTreeSet { + let mut files = Vec::new(); + rust_sources(&repo_root().join("src/models"), &mut files); + let needle = format!("fn {predicate}(&self) -> bool {{"); + let mut out = BTreeSet::new(); + for file in files { + let Ok(src) = fs::read_to_string(&file) else { + continue; + }; + let Some((name, body)) = language_model_impl(&src) else { + continue; + }; + if let Some(at) = body.find(&needle) { + let tail = &body[at + needle.len()..]; + let answer: String = tail + .chars() + .take_while(|c| *c != '}') + .filter(|c| !c.is_whitespace()) + .collect(); + // Skip anything that is not a bare literal: a computed answer is + // already delegating or deciding for itself. + if answer == "false" { + out.insert(name); + } + } + } + out +} + +/// Types a wrapper holds as a field, which is what "wraps" has to mean here. +/// +/// Matching a bare mention would fire on a doc comment or a use statement and +/// turn this test into noise the first time someone writes prose about a +/// backbone they do not embed. +fn field_types(src: &str) -> BTreeSet { + let mut out = BTreeSet::new(); + for line in src.lines() { + let line = line.trim(); + if line.starts_with("//") { + continue; + } + let Some((_, ty)) = line.split_once(':') else { + continue; + }; + for token in ty.split(|c: char| !(c.is_alphanumeric() || c == '_')) { + if !token.is_empty() { + out.insert(token.to_string()); + } + } + } + out +} + +#[test] +fn a_vision_wrapper_delegates_padded_prefill_when_its_backbone_refuses_it() { + let mut files = Vec::new(); + rust_sources(&repo_root().join("src/vision"), &mut files); + files.sort(); + + let mut violations = Vec::new(); + for predicate in PREDICATES { + let refusing = backbones_answering_false(predicate); + assert!( + refusing.contains("Qwen35Model") || predicate.contains("maskless"), + "expected at least the Qwen 3.5 backbone to refuse {predicate}; the \ + scanner probably stopped matching the source shape" + ); + for file in &files { + let Ok(src) = fs::read_to_string(file) else { + continue; + }; + let Some((wrapper, body)) = language_model_impl(&src) else { + continue; + }; + if body.contains(&format!("fn {predicate}(")) { + continue; + } + let fields = field_types(&src); + let wrapped: Vec<&String> = refusing.intersection(&fields).collect(); + if wrapped.is_empty() { + continue; + } + violations.push(format!( + "{}: `{wrapper}` holds {} but does not override `{predicate}`, so it \ + answers the trait default `true` and re-enables a padded prefill its \ + backbone disabled", + file.strip_prefix(repo_root()).unwrap_or(file).display(), + wrapped + .iter() + .map(|s| s.as_str()) + .collect::>() + .join(", "), + )); + } + } + + assert!( + violations.is_empty(), + "vision wrappers must forward the padded-prefill predicates to their text \ + backbone (#1201). Add a delegating override to each of:\n {}", + violations.join("\n ") + ); +}