[codex] Support raw image refs for multimodal rendering - #89
Open
eligotts wants to merge 30 commits into
Open
Conversation
This was referenced Jun 18, 2026
Drop the cache-only None path. Every image (current and prior turns) carries its raw descriptor ref; _descriptor_only_mm_data no longer strips the pointer, so refs carry forward without a rebuild. Removes the now-orphaned materialize_image_refs / materialize_kimi_image_refs and the materialize_all_image_refs flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tale comments - Drop the render-time processor constructor arg from Qwen3VL/Qwen35/Kimi renderers: geometry is computed deterministically from config; no renderer runs the HF image processor at render. Remove Kimi dead _get_processor/_process_image/self._processor/_image_cache. - mm_store: remove all backcompat aliases (MMRAW_PREFIX, MM_RAW_PAYLOAD_KEY/VALUE, mmraw_ref, split_mmraw_ref, image_asset_dir) -- no consumers. - client.py: fix stale generate() docstring + comment that referenced the removed None/cache path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It only sized Kimi per-renderer image cache, which was deleted with the render-time processor path. No consumers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s-renderers # Conflicts: # renderers/configs.py # renderers/qwen3_vl.py
eligotts
marked this pull request as ready for review
June 29, 2026 16:36
ApprovabilityVerdict: Needs human review This PR introduces a new You can customize Macroscope's approvability policy. Learn more. |
- Fix bridge merge mutating the caller's previous sidecar in place: shared merge_multi_modal_data helper copies inner lists, replacing the three per-renderer merge blocks; bridge test asserts no mutation. - Import Qwen's smart_resize from transformers (torch-free PIL-backend module) instead of maintaining a port. - Resolve and read each raw image asset once per layout describe. - mm_store: full sha256 content-addressed filenames; raise on undecodable base64 instead of silently passing the data URL through. - Drop the dead features/mm_data tuple plumbing in client.generate. - Add layout-math parity test against the real Qwen3-VL and Kimi-K2.5 image processors at rounding-boundary dimensions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s-renderers Weaves main's independent bridge-mutation fix and thinking-deviation skip guard into this branch's raw/processed offload restructuring: - kimi_k25.py/qwen35.py/qwen3_vl.py: kept our shared merge_multi_modal_data() helper over main's three duplicated inline fixes for the same bug (identical behavior, single implementation). - test_multimodal.py: re-injected main's _skip_for_disabled_thinking_deviation guard into this branch's parallel renderer_cases/processor_cases structure at all 4 call sites; took main's tuple-based prior_mm/prior_counts assertion over our redundant duplicate. - test_client.py: kept both additions (non-overlapping). Full suite: 2578 passed, 153 skipped, 1 xfailed.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e64cc58. Configure here.
The ref rides as a string inside a JSON request body, so serializing the payload as compact JSON costs only the escaping of its own quotes. The base64 wrapper inflated the whole payload by a third for no benefit. The saving is small in offload mode (the payload is a short file:// URI), but it keeps the ref format identical to the inline bundle, which carries the image source in the payload and saves ~32 KiB per image slot. The payload contains colons now, so refs parse with a single partition; the base64-alphabet guard on the segment is gone with the wrapper.
…s-renderers # Conflicts: # renderers/client.py
Bare filesystem paths were a second route the rest of the stack can't honor anyway — prime-rl's trainer-side file_uri_to_path already rejects anything without a file:// scheme, and verifiers ingress only ever emits as_uri(). Folds the two-step helper into one and drops the dead branch.
Delete the baked QWEN_VL_IMAGE_LAYOUT / KIMI_K25_IMAGE_LAYOUT constants. The layout spec dataclass is now the canonical knob list: renderers fill it from the checkpoint's preprocessor_config.json (raw JSON read, no processor instantiation) via qwen_layout_from / kimi_layout_from, and fingerprint() hashes exactly its fields. Layout parity tests additionally assert our grid math against HF's metadata-only oracle get_number_of_image_patches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…derer Materialize-side fingerprint enforcement is gone (prime-rl trusts the train-time same-checkpoint contract and keeps grid asserts), so the fingerprint machinery goes with it: no fingerprint()/image_layout_fingerprint, no fingerprint field in mmraw refs or descriptors, and the extractors are Mapping-only (config JSON is the single source; no live-processor or SizeDict handling). Config resolution is one cache-aware hf_hub_download, resolved lazily once per renderer and stashed as the layout spec; describe/item helpers take the spec directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Prune the leftovers of the fingerprint era down to what the geometry math and the wire actually use: - Kimi spec drops image_mean/image_std (fingerprint inputs, not geometry) and the float_triple extraction; Qwen spec drops temporal_patch_size (grid_t is 1 for images). - Kimi's ref payload drops num_media_tokens — no consumer; adapters read grid_thws only. The descriptor field stays: the parity test asserts it against the processor's media_tokens_calculator. - The descriptor envelope drops its modality key: an item's modality is its key in MultiModalData.mm_items; only the standalone mmraw: ref carries its own (consumed by the vLLM-front routing check). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kimi and Qwen3.5 were importing file:// loaders, part detectors, and layout_model_name from qwen3_vl. Lift those cross-family helpers into renderers.mm_image so family modules no longer depend on each other for I/O utilities. Co-authored-by: Cursor <cursoragent@cursor.com>
SFT can re-run the image processor on duplicate images within a renderer instance; removing self._image_cache / PROCESSED_IMAGE_CACHE_MAX and the stale image_cache_max docs keeps the multimodal surface to one concept. Co-authored-by: Cursor <cursoragent@cursor.com>
Reject http(s) fetches and raw bytes; keep PIL, path/file://, and data:image base64 so SFT and offline callers still work without the renderer acting as an HTTP client. Co-authored-by: Cursor <cursoragent@cursor.com>
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.

Design update — inline/offload image storage
This PR now supports both raw image transport modes used by prime-rl:
offload: existing behavior, raw image bytes are written to run-scoped image assets and refs carry a file-backed image id.inline: data-image URIs remain inline and raw refs carry the inline source instead of requiringraw_image_id.This repo adds inline-capable
mmraw:v3refs while preservingmmraw:v2parsing, keeps Qwen image hashes aligned to the raw decoded bytes, and emits raw descriptor items with eitherraw_image_idorraw_uri.Validation after latest push:
uv run pytest tests/test_client.py -qpassed (14 passed).Design update — dropped the
None/cache-only image pathThis PR and its companions (prime-rl #2836 / verifiers #1746 / renderers #89) no longer use the "send
Nonefor already-cached images" mechanism. Every image carries its raw descriptor ref at every slot (current and prior turns);/inference/v1/generaterematerializes each ref from disk every request.Why: the
Nonepath coupled correctness to deployment (LRU cache present, single replica / DP-affinity, no eviction) and surfaced a miss as a hard vLLMEngineDeadError(qwen3-vl mrope dereferences aNoneimage_grid_thw) that the retry net couldn't catch across the engine→API IPC. Dropping it is deployment-agnostic (a miss is impossible) and non-hacky. vLLM'smm_hashencoder cache still skips the expensive GPU re-encode for free — we only forgo the cheap IPC/CPU-reprocess dedup.Validated: color-codeword (Qwen3-VL-4B) under DP=2, no affinity / no cache reliance: 0 crashes, 0
data=None, multi-turn accumulation correct, reward ~0.84. Also confirmed under TP.This repo: every image emits a raw descriptor ref at every slot.
_descriptor_only_mm_datano longer strips the pointer (pixel_valueswere never present in v1, so the strip was both stale and the root cause of the descriptor-only/rebuild churn). Removed thematerialize_all_image_refsflag and the now-orphanedmaterialize_image_refs/materialize_kimi_image_refs.Original description
Summary
mmraw:raw multimodal refs inrenderers.mm_store, parsed asRawMMRefobjects withfamily,modality, hash, image locator, and adapter-owned payloadprime_raw_mm_itemenvelopes instead of processed image payloads for Qwen-VL and Kimi K2.5 image renderingimage_grid_thwfor Qwen,grid_thws/media token metadata for Kimi)Companion PRs
Notes
file://.../assets/images/...refs before rendering.Validation
uvx ruff@0.15.18 check .passed.uvx ruff@0.15.18 format --check .passed.uvx 'ty<0.0.22' check .exited 0; remaining diagnostics are warning-level advisories under the repo config.PYTHONPATH=/home/ubuntu/renderers uv run --no-project --active pytest -q tests/test_client.pypassed:14 passed./home/ubuntu/renderers,/home/ubuntu/verifiers, and/home/ubuntu/prime-rl-v1-raw-mm-offloadcompleted inference, env rollouts, train batch creation, trainer step 0, and decoded strict trainer-bound raw image refs.Update: review hardening (
e3c12e9)render_completion_updatemutating the caller'sprevious_multi_modal_datain place (shallow dict copy +setdefault().extend()); the merge now lives in a sharedmerge_multi_modal_datahelper inbase.pythat copies inner lists, and the bridge test asserts the previous sidecar is unmutated.mm_storeuses full sha256 content-addressed filenames and raises on undecodable base64.test_raw_layout_math_matches_image_processor: parity against the real Qwen3-VL-4B and Kimi-K2.5 (pinned revision) processors at rounding-boundary dimensions. Full suite:2182 passed.Update: merged main (
e64cc58)Reconciled with renderers
main(12 commits: Hy3/PrimeQwen3/LagunaXS21 configs, kept-tokens parsing, SFT stop-token opt-in, a qwen thinking-wrapper fix, andbdb96b0— main's own independent fix for the same bridge-mutation bug this PR'smerge_multi_modal_data()already covers).kimi_k25.py/qwen35.py/qwen3_vl.py: kept this branch's sharedmerge_multi_modal_data()helper over main's three duplicated inline fixes for the identical bug — same behavior, one implementation instead of three.test_multimodal.py: re-injected main's_skip_for_disabled_thinking_deviationskip guard into this branch's parallelrenderer_cases/processor_casesstructure at all 4 call sites; took main's tuple-basedprior_mm/prior_countsassertion over this branch's redundant duplicate check.test_client.py,__init__.py,configs.py: non-overlapping additions on both sides, no semantic conflict.client.py: no textual conflict (main's fix landed near, not on, this branch's rewritten_build_vllm_mm_features) — manually verified the mergedgenerate()is consistent (prompt_attr/mm_datanaming intact).Full suite:
2578 passed, 153 skipped, 1 xfailed.Update: unwrapped the ref payload (
7c58fd6)raw_mm_refserializes its payload as compact JSON instead of base64-wrapping it. The ref travels as a string inside a JSON request body, so the wrapper's 33% inflation bought nothing. The saving is small in offload mode (the payload is a shortfile://URI), but it keeps the ref format identical to the inline bundle, where the payload carries the image source and the wrapper cost ~32 KiB per image slot. Refs parse with a singlepartition(":")now that the payload contains colons.Update: synced with main (
1af6589)Merged renderers
main(5 commits: vLLM token-serving import fix #115, GLM tool-name validation #114, setuptools bump, ruff-action pin).One conflict, in
client.py: main's #115 updated the vLLMmm_serdeimport path inside the processed-tensor encoder that this PR deletes by design. Kept this branch's raw-ref_build_vllm_mm_features; main's fix applies to code that no longer exists here.Full suite: 2622 passed, 125 skipped, 1 xfailed.
Update: single raw-mode image route (
66505d2)Raw mode now accepts exactly one image source: an offloaded
file://URL. The bare-filesystem-path fallback was a second route the rest of the stack couldn't honor anyway — prime-rl's trainer-sidefile_uri_to_pathalready rejects non-file://sources and verifiers ingress only ever emitsas_uri(). Anything else raises the existing "requires offloaded file:// image assets" error.Update: layout knobs sourced from the checkpoint config (
f0d1dd9)Deleted the baked
QWEN_VL_IMAGE_LAYOUT/KIMI_K25_IMAGE_LAYOUTconstants. The layout spec dataclass is now the canonical knob list per family:preprocessor_config.json— a cached raw JSON read (hub_image_processor_config), never a processor instantiation, so render hosts still avoid torch and (for Kimi) trust-remote-code execution. No baked fallback: a missing knob fails loudly.qwen_layout_from/kimi_layout_from) fills the spec from either the config dict or a live image processor object (prime-rl's adapters use the latter, deleting their hand-duplicated knob lists). Qwen honors both config shapes (top-levelmin_pixels/max_pixels, or nestedsize.shortest_edge/longest_edge) and live-processorSizeDictattributes.fingerprint()on the spec hashes exactly its own fields, so the knob list and the hash can't drift. Fingerprint values are byte-identical to before.get_number_of_image_patches(Qwen), and asserts config-sourced spec == live-processor spec for both families (this caught aSizeDictextraction bug in review).New runtime requirement: the tokenizer must carry
name_or_pathand the checkpoint'spreprocessor_config.jsonmust be resolvable (local path, HF cache, or hub); failures surface at first image render.Full suite: 2622 passed, 125 skipped, 1 xfailed.
Update: fingerprints deleted; layout resolved once per renderer (
29d443c)Companion to prime-rl's "trust the train-time checkpoint contract" change: materialize no longer re-checks layout knobs (it keeps output-level grid asserts, which need no knob extraction), so the fingerprint machinery is deleted end to end — no
image_layout_fingerprint, nofingerprint()on the layout specs, nofingerprint/layout_fingerprintfields inmmraw:refs or descriptor envelopes (a ref format change; nothing is merged, both bundles move together).Render-side resolution is also simplified to the cleanest shape:
hub_image_processor_configis a local-path check plus one cache-awarehf_hub_download(the previouslocal_files_onlytwo-step re-implemented hub internals).qwen_layout_from/kimi_layout_from) are Mapping-only — config JSON is the single knob source; the live-processor/SizeDictduck-typing is gone with its only caller._raw_image_layout), mirroring the existing lazy_get_processoridiom;describe_*_layout/*_image_item_for_rendertake the spec directly instead of a model name.Full suite: 2622 passed, 125 skipped, 1 xfailed.
Update: pruned unread layout knobs and envelope fields (
aa4b0c6)Review follow-up to the fingerprint deletion — removed everything only fingerprints consumed:
image_mean/image_std(+ thefloat_triplevalidator); Qwen spec dropstemporal_patch_size(grid_tis 1 for images). Specs now carry exactly what the geometry math reads.num_media_tokens(no consumer; adapters readgrid_thwsonly). The descriptor field stays — the parity test asserts it against the processor'smedia_tokens_calculator.modalitykey: an item's modality is its key inMultiModalData.mm_items; only the standalonemmraw:ref carries its own (consumed by the vLLM-front routing check).Full suite: 2622 passed, 125 skipped, 1 xfailed.
Note
High Risk
Changes core multimodal wire format, vLLM client encoding, and image input requirements (file offload only in raw mode), with companion PRs required for end-to-end rollout.
Overview
Multimodal inference path is reworked so Qwen-VL, Qwen3.5, and Kimi K2.5 no longer run HF image processors on the default path. They predict placeholder counts from checkpoint
preprocessor_config.jsonand emitprime_raw_mm_itemenvelopes (family payload +raw_image_uri).multimodal_outputon base renderer config selectsraw(default, inference) vsprocessed(SFT-stylepixel_values). Raw mode requires offloadedfile://assets; sharedmm_store/mm_imagehandle offload, refs, and local image I/O.renderers.client.generatedrops vLLM/torch-specific Qwen tensor encoding and builds a generic features payload of hashes, placeholders, andmmraw:refs per slot (including prior turns—no cache-onlyNoneslots).image_cache_maxand constructorprocessorargs are removed; processors load lazily only forprocessed.Bridge turns use
merge_multi_modal_dataso prior sidecars are merged without mutating caller state.AutoRendererConfigpropagatesmultimodal_output. Optionalrenderers[vision]extra adds Pillow/torch for processed paths. Tests and docs shift parity fixtures tofile://images and assert raw-ref serialization and layout math vs real processors.Reviewed by Cursor Bugbot for commit 2fbf8bd. Bugbot is set up for automated code reviews on this repo. Configure here.