feat(cpu): add Qwen3.5 0.8B multi-image support - #696
Conversation
📝 WalkthroughWalkthroughQwen3.5 now supports multiple ordered still images per prompt. The change updates CLI parsing, tokenization, preprocessing, vision processing, embedding injection, position IDs, validation, documentation, and CPU tests. ChangesQwen3.5 multi-image flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Qwen35Runner
participant Qwen3_5Tokenizer
participant Qwen3_5ImagePreprocessor
participant Qwen3_5VisionModel
participant Qwen3_5Model
Qwen35Runner->>Qwen3_5Tokenizer: pass ordered image_paths
Qwen3_5Tokenizer->>Qwen3_5ImagePreprocessor: preprocess image_paths
Qwen3_5ImagePreprocessor-->>Qwen3_5Tokenizer: concatenated patches and image grids
Qwen3_5Tokenizer->>Qwen3_5VisionModel: process image patch ranges
Qwen3_5VisionModel-->>Qwen3_5Model: concatenated image features
Qwen3_5Model->>Qwen3_5Model: inject features and generate position IDs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
mllm/utils/Argparse.hpp (1)
82-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the append semantics of the vector specialization.
Argument<std::vector<std::string>>::parseappends one value per occurrence of the flag. This differs from every other type, which overwrites. Add a short comment so callers understand that repeated flags accumulate in argument order.The coding guidelines require clear comments for public APIs.
♻️ Proposed comment
} else { + // Vector arguments accumulate: each occurrence of the flag appends one + // value, preserving command-line order. Scalar types overwrite instead. if constexpr (std::is_same_v<T, std::vector<std::string>>) { value_.push_back(value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/utils/Argparse.hpp` around lines 82 - 84, Add a concise public-API comment adjacent to the std::vector<std::string> branch in Argument::parse, documenting that each repeated flag appends one value and preserves argument order, unlike scalar types that overwrite.Source: Coding guidelines
mllm/models/qwen3_5/tokenization_qwen3_5.hpp (1)
365-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the magic offset and the unused loop index.
Line 375 adds
5to the match position. The value is tied to the length of"user\n". If the marker changes, the insertion point silently moves into the wrong place. Bind the offset to the marker length. The loop at line 372 also does not usei, and the48reserve is a second magic value.♻️ Proposed refactor
const bool has_image = !message.image_paths.empty(); auto applied_string = Qwen3_5Message::message_template; if (has_image) { - const auto user_content = applied_string.find("user\n"); + static constexpr std::string_view kUserMarker = "user\n"; + static constexpr std::string_view kImageMarker = "<|vision_start|><|image_pad|><|vision_end|>"; + const auto user_content = applied_string.find(kUserMarker); if (user_content == std::string::npos) { throw std::runtime_error("Qwen3.5 message template is malformed"); } std::string image_markers; - image_markers.reserve(message.image_paths.size() * 48); - for (size_t i = 0; i < message.image_paths.size(); ++i) { - image_markers += "<|vision_start|><|image_pad|><|vision_end|>"; - } - applied_string.insert(user_content + 5, image_markers); + image_markers.reserve(message.image_paths.size() * kImageMarker.size()); + for (size_t i = 0; i < message.image_paths.size(); ++i) { image_markers.append(kImageMarker); } + applied_string.insert(user_content + kUserMarker.size(), image_markers); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/tokenization_qwen3_5.hpp` around lines 365 - 376, Update the image-marker insertion in Qwen3_5Message handling to derive the insertion offset from the matched "user\n" marker length instead of the literal 5. Replace the unused index-based loop with iteration over the image paths, and derive the reserve size from the marker string rather than the literal 48.tests/cpu/Qwen35MultimodalTest.cpp (1)
97-122: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the image fixtures collision-safe and clean them up on failure.
The test writes two files with fixed names into the shared temporary directory. Two concurrent test processes use the same paths, so a parallel run can read a half-written fixture. If an assertion throws, lines 120-121 do not run and the files stay on disk.
Use a unique per-run subdirectory and remove it with a scope guard.
♻️ Proposed change
TEST_F(Qwen35MultimodalTest, PreprocessesAndConcatenatesDifferentImagesInOrder) { - const auto temp_dir = std::filesystem::temp_directory_path(); + const auto temp_dir = std::filesystem::temp_directory_path() + / ("mllm_qwen35_multi_image_" + std::to_string(::getpid())); + std::filesystem::create_directories(temp_dir); + struct Cleanup { + std::filesystem::path dir; + ~Cleanup() { std::error_code ec; std::filesystem::remove_all(dir, ec); } + } cleanup{temp_dir}; - const auto first_path = temp_dir / "mllm_qwen35_multi_image_first.ppm"; - const auto second_path = temp_dir / "mllm_qwen35_multi_image_second.ppm"; + const auto first_path = temp_dir / "first.ppm"; + const auto second_path = temp_dir / "second.ppm"; @@ EXPECT_FLOAT_EQ(patches.ptr<float>()[4 * 1536], 1.0F); - - std::filesystem::remove(first_path); - std::filesystem::remove(second_path); }
::getpid()needs<unistd.h>on POSIX. For a portable identifier, usestd::to_string(std::hash<std::thread::id>{}(std::this_thread::get_id()))or a static counter combined withstd::chrono::steady_clock::now().time_since_epoch().count(). The coding guidelines require portability across Linux and Windows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cpu/Qwen35MultimodalTest.cpp` around lines 97 - 122, Update the PreprocessesAndConcatenatesDifferentImagesInOrder test to create a unique per-run subdirectory under the temporary directory using a portable identifier, then place both image fixtures inside it. Add a scope guard that removes the entire subdirectory on all exits, including assertion failures, and remove the direct cleanup calls.Source: Coding guidelines
mllm/models/qwen3_5/multimodal_qwen3_5.hpp (1)
199-199: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument why the advance ignores
grid_t.Line 199 advances the shared position by
max(grid_h, grid_w) / spatial_merge_size. The reference rule advances by the maximum over all three axes, including the temporal axis. The two rules agree only whilegrid_t == 1. Still images always satisfy that, and the model rejects video tokens upstream, so the current behavior is correct. Add a comment that records this assumption. Video support will needgrid_tin the maximum.♻️ Proposed comment
+ // Still images always have grid_t == 1, so the temporal axis never sets the + // maximum. Video input must include grid_t in this maximum. current_position += std::max(grid_h, grid_w) / spatial_merge_size;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/multimodal_qwen3_5.hpp` at line 199, Add a concise comment immediately above the position update in the relevant multimodal processing code, documenting that the temporal dimension is intentionally omitted because video tokens are rejected upstream and supported inputs have grid_t == 1; note that video support must include grid_t in the maximum. Leave the existing current_position calculation unchanged.mllm/models/qwen3_5/image_preprocessor_qwen3_5.hpp (1)
72-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the rank before you read
shape()[1], and document the overload.Line 80 reads
processed.first.shape()[1]before line 81 checksshape().size() != 2. The current single-image operator always returns a rank-2 tensor, so the read is safe today. If that contract changes, the read goes out of bounds. Swap the two statements.The coding guidelines require a comment on public APIs. Add a short comment that states the return contract: concatenated patches in prompt order plus one grid row per image.
♻️ Proposed change
+ // Preprocesses every image in prompt order. Returns concatenated patches in + // [total_patches, patch_features] and one [t,h,w] grid row per image. std::pair<Tensor, Tensor> operator()(const std::vector<std::string>& image_paths) const { if (image_paths.empty()) { throw std::invalid_argument("Qwen3.5 image path list must not be empty"); } std::vector<std::pair<Tensor, Tensor>> processed_images; processed_images.reserve(image_paths.size()); int32_t total_patches = 0; int32_t patch_features = -1; for (const auto& image_path : image_paths) { auto processed = (*this)(image_path); + if (processed.first.shape().size() != 2) { + throw std::runtime_error("Qwen3.5 preprocessed images have incompatible patch features"); + } if (patch_features < 0) patch_features = processed.first.shape()[1]; - if (processed.first.shape().size() != 2 || processed.first.shape()[1] != patch_features) { + if (processed.first.shape()[1] != patch_features) { throw std::runtime_error("Qwen3.5 preprocessed images have incompatible patch features"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mllm/models/qwen3_5/image_preprocessor_qwen3_5.hpp` around lines 72 - 83, In the vector overload of operator(), validate processed.first.shape().size() == 2 before reading shape()[1], then enforce the existing patch-feature compatibility check. Add a brief public-API comment documenting that it returns concatenated patches in prompt order plus one grid row per image.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@mllm/models/qwen3_5/image_preprocessor_qwen3_5.hpp`:
- Around line 72-83: In the vector overload of operator(), validate
processed.first.shape().size() == 2 before reading shape()[1], then enforce the
existing patch-feature compatibility check. Add a brief public-API comment
documenting that it returns concatenated patches in prompt order plus one grid
row per image.
In `@mllm/models/qwen3_5/multimodal_qwen3_5.hpp`:
- Line 199: Add a concise comment immediately above the position update in the
relevant multimodal processing code, documenting that the temporal dimension is
intentionally omitted because video tokens are rejected upstream and supported
inputs have grid_t == 1; note that video support must include grid_t in the
maximum. Leave the existing current_position calculation unchanged.
In `@mllm/models/qwen3_5/tokenization_qwen3_5.hpp`:
- Around line 365-376: Update the image-marker insertion in Qwen3_5Message
handling to derive the insertion offset from the matched "user\n" marker length
instead of the literal 5. Replace the unused index-based loop with iteration
over the image paths, and derive the reserve size from the marker string rather
than the literal 48.
In `@mllm/utils/Argparse.hpp`:
- Around line 82-84: Add a concise public-API comment adjacent to the
std::vector<std::string> branch in Argument::parse, documenting that each
repeated flag appends one value and preserves argument order, unlike scalar
types that overwrite.
In `@tests/cpu/Qwen35MultimodalTest.cpp`:
- Around line 97-122: Update the
PreprocessesAndConcatenatesDifferentImagesInOrder test to create a unique
per-run subdirectory under the temporary directory using a portable identifier,
then place both image fixtures inside it. Add a scope guard that removes the
entire subdirectory on all exits, including assertion failures, and remove the
direct cleanup calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d03d5dce-cf1c-480c-a6ff-ba0375b85e23
📒 Files selected for processing (9)
examples/qwen3_5/README.mdexamples/qwen3_5/main.cppmllm/models/qwen3_5/image_preprocessor_qwen3_5.hppmllm/models/qwen3_5/modeling_qwen3_5.hppmllm/models/qwen3_5/modeling_qwen3_5_vision.hppmllm/models/qwen3_5/multimodal_qwen3_5.hppmllm/models/qwen3_5/tokenization_qwen3_5.hppmllm/utils/Argparse.hpptests/cpu/Qwen35MultimodalTest.cpp
2 participants
What this PR does
Extends the existing Qwen3.5-0.8B single-image multimodal CPU path with
ordered multi-image input:
--image_patharguments in the desktop and Android runnerEach image runs through the vision tower independently before its embeddings
are concatenated in prompt order. This preserves per-image attention isolation
and avoids dense vision attention over the total patch count of all images.
End-to-end multi-image demo
Prompt
Qwen3.5-0.8B Multimodal on OnePlus 13T
The auxiliary Pixel 9 Pro XL run produced the same response. Both runs verified
the model, tokenizer, images, runner, and loaded candidate runtime libraries.
Review guide
main.cpp,Argparse.hpp,tokenization_qwen3_5.hppimage_preprocessor_qwen3_5.hpp{N, 3}gridsmodeling_qwen3_5_vision.hpp,modeling_qwen3_5.hpp,multimodal_qwen3_5.hppQwen35MultimodalTest.cpp,README.mdSuggested review order: template/preprocessing → vision execution → MRoPE and
embedding injection → tests and runner.
Current-head validation
Candidate HEAD:
bc64f0abecca9cf1c150de5affffbc76fb33118dMerged-main base:
cc86c4ca8b2233edbf137f6b3e53bfdd11017d31arm64-v8abuild plus AArch64 ELF, Build ID, dependency, and bundle-hash audits passedFull validation details
git diff --checkpassed; temporary debug/dump interfaces were absent[[1,14,20],[1,22,46]]; 354 input tokens; image spans[4,74)and[76,329)694ad3b15a73fdb82752a679ae6009f36a0571ec; runnere76e40394419e1979391175edb63c178a4f69b9elibMllmRT.soBuild IDb0d780d6df31db5c9f0158b4815828566e897657;libMllmCPUBackend.sob712bfd2b6eeb1e8289f9dedbe2440ac22905d64This image shows two tabby cats lying on a bright pink surface, appearing to be asleep or resting.The device artifacts were built and validated from source bytes identical to
the final commit; creating the commit did not modify the tested files. These
runs establish the declared correctness and integration paths, not formal
image-quality, numerical-parity, or performance claims.
How to use it
Repeat
--image_pathin the intended prompt order:mllm-qwen3-5-runner \ --model_path /path/to/model.mllm \ --model_version v2 \ --tokenizer_path /path/to/tokenizer.json \ --config_path examples/qwen3_5/config_0.8B_multimodal_w4a32_kai.json \ --image_path /path/to/first.jpg \ --image_path /path/to/second.png \ --prompt "Describe the first and second images."Known limits
Extends the single-image multimodal support merged in #695.
Summary by CodeRabbit
--image_pathoption can now be repeated to provide images in order.