From 044a804c154ec51d3ce94d3597e93509e404edd2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 31 Jul 2026 11:10:36 +0900 Subject: [PATCH] fix(multimodal): pin the Qwen2-VL XLA loader contract and document all three outcomes `xla_loader_keeps_text_and_unqualified_vlm_image_capability_false` asserted `Ok(None)` for `qwen2_vl`, but #915 made that family return `Err` when the `xla-iree` feature is absent. The commit that changed the contract edited the same test file without touching this loop, so the test has failed deterministically on `main` ever since. The production behavior is correct and stays. Qwen2-VL has no MLX vision fallback, which is why `MLXCEL_XLA_VISION_BACKEND=host` is already rejected for it, so without `xla-iree` no path can embed its images. `Ok(None)` there is indistinguishable from a text-only checkpoint and would start a session that silently ignores every image. Failing at load takes nothing away that worked: `xla-iree` also enables `mlxcel-xla/iree`, and without it `prefill` and `decode_step` return `NOT_WIRED`, so such a build cannot generate from any checkpoint. `qwen2_vl` leaves the capability-false loop and is replaced by `mllama`, so the loop still covers an unqualified VLM family rather than only a text-only one, and a new `#[cfg(not(feature = "xla-iree"))]` test pins the error. That test asserts the message both names the missing feature and carries the rebuild instruction, because both callers surface the string verbatim and the instruction is the only actionable part an operator gets. The doc comment on `load_xla_image_preprocessor` described only two of the three outcomes it implements, which is how the contract and its test diverged in the first place. It now covers all three and records why the Qwen2-VL and LLaVA asymmetry is deliberate: LLaVA has a complete MLX host tower to fall back to, Qwen2-VL does not. Closes #966 --- src/multimodal/host_preprocessor.rs | 45 +++++++++++++++++++---- src/multimodal/host_preprocessor_tests.rs | 38 ++++++++++++++++++- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/src/multimodal/host_preprocessor.rs b/src/multimodal/host_preprocessor.rs index a4024cfd8..b63719c7e 100644 --- a/src/multimodal/host_preprocessor.rs +++ b/src/multimodal/host_preprocessor.rs @@ -132,16 +132,38 @@ pub trait HostMultimodalPreprocessor { /// Load the image preprocessor supported by the OpenXLA host-first path. /// -/// `Ok(None)` is the conservative result for text-only checkpoints and VLM -/// families whose processor/position contract has not been qualified for XLA -/// yet. Once a checkpoint is identified as the supported LLaVA family, missing -/// or malformed processor/projector weights are startup errors rather than a -/// capability downgrade. +/// Three outcomes are possible, one per axis this loader decides on: +/// +/// - `Ok(None)` is the conservative result for text-only checkpoints and VLM +/// families whose processor/position contract has not been qualified for XLA +/// yet. +/// - `Ok(Some(preprocessor))` is returned for a qualified family that can run +/// its vision path in this build. LLaVA reaches this even without the +/// `xla-iree` feature, because it has a complete MLX host vision tower and +/// projector to fall back to; only an explicit +/// `MLXCEL_XLA_VISION_BACKEND=iree` makes feature absence fatal there. +/// - `Err(..)` is returned for a qualified family this build cannot serve +/// images for. That covers missing or malformed processor/projector weights +/// on an identified LLaVA checkpoint, and Qwen2-VL in a build without +/// `xla-iree`. +/// +/// The Qwen2-VL asymmetry with LLaVA is deliberate, not an oversight. Qwen2-VL +/// has no MLX vision fallback, which is the same reason +/// `MLXCEL_XLA_VISION_BACKEND=host` is rejected for it below, so without +/// `xla-iree` no code path can embed its images. `Ok(None)` there would be +/// indistinguishable from a text-only checkpoint and would start a session that +/// silently ignores every image. Failing takes away nothing that worked: +/// `xla-iree` also enables `mlxcel-xla/iree`, and without that the session's +/// `prefill` and `decode_step` return `mlxcel_xla::NOT_WIRED`, so an +/// `xla-backend`-only build cannot generate from any checkpoint at all. Both +/// callers (`backend::xla::create_session` and the server's image-preprocess +/// stage) turn this error into a startup failure, so the message carries the +/// rebuild instruction the operator needs. /// /// # Errors /// -/// Returns a typed configuration or weight-loading error for a supported LLaVA -/// checkpoint that cannot construct its complete host preprocessor. +/// Returns a typed configuration or weight-loading error for a qualified +/// checkpoint that cannot construct a complete image path in this build. pub fn load_xla_image_preprocessor( model_path: &Path, ) -> Result>, HostPreprocessorError> { @@ -172,10 +194,17 @@ pub fn load_xla_image_preprocessor( ); return Ok(Some(Box::new(preprocessor))); } + // No MLX fallback exists for this family, so an image-capable session + // cannot be built here. Report it as a build-configuration error with + // the remedy attached: both callers surface this string verbatim, so + // the rebuild instruction is the only actionable part the operator + // gets. #[cfg(not(feature = "xla-iree"))] { return Err(HostPreprocessorError::InvalidConfig( - "Qwen2-VL XLA image execution requires the xla-iree feature".to_string(), + "Qwen2-VL XLA image execution requires the xla-iree feature; rebuild mlxcel with \ + `--features xla-iree` (this family has no MLX vision fallback)" + .to_string(), )); } } diff --git a/src/multimodal/host_preprocessor_tests.rs b/src/multimodal/host_preprocessor_tests.rs index a83e0f032..1b6120d6f 100644 --- a/src/multimodal/host_preprocessor_tests.rs +++ b/src/multimodal/host_preprocessor_tests.rs @@ -67,7 +67,12 @@ fn iree_vision_contract_policy_is_explicit_and_strict() { #[test] fn xla_loader_keeps_text_and_unqualified_vlm_image_capability_false() { - for model_type in ["llama", "qwen2_vl"] { + // `llama` is text-only and `mllama` is a VLM family whose processor and + // position contract has never been qualified for XLA. Both are capability + // downgrades rather than errors. `qwen2_vl` is deliberately not in this set: + // it is qualified, so a build that cannot run it is an error instead, pinned + // by `xla_loader_rejects_qwen2_vl_without_the_iree_feature` below. + for model_type in ["llama", "mllama"] { let model_dir = tempfile::tempdir().unwrap(); std::fs::write( model_dir.path().join("config.json"), @@ -82,6 +87,37 @@ fn xla_loader_keeps_text_and_unqualified_vlm_image_capability_false() { } } +/// Qwen2-VL is qualified for the XLA vision path but has no MLX fallback, so a +/// build without `xla-iree` has no way to embed its images. The loader reports +/// that as a build-configuration error instead of downgrading capability to +/// `Ok(None)`, which would be indistinguishable from a text-only checkpoint and +/// would start a session that silently ignores images. Gated to the build the +/// contract is about: with `xla-iree` the same config takes the IREE load path. +#[cfg(not(feature = "xla-iree"))] +#[test] +fn xla_loader_rejects_qwen2_vl_without_the_iree_feature() { + let model_dir = tempfile::tempdir().unwrap(); + std::fs::write( + model_dir.path().join("config.json"), + r#"{"model_type":"qwen2_vl"}"#, + ) + .unwrap(); + let error = load_xla_image_preprocessor(model_dir.path()) + .err() + .expect("qwen2_vl without xla-iree must fail startup, not downgrade image capability"); + let HostPreprocessorError::InvalidConfig(message) = &error else { + panic!("expected a build-configuration error, got {error:?}"); + }; + assert!( + message.contains("requires the xla-iree feature"), + "the message must name the missing feature: {message}" + ); + assert!( + message.contains("--features xla-iree"), + "the message must carry the actionable rebuild instruction: {message}" + ); +} + #[test] fn xla_loader_fails_startup_for_llava_missing_required_artifacts() { let model_dir = tempfile::tempdir().unwrap();