fix(multimodal): pin the Qwen2-VL XLA loader contract and document all three outcomes - #987
Merged
Merged
Conversation
…l 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
inureyes
force-pushed
the
fix/issue-966-qwen2vl-xla-loader-contract
branch
from
July 31, 2026 02:42
a1bcbfa to
044a804
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was broken
multimodal::host_preprocessor::tests::xla_loader_keeps_text_and_unqualified_vlm_image_capability_falsehas failed deterministically onmainsinceedc0ebbb6(#915). It loops over["llama", "qwen2_vl"]assertingload_xla_image_preprocessor(...)returnsOk(None), but #915 madeqwen2_vlreturnErr(InvalidConfig("Qwen2-VL XLA image execution requires the xla-iree feature"))when the feature is absent, so the.unwrap()panics. That commit edited the same test file, adding an import and two export tests, and left this loop untouched, so the contract and the test that pins it diverged inside a single change to a single file.This is the third deterministically-failing test found on
mainin one week, after the two repaired in #946 and #953. It is the first one the nightly verification job added in #953 should catch on its own.Which side is right
The production behavior stays; the test changes. The reasoning, recorded here so it is not relitigated:
Qwen2-VL has no MLX vision fallback. The arm immediately above already rejects
MLXCEL_XLA_VISION_BACKEND=hostfor it with exactly that reason, so in a build withoutxla-ireethere is no code path that can embed its images.Ok(None)for such a checkpoint is indistinguishable from a text-only checkpoint, so a session would start with every image silently ignored, which is a worse outcome than a named failure at load.Failing at load also takes away nothing that worked.
xla-iree = ["xla-backend", "mlxcel-xla/iree"], and withoutmlxcel-xla/ireethe session'sprefillanddecode_stepreturnmlxcel_xla::NOT_WIRED, whose text is "the OpenXLA inference session was built without theireefeature; rebuild ...". Anxla-backend-only build cannot generate from any checkpoint at all, so there is no working configuration being broken here.Feature absence already errors elsewhere in the same function: an explicit
MLXCEL_XLA_VISION_BACKEND=ireereturnsErrfor the same reason.The counter-argument, addressed rather than dropped
Without
xla-iree, LLaVA does not error. Unless the policy is explicitlyireeit falls through to the host preprocessor and returnsOk(Some(host)). Qwen2-VL is the only family where a missing compile-time feature is fatal, and that asymmetry deserved an answer rather than silence.It is deliberate and now documented where a reader will find it: LLaVA has a complete MLX host vision tower and projector to fall back to, Qwen2-VL has none. The doc comment on
load_xla_image_preprocessorpreviously described only two of the three outcomes the function implements, which is precisely how the contract drifted from its test. It now covers all three (Ok(None),Ok(Some),Err), states which families reach each and why, and records theNOT_WIREDreasoning.Changes
The capability-false loop drops
qwen2_vland gainsmllama, so it still covers an unqualified VLM family rather than degrading to a text-only-only test, with an inline comment explaining whyqwen2_vlis deliberately absent and where its contract is pinned instead.A new
xla_loader_rejects_qwen2_vl_without_the_iree_feature, gated#[cfg(not(feature = "xla-iree"))]because with the feature the same config takes the IREE load path, asserts the error variant and that its message both names the missing feature and carries the--features xla-ireerebuild instruction. Both callers surface this string verbatim into a startup failure, so the instruction is the only actionable part an operator receives; the error text was extended to carry it.Validation
cargo test --release --lib --features metal,accelerate multimodal::host_preprocessorgoes from11 passed; 1 failedto13 passed; 0 failed.cargo fmt --all -- --checkand the cross-repo-ref guard are clean.No production behavior changes: the only non-test edits are the doc comment and the error string.
Closes #966