Phi-3.5 exporter support: LongRoPE su + real CLIP vision + golden coverage (reworks #430) - #430
Merged
Merged
Conversation
Performance Comparison
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the golden-test YAML case coverage (L4/L5 lane) for several recently-added or recently-investigated models by adding a new canonical case and refining skip reasons to reflect reproduced, actionable blockers in the Mobius export/load path.
Changes:
- Added a new
L4+L5YAML case forModelCloud/glm-4-9b-gptq-4bitwith an explicit GPTQModel/Transformers loading blocker. - Updated the
skip_reasonforphi3_5-vision-instructto reflect current Mobius export/load blockers (RoPEsu+ CLIP CLS/positional weight mismatch). - Updated the
skip_reasonforphi4-reasoning-vision-15bto reflect current Mobius weight-loading/name-mapping issues for the SigLIP vision subgraph.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| testdata/cases/vision-language/phi4-reasoning-vision-15b.yaml | Replaces generic “too large” skip reason with a concrete, reproduced Mobius vision-subgraph/weight-mapping blocker. |
| testdata/cases/vision-language/phi3_5-vision-instruct.yaml | Replaces prior Transformers-compatibility skip with current Mobius RoPE/CLIP weight-shape support blockers. |
| testdata/cases/causal-lm/glm-4-9b-gptq-4bit.yaml | Adds a new canonical L4+L5 case for the GLM-4 GPTQ checkpoint, currently explicitly skipped due to GPTQModel block-pattern detection failure. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Phi-3/Phi-3.5 checkpoints label LongRoPE as rope_scaling type "su" (short/
long-factor scaled rotary embeddings); newer HuggingFace configs spell the
identical algorithm "longrope". The exporter recognized only "longrope" and
raised `ValueError: Unsupported rope type: su` when loading e.g. the
Phi-3.5-vision text tower.
Config extraction:
- Add a rope_type alias map in mobius/_configs/_base.py
(_ROPE_TYPE_ALIASES = {"su": "longrope"} + _canonical_rope_type) so both
spellings resolve to the existing LongRope code path. "su" is canonicalized
at extraction time rather than special-cased downstream.
Golden reference (transformers 5.x):
- Factor the DynamicCache legacy shims into one shared helper
_install_dynamic_cache_legacy_shims() used by both the causal-LM and
multimodal loaders. Fix a pre-existing bug where get_usable_length returned
past + new (must be past length only), which made the bundled Phi-3.5
modeling build a doubled causal mask; add to_legacy_cache and seen_tokens.
- load_torch_model now honors a trust_remote_code flag (default True) and the
golden generator passes the YAML case's value. Set phi-3_5-mini to
trust_remote_code: false so the transformers-native phi3 implementation is
used; the checkpoint's bundled modeling_phi3.py depends on cache APIs
removed in transformers 5.x.
- Un-skip testdata/cases/causal-lm/phi-3_5-mini.yaml and add regenerated L4/L5
golden files. L4 (prefill argmax) and L5 (generation) golden tests now pass.
Tests:
- Add TestLongRopeAliasExtraction: su and longrope produce identical
RoPEConfig, su dispatches to LongRope, end-to-end phi3 graph build, missing
original_max_position_embeddings, factor-length mismatch, and
non-alias-unchanged cases.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR #430 only added a skip_reason. This makes the mobius exporter actually load and build the Phi-3.5-Vision (model_type `phi3_v`) checkpoint's vision path, replacing the previous SigLIP-style stub that could neither load the CLIP CLS-token weights nor match the projector shape. Two concrete gaps fixed: 1. Non-standard vision config: the CLIP geometry lives in the `img_processor` dict, not a standard `vision_config`. The phi3_v vision hook now also parses `layer_idx` (feature layer, default -2) and sets `hidden_act=quick_gelu`; a general `feature_layer` field was added to VisionConfig. 2. CLIP CLS-token weights the exporter did not support: the vision encoder now uses the real CLIP tower (CLS token + pre_layrnorm + 577-entry position embedding) instead of the SigLIP `VisionModel`. It emits the exact HuggingFace `get_img_features` output: run to `hidden_states[-2]` (23 of 24 layers, no post_layernorm) and drop the CLS token. Implementation (DRY — reuses the existing CLIP infra in clip.py): - clip.py: `CLIPVisionModel` gains `feature_layer` + `drop_class_token`; only the encoder layers needed to reach the selected hidden state are built. `_rename_clip_vision_weight` now maps HF `patch_embedding.{weight,bias}` onto the `.projection` Conv sub-module (previously unmapped → unfed initializer), and CLIP's patch conv is now correctly bias-free. Added `ClipVisionConfigView` to bridge nested VisionConfig → CLIP's field names and `resolve_clip_feature_num_layers` for the hidden-states math. - phi3_v.py: `_Phi3VVisionEncoderModel` builds the CLIP tower and outputs raw patch features `(num_crops, 576, 1024)`; `_rename_phi3v_vision_weight` maps `img_processor.vision_model.*` → `vision_tower.*` and drops the host-side `img_projection` / `sub_GN` / `glb_GN` (HD feature transform). The `img_projection` MLP (fc1 is 3072x4096) and the HD 2x2 patch-merge with learnable separators stay host-side: the projector's 4096-wide input only exists after the image-size-dependent HD transform, which cannot be a static graph. The raw patch-feature tensor is the clean, faithful boundary. Tests (writing-tests skill): - clip_test.py: CLIP weight rename (patch/CLS/pre-layrnorm/mlp) + feature-layer resolver edge cases. - phi3_v_test.py: img_processor CLIP weight mapping + projector/separator drop. - _extractors_test.py: img_processor vision-config parsing (dims + layer_idx). - vision_integration_test.py: numeric parity for feature_layer=-2 + CLS drop vs a PyTorch CLIP reference (and CLIP patch conv is now bias-free). Verified: real Phi-3.5-vision checkpoint builds the vision encoder standalone, all 373 weight initializers populate, and ONNX Runtime produces (num_crops, 576, 1024). End-to-end golden un-skip additionally needs the rope_type=su text tower fix and a host-side HD-transform+projector shim (tracked separately). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ework/phi35-430-integrate
… (L4) Implement the real Phi-3.5-Vision host-side HD feature transform + img_projection in src/mobius/models/_phi3_vision_projector.py (pure NumPy, mirrors modeling_phi3_v.py get_img_features -> hd_feature_transform -> reshape_hd_patches_2x2merge -> add_image_newline -> img_projection): 2x2 spatial patch merge (N,576,1024)->(N,144,4096), learnable sub_GN/glb_GN separator insertion in the fixed sub_glb order, then Linear(4096->3072)->GELU->Linear. load_phi3_vision_projector_weights reads the projector + separator tensors (dropped from the ONNX graphs) directly from the checkpoint safetensors. Wire it into tests/e2e_golden_test.py via _run_vl_vision_to_image_features (flattens HF 5-D pixel_values, runs the ONNX vision encoder, applies the host-side transform) for both L4 prefill and L5 generation. Fix _build_mm_prompt to use Phi3V's inner-tokenizer chat template + img_tokens (<|image_1|>) placeholders. Un-skip testdata/cases/vision-language/phi3_5-vision-instruct.yaml and add the golden JSON regenerated from the real checkpoint + cat image. L4 golden passes (top-1 argmax matches HF over all 775 tokens); the host-side projector matches HF hd_feature_transform at cos=1.0000, max-abs 6.9e-5. L5 greedy generation is xfail (strict=False) with a precise reason: it drifts from HF after the first few tokens due to the same decoder-side LongRoPE parity gap already xfailed for phi4mm-multi-image-audio (shared Phi-3.5 decoder), not a vision/projector defect. Make load_torch_multimodal_model tolerant of older transformers (no AutoModelForImageTextToText; torch_dtype vs dtype) so the golden can be generated offline under transformers 4.43 (the bundled 4.x remote code cannot generate on transformers 5.x). Align a stale su-rope assertion in _config_resolver_test.py to Bishop's deliberate su->longrope canonicalization. Add 11 NumPy unit tests for the projector (2x2 merge ordering, separator insertion, GELU-MLP math, sub_glb token count/ordering, weight loader + error paths). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
force-pushed
the
test/l4-l5-golden-new-models
branch
from
July 25, 2026 17:36
1a34c82 to
f111938
Compare
Member
Author
|
🔧 Reworked per your feedback — this PR now fixes the mobius exporter modeling code itself instead of adding
Branch force-updated to the reworked content ( |
justinchuby
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/mobius/models/clip.py:261
- CLIPVisionModel.preprocess_weights() currently renames all CLIP vision weights, including encoder layers that are not instantiated when feature_layer is set (e.g. feature_layer=-2 skips the final encoder layer) and post_layernorm.* even though post_layernorm is not created in that mode. This contradicts the class docstring (“post_layernorm weights are intentionally left unmapped”) and produces warnings during apply_weights(). Filter out weights for skipped encoder layers and post_layernorm when feature_layer is active.
def preprocess_weights(
self, state_dict: dict[str, torch.Tensor]
) -> dict[str, torch.Tensor]:
new_state_dict = {}
for name, tensor in state_dict.items():
new_name = _rename_clip_vision_weight(name)
if new_name is not None:
new_state_dict[new_name] = tensor
return new_state_dict
Comment on lines
+224
to
+231
| from math import erf, sqrt | ||
|
|
||
| hidden = tokens @ weights.projection_first_weight.T + weights.projection_first_bias | ||
| # Exact GELU (erf form), matching torch.nn.GELU()'s default approximate="none". | ||
| gelu = np.vectorize(lambda value: 0.5 * value * (1.0 + erf(value / sqrt(2.0)))) | ||
| activated = gelu(hidden).astype(np.float32) | ||
| projected = activated @ weights.projection_second_weight.T + weights.projection_second_bias | ||
| return projected.astype(np.float32) |
Comment on lines
+274
to
+282
| per_image_token_blocks: list[np.ndarray] = [] | ||
| for image_index, (height, width) in enumerate(image_sizes): | ||
| height_crops = int(height) // _CROP_PIXEL_SIZE | ||
| width_crops = int(width) // _CROP_PIXEL_SIZE | ||
| crop_count = height_crops * width_crops | ||
|
|
||
| sub_features = image_features[image_index, 1 : 1 + crop_count] | ||
| sub_merged = _reshape_hd_patches_2x2_merge(sub_features, height_crops, width_crops) | ||
| sub_with_newline = _add_image_newline(sub_merged, sublayer_separator) |
Comment on lines
+777
to
+784
| # The HF processor emits pixel_values shaped | ||
| # (num_images, num_crops + 1, 3, 336, 336). The ONNX vision encoder | ||
| # takes 4-D NCHW input, so flatten the leading crop dimension. | ||
| pixel_values = np.asarray(processed["pixel_values"], dtype=np.float32) | ||
| num_images, num_crops_including_global = pixel_values.shape[:2] | ||
| flat_pixel_values = pixel_values.reshape( | ||
| num_images * num_crops_including_global, *pixel_values.shape[2:] | ||
| ) |
… xfail
Root-cause the Phi-3.5-Vision L5 greedy-generation drift that was xfailed as a
"decoder-side LongRoPE parity gap". It is NOT LongRoPE.
Evidence (fp32/fp64 oracle + instrumented decode):
* The LongRoPE cos/sin cache matches HF to ~3.6e-5 (a 1-2 ULP inv_freq
ordering difference amplified by position). An A/B rebuild that switched the
mobius inv_freq to HF's exact `1/(factor*theta**x)` ordering changed the
step-4 logits by <1e-4 and did NOT change any generated token — so rope
precision is not the cause.
* Instrumented decode shows the divergence is a HARD miss at the first
image-grounded token (step 4): the golden token ("P" of "Pallas's cat") is
ranked 481 in mobius with a 7.28-logit deficit, while mobius emits
"The image shows a mathematical expression written in LaTeX..." — the classic
Phi-3.5-Vision *blind* (no-image) hallucination.
Actual root cause: the embedding graph detected image slots with
`input_ids == image_token_id` (32044), but the HF Phi-3/3.5-Vision processor
marks image positions with *negative* placeholder ids (-1, -2, ...). The mask
therefore never fired, image features were never fused into the prompt
embeddings, and the decoder ran blind. The first few generic tokens
("The image shows a") match by text prior, then it diverges at the first token
that needs the image. L4 prefill "passed" only because its argmax ("The") is
image-independent.
Fix (mirrors modeling_phi3_v exactly):
* Detect image positions as `(input_ids < 0) & (input_ids > -MAX_INPUT_ID)`.
* Pad image_features with one trailing zero row before the Gather so decode
steps (which pass an empty 0-row image tensor) don't index an empty tensor
and crash — the padding is discarded by the fusion `Where`.
Result: phi3_5-vision L5 now matches the HF golden token-for-token (30/30);
L4 still passes; phi-3_5-mini L4+L5 unchanged; build_graph + cli fast lane green.
Cleared the phi3_5-vision L5 xfail.
The sibling phi4mm-multi-image-audio xfail is a genuinely different issue and is
left in place: its processor emits POSITIVE image/audio ids (200010/200011) that
the InputMixer already matches correctly, so its fusion is not affected.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
⭐ Update: Phi-3.5-vision L5 now PASSES (was xfail). Root-caused the L5 drift scientifically: built an fp32/fp64 LongRoPE oracle — the rope difference was ~3.6e-5 and flipped zero tokens, so it was not the previously-suspected LongRoPE gap. The real bug: the exporter matched image slots via positive Branch fast-forwarded to |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/mobius/models/_phi3_vision_projector.py:230
- _apply_image_projection uses np.vectorize over math.erf for GELU, which is a Python-level loop and will be very slow for real Phi-3.5-Vision token counts. Since torch is already a dependency in mobius, using torch.nn.functional.gelu(approximate="none") keeps exact GELU semantics but runs in optimized native code.
from math import erf, sqrt
hidden = tokens @ weights.projection_first_weight.T + weights.projection_first_bias
# Exact GELU (erf form), matching torch.nn.GELU()'s default approximate="none".
gelu = np.vectorize(lambda value: 0.5 * value * (1.0 + erf(value / sqrt(2.0))))
activated = gelu(hidden).astype(np.float32)
projected = activated @ weights.projection_second_weight.T + weights.projection_second_bias
Comment on lines
+22
to
+24
| # Largest int64 value; used as an open-ended ``Slice`` end (ONNX clamps it to | ||
| # the actual dimension size). | ||
| _INT64_MAX = 9223372036854775807 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+166
to
+168
| media_prefix = "".join( | ||
| f"{numbered_media_tokens[index]}\n" for index in range(len(media_paths)) | ||
| ) |
| image_features_per_crop = raw_patch_features.reshape( | ||
| num_images, num_crops_including_global, -1, image_dim_out | ||
| ) | ||
| projector_weights = load_phi3_vision_projector_weights(case.model_id) |
Comment on lines
+14
to
+16
| This module reproduces that host-side tail faithfully in NumPy, mirroring | ||
| ``modeling_phi3_v.py`` (``hd_feature_transform`` → ``reshape_hd_patches_2x2merge`` | ||
| → ``add_image_newline`` → ``img_projection``). The output is the flat sequence |
Remove the stale positive image-token field, validate numbered media placeholders, cache projector weights across L4/L5, and correct the host-projector dependency documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Status update from Sapper:
Code comments are addressed; awaiting Justin's review approval + merge. I did not merge this PR. |
Comment on lines
+225
to
+230
| hidden = tokens @ weights.projection_first_weight.T + weights.projection_first_bias | ||
| # Exact GELU (erf form), matching torch.nn.GELU()'s default approximate="none". | ||
| hidden_tensor = torch.from_numpy(hidden) | ||
| activated = (0.5 * hidden_tensor * (1.0 + torch.erf(hidden_tensor / np.sqrt(2.0)))).numpy() | ||
| projected = activated @ weights.projection_second_weight.T + weights.projection_second_bias | ||
| return projected.astype(np.float32) |
Comment on lines
+42
to
+50
| def _to_legacy_cache(self): # type: ignore[misc] | ||
| legacy_cache: tuple = () | ||
| for layer in getattr(self, "layers", []): | ||
| keys = getattr(layer, "keys", None) | ||
| values = getattr(layer, "values", None) | ||
| if keys is None or values is None: | ||
| continue | ||
| legacy_cache += ((keys, values),) | ||
| return legacy_cache |
Member
Author
|
Follow-up: the current PR description already explicitly documents the checked-in golden JSON artifacts under Validation. I replied to and resolved that stale description thread; there are now no unresolved current review threads. |
Verify projector checkpoint weights are loaded once per model across golden phases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Patch-coverage follow-up: added a focused cache test proving projector checkpoint weights load once per model across golden phases. Two focused golden-harness tests pass; pushed |
Comment on lines
+203
to
207
| self.vision_tower = CLIPVisionModel( | ||
| clip_config, | ||
| feature_layer=config.vision.feature_layer, | ||
| drop_class_token=True, | ||
| ) |
Use a dtype-matched sqrt scalar and build legacy caches linearly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cast the structural config view at the CLIP constructor boundary for mypy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Latest Copilot follow-ups are addressed through |
Comment on lines
14
to
17
| from mobius.components import FCMLP | ||
| from mobius.components._common import Embedding, LayerNorm | ||
| from mobius.components._conv import Conv2d | ||
| from mobius.components._common import INT64_MAX, Embedding, LayerNorm | ||
| from mobius.components._conv import Conv2d, Conv2dNoBias | ||
| from mobius.components._encoder import EncoderAttention |
Comment on lines
+226
to
+230
| renamed: dict[str, torch.Tensor] = {} | ||
|
|
||
| vision_pfx = "model.vision_embed_tokens.img_processor." | ||
| proj_0_pfx = "model.vision_embed_tokens.img_projection.0." | ||
| proj_2_pfx = "model.vision_embed_tokens.img_projection.2." | ||
|
|
||
| for key, value in state_dict.items(): | ||
| if key.startswith(vision_pfx): | ||
| renamed["vision_tower." + key[len(vision_pfx) :]] = value | ||
| elif key.startswith(proj_0_pfx): | ||
| renamed["multi_modal_projector.fc1." + key[len(proj_0_pfx) :]] = value | ||
| elif key.startswith(proj_2_pfx): | ||
| renamed["multi_modal_projector.fc2." + key[len(proj_2_pfx) :]] = value | ||
|
|
||
| new_key = _rename_phi3v_vision_weight(key) | ||
| if new_key is not None: | ||
| renamed[new_key] = value |
justinchuby
commented
Jul 27, 2026
| return snapshot_download(model_id_or_directory, local_files_only=True) | ||
|
|
||
|
|
||
| def load_phi3_vision_projector_weights( |
|
|
Use the public components API for CLIP, broaden the CLIP vision config contract, and filter unused CLIP layer weights when Phi-3.5-Vision exports an intermediate feature layer. Resolve cached Phi-3.5 projector shards via the HuggingFace cache index instead of requiring a full local snapshot, and remove the stale positive image-token constant. Add assertions covering skipped vision tower weights. Signed-off-by: justinchuby <justinchuby@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
force-pushed
the
test/l4-l5-golden-new-models
branch
from
July 29, 2026 17:32
f41a275 to
8fa912e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/vision_integration_test.py:155
- The state-dict key list in the class docstring still claims the patch embedding Conv has a bias ("projection.{weight,bias}"), but the module is now constructed with
bias=False, so there will be no...projection.biasparameter. Updating the docstring avoids confusion when debugging weight-name alignment/parity.
self.embeddings.patch_embedding.projection = nn.Conv2d(
_CHANNELS, _HIDDEN, kernel_size=_PATCH_SIZE, stride=_PATCH_SIZE, bias=False
)
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.
Rework of #430 — real Phi-3.5 exporter support (not skip-strings)
This PR fixes the exporter modeling code so the previously-unsupported Phi-3.5 models actually build and produce passing golden coverage — no more
skip_reasonstrings.What changed
1. Phi LongRoPE
rope_type="su"support (unblocks Phi-3.5-mini)suis Phi's older alias forlongrope. Mapped both to one code path via_ROPE_TYPE_ALIASES={"su":"longrope"}+_canonical_rope_type()in_configs/_base.py(DRY).get_usable_lengthbug in_testing/torch_reference.py(past+new → past-only; was doubling the causal mask). Shared by text + vision paths.2. Phi-3.5-vision real CLIP export (unblocks Phi-3.5-vision)
img_processornon-standard vision-config parsing, CLS-token drop (img_features[:,1:]),feature_layer=-2, bias-free patch conv.img_projectionprojector shim (_phi3_vision_projector.py): 2×2 spatial merge (576,1024)→(144,4096), sub_GN/glb_GN separators, MLP 4096→3072→GELU→3072. cos=1.0 parity vs HF (max abs 9.16e-5).3. Image-feature fusion fix — clears the L5 divergence ⭐
input_ids == 32044(positive id), but the HF Phi-3/3.5-Vision processor marks image slots with negative placeholder ids (-1, -2, …, with-MAX_INPUT_ID < id < 0). The mask never matched → image features were never fused → the decoder ran blind and produced the classic no-image hallucination at the first image-grounded token.modeling_phi3_vexactly: detect slots as(input_ids < 0) & (input_ids > -1e9), and padimage_featureswith one zero row so theGatheris valid at decode (0 image rows). Padding is discarded by the fusionWhere.first_div=None). The xfail is removed.Validation
_decompose_attention_test.pysoftcap failures remain (pre-existing on origin/main, unrelated).ruff check+ruff format --checkclean.Reviews (all fresh, authors locked out)
Leaving this for @justinchuby to merge (mobius PRs are not self-merged).